-
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
[Metricbeat] Fix: mixed modules fail loading standard and light metricsets #15011
[Metricbeat] Fix: mixed modules fail loading standard and light metricsets #15011
Conversation
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
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.
Thanks for working on this! Just a suggestion on the modules info method renaming.
MetricSets(module string) ([]string, error) | ||
DefaultMetricSets(module string) ([]string, error) | ||
MetricSets(r *Register, module string) ([]string, error) | ||
DefaultMetricSets(r *Register, module string) ([]string, error) |
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.
Many methods here start to need the main register, we may need to rethink this interface and the relation between module sources. But no need to address this in this PR.
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.
This is valid concern. I admit it took me a while to find the best solution for the moment (don't refactor half of the project). I'm afraid that it will be related with heavier refactoring.
…csets (elastic#15011) * Prepare test case for mixed module * Ignore registered modules while loading light metricsets * Test case: fail on unregistered module * Fix: mage fmt * Fix: replace String with ...Info * Rename method to ModulesInfo() (cherry picked from commit 1769f67)
…csets (elastic#15011) * Prepare test case for mixed module * Ignore registered modules while loading light metricsets * Test case: fail on unregistered module * Fix: mage fmt * Fix: replace String with ...Info * Rename method to ModulesInfo() (cherry picked from commit 1769f67)
On tests, loading any metricset from the AWS module is trying to load the billing metricset as light metricset, what fails. This shouldn't happen after #15011, but it is probably happening because on tests, not all metricsets are registered. billing metricset was refactored to a native implementation recently, in #20527. By now, remove billing from the list so tests can be executed.
On tests, loading any metricset from the AWS module is trying to load the billing metricset as light metricset, what fails. This shouldn't happen after elastic#15011, but it is probably happening because on tests, not all metricsets are registered. billing metricset was refactored to a native implementation recently, in elastic#20527. By now, remove billing from the list so tests can be executed. (cherry picked from commit 43f9bbc)
On tests, loading any metricset from the AWS module is trying to load the billing metricset as light metricset, what fails. This shouldn't happen after #15011, but it is probably happening because on tests, not all metricsets are registered. billing metricset was refactored to a native implementation recently, in #20527. By now, remove billing from the list so tests can be executed. (cherry picked from commit 43f9bbc)
Issue: #14698
This PR adjusts current behavior of the metricbeat which fails initialization of mixed modules while loading standard and light metricsets.
Added two test cases.