Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Metricbeats] Migrate uwsgi/status to ReporterV2 #11153

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented Mar 8, 2019

See #10774

This one was a bit of a pain, as it has a lot more tests and varies from the normal module setup in a number of ways. I ended up having to change a bit (and was tempted to clean some things up, but that's probably a job for a separate PR) so if anything seems unnecessary or weird just tell me.

I also didn't commit the updated data.json, as this PR is large enough and @sayden advised me against it.

@fearful-symmetry fearful-symmetry added Team:Integrations Label for the Integrations team technical_debt labels Mar 8, 2019
@fearful-symmetry fearful-symmetry requested a review from a team March 8, 2019 14:55
@fearful-symmetry fearful-symmetry changed the title Migration/mb/reporterv2/uwsgi/status [Metricbeats] Migrate uwsgi/status to ReporterV2 Mar 8, 2019
@ruflin
Copy link
Contributor

ruflin commented Mar 8, 2019

@fearful-symmetry I definitively would recommend to update the data.json as it will make it much easier to verify that the PR changes does the right. Perhaps I miss something here? We skipped some data.json in the k8s as it was not easy to generate them but I don't think it's the case here.

totals := findItems(events, "total")
assert.Equal(t, 1, len(totals))
}

func TestData(t *testing.T) {
compose.EnsureUp(t, "uwsgi_http")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We started to skip the EnsureUp in the TestData runs as it would boot up the environment but then on CI it skips the generation anyway. And when generating it locally, I personally always boot up the env manually.

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. One minor comment for the skip and it would be great to have the data.json update. It will validate mostly that the change is as expected, but don't be surprised if the values change, this is ok.

@fearful-symmetry
Copy link
Contributor Author

Updated to fix a test and the compose line that @ruflin mentioned
Re, the data.json file: It dates back to the commit where the uwsgi module was added, and I couldn't even reproduce it before I started the migration. I suspect it's hand-built?

@fearful-symmetry
Copy link
Contributor Author

fearful-symmetry commented Mar 8, 2019

new data.json at @ruflin 's request.

"read_errors": 0,
"pid": 1
},

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing that empty line in here it definitively indicates this was generated manually. I'm glad we have a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fearful-symmetry
Copy link
Contributor Author

jenkins failure seems unrelated.

@fearful-symmetry
Copy link
Contributor Author

jenkins, test this

@fearful-symmetry fearful-symmetry merged commit 0d10900 into elastic:master Mar 8, 2019
@fearful-symmetry fearful-symmetry deleted the migration/mb/reporterv2/uwsgi/status branch March 8, 2019 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants