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] Add reporting interface with error #10727

Merged
merged 14 commits into from
Mar 6, 2019

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Feb 13, 2019

In Metricbeat modules we often see the pattern that before fetching the actual data, some processing is done and checks for errors happen. If an error happened, we have 3 lines of code:

  • creating the error with wrap
  • reporting the error
  • logging the error

By introducing the reporter interface with support for return an error I would like to eliminate the overhead and allow to directly return and error which is then reported and also logged.

So far we logged the error on the Error level. I'm now wondering if we should log these errors actually on the Info level as it's normally not a misbehaving of the Beat but the service does not respond as expected. So for the operator of the Beat normally no actions are needed.

As an example metricset I took php_fpm.process.

@ruflin ruflin added discuss Issue needs further discussion. review Metricbeat Metricbeat technical_debt labels Feb 13, 2019
@ruflin ruflin requested review from a team as code owners February 13, 2019 13:56
metricbeat/mb/mb.go Outdated Show resolved Hide resolved
@ruflin ruflin force-pushed the reporter-error-interface branch from db23d86 to 1d03797 Compare February 13, 2019 13:57
Copy link
Contributor

@cachedout cachedout left a comment

Choose a reason for hiding this comment

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

I like this a lot! I was just wondering about this last night so I am very happy to see it.

metricbeat/mb/module/wrapper.go Show resolved Hide resolved
@ycombinator
Copy link
Contributor

So far we logged the error on the Error level. I'm now wondering if we should log these errors actually on the Info level as it's normally not a misbehaving of the Beat but the service does not respond as expected. So for the operator of the Beat normally no actions are needed.

I'm still in favor of logging errors at the ERROR level. From a debugging perspective, when looking at the Metricbeat logs, it makes it really easy to spot errors. Then, from the error, we can figure out if it's a service error or a beat error. My concern with dropping service errors down to INFO level is that they'll get lost in the log noise pretty easily, making debugging harder.

@kaiyan-sheng
Copy link
Contributor

Maybe adding a log level as input parameter so we can set to ERROR or INFO depends on the each specific case?

Copy link
Contributor

@sayden sayden left a comment

Choose a reason for hiding this comment

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

Nice idea!

@ruflin
Copy link
Contributor Author

ruflin commented Feb 14, 2019

For the logging level, have a look at my longer comment here: #10727 (comment) I think error logging should be focused on the production and not the setup / testing use case.

@ruflin ruflin force-pushed the reporter-error-interface branch 2 times, most recently from bf89bbd to c6d1ada Compare February 14, 2019 12:51
@ruflin ruflin force-pushed the reporter-error-interface branch from c6d1ada to 9c09c1c Compare February 15, 2019 20:53
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

I'd try to avoid adding a new fetcher interface, but as we can still reuse ReporterV2 and it is so easy to implement from the ReportingMetricSetV2 one I am not going to complain 🙂

metricbeat/mb/module/wrapper.go Show resolved Hide resolved
metricbeat/module/elasticsearch/node/node.go Outdated Show resolved Hide resolved
metricbeat/mb/module/wrapper.go Show resolved Hide resolved
@ruflin ruflin force-pushed the reporter-error-interface branch from 1b91a13 to cd2bf04 Compare March 4, 2019 13:51
metricbeat/mb/testing/modules.go Outdated Show resolved Hide resolved
metricbeat/mb/testing/data_generator.go Outdated Show resolved Hide resolved
@ruflin ruflin removed the discuss Issue needs further discussion. label Mar 5, 2019
@ruflin ruflin force-pushed the reporter-error-interface branch from a86ffb4 to 681f41a Compare March 6, 2019 13:58
@ruflin ruflin merged commit 45074a3 into elastic:master Mar 6, 2019
@ruflin ruflin deleted the reporter-error-interface branch March 6, 2019 15:04
ycombinator added a commit that referenced this pull request Apr 12, 2019
…ogs errors (#11763)

Refactors code in the `kibana` Metricbeat module to use the new `Fetch` interface introduced in #10727.

Note that x-pack code paths in this module were not refactored to use the new interface as we don't want errors from those code paths to be reported into `metricbeat-*` indices, only logged to Metricbeat logs.

Related: #11767.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants