-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Refactoring: use new Fetch interface that automatically reports and logs errors #11763
Refactoring: use new Fetch interface that automatically reports and logs errors #11763
Conversation
5e0f2f3
to
8569a60
Compare
8569a60
to
a75e1f3
Compare
Pinging @elastic/stack-monitoring |
} | ||
|
||
if m.XPackEnabled { | ||
intervalMs := m.calculateIntervalMs() | ||
err = eventMappingStatsXPack(r, intervalMs, now, content) | ||
if err != nil { | ||
m.Logger().Error(err) | ||
return | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be return err here? Also is m.Logger above still needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see the note in the PR description about x-pack code paths 😄.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh good to know! Thanks!
Reviewers: I plan to put up similar PRs to this one for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. +1 on having at least 1 PR per module and not all in one.
now := time.Now() | ||
|
||
m.fetchStats(r, now) | ||
err := m.fetchStats(r, now) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, just realised this might be odd when x-pack is enabled and we send up an error event ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it won't be odd because of this line: https://github.com/elastic/beats/pull/11763/files#diff-8710596f0decaae3ca552cedea1eb0a0R153
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment around that line now so hopefully it's a bit clearer now.
Refactors code in the
kibana
Metricbeat module to use the newFetch
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 and #11795.