[Metricbeat] Introduce Logger per metricset #11106
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently each Metricset initialises its own logger. The selector has to be set manually. This PR moves this code to the initialisation of the Metricset, meaning each metricset has its own logger. This should reduce the code needed in each Metricset and removes errors like putting a wrong or inconsistent selectors. It's different from before where there was a global logger for all instances of the same metricset and now each metricset instance has it's own logger. This should not make a difference. As an example implementation the `system.fsstat` metricset was adjusted.
ruflin
added
discuss
jsoriano
approved these changes
Mar 6, 2019
62 tasks
andrewkroh
reviewed
Mar 6, 2019
| @@ -106,6 +107,7 @@ type MetricSet interface { | |||
| HostData() HostData // HostData returns the parsed host data. | |||
| Registration() MetricSetRegistration // Params used in registration. | |||
| Metrics() *monitoring.Registry // MetricSet specific metrics | |||
| Logger() *logp.Logger // MetricSet specific logger | |||
There was a problem hiding this comment.
I like the idea, though I would have went with a shorter Log() name in effort to keep the code lines shorter like (ms.Log().Info(...)).
There was a problem hiding this comment.
@andrewkroh I like that, will open a follow up PR.
10 tasks
ruflin
added a commit
to ruflin/beats
that referenced
this pull request
Mar 7, 2019
Based on the comment elastic#11106 (comment) the Logger method is renamed to Log. It turned out this was already almost a convention which we used across the metricsets. This meant the Log variable and Log() method conflicted and required an update in the metricsets that had a Log variable. No functionality was changed in this PR.
ycombinator
added a commit
that referenced
this pull request
Apr 11, 2019
With #11106, the base `Metricset` struct started defining a `Logger()` method which returned a `*logp.Logger` for metricsets to use. With this change, each metricset no longer needs to define its own logger. This PR updates Elastic Stack metricsets to use the base `Metricset`'s `Logger()` instead of defining their own.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently each Metricset initialises its own logger. The selector has to be set manually. This PR moves this code to the initialisation of the Metricset, meaning each metricset has its own logger. This should reduce the code needed in each Metricset and removes errors like putting a wrong or inconsistent selectors.
It's different from before where there was a global logger for all instances of the same metricset and now each metricset instance has it's own logger. This should not make a difference.
As an example implementation the
system.fsstatmetricset was adjusted.