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

[Metricbeat] migrate nginx module to ReporterV2 error #11414

Conversation

berfinsari
Copy link
Contributor

See #11374 and #10727

@berfinsari berfinsari requested a review from a team as a code owner March 24, 2019 02:33
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@ruflin ruflin added module review Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Mar 25, 2019
@ruflin
Copy link
Member

ruflin commented Mar 25, 2019

jenkins, test this

Copy link
Member

@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.

I think the CI failure is related. In [fail] 0.10% test_cmd.TestCommands.test_modules_test_error we use the nginx module to test the error output. With this change, it's logged now on the Info level (which I think is fine and expected) but we need to adjust the tests there.

@fearful-symmetry
Copy link
Contributor

@ruflin Do we need to change whatever test harness is using the nginx moule?

@fearful-symmetry fearful-symmetry self-assigned this Mar 26, 2019
@fearful-symmetry
Copy link
Contributor

self.log_contains("ERROR error making http request"),

Did we just hard-code expected error values into the testing framework? The error body that's returned also seems completely different, but I could be reading the CI logs wrong.

@ruflin
Copy link
Member

ruflin commented Mar 27, 2019

@fearful-symmetry Yes, we need to adjust these tests. For the hardcoded errors: Yes, they are there :-) I think the issue is that now the errors are on the INFO level and not ERROR level anymore.

@fearful-symmetry
Copy link
Contributor

Ah, it looks like it's not a log level problem, just the addition of errors.Wrap that conflicts with what the test is looking for? We have the assert looking for:

 self.log_contains("ERROR error making http request"), 

and our new output from travis is:

ERROR error in http fetch: error making http request: Get http://errorhost:80/server-status: lookup errorhost: no such host

So the problem is just the error in http fetch

@berfinsari I think the solution is just to update test_cmd.py with the new string introduced by errors.Wrap()

@fearful-symmetry
Copy link
Contributor

@berfinsari Do you still want to work on this?

@sayden
Copy link
Contributor

sayden commented Jul 17, 2019

Ping @berfinsari 🙂

Don't feel pressed. It's perfectly ok if you don't have time now, anyone else can continue with this 😉

@sayden
Copy link
Contributor

sayden commented Sep 16, 2019

Merged already in #13518

Thanks for the help anyways, @berfinsari 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Metricbeat Metricbeat module review Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants