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] Fix: mixed modules fail loading standard and light metricsets #15011

Merged
merged 6 commits into from
Dec 11, 2019

Conversation

mtojek
Copy link
Contributor

@mtojek mtojek commented Dec 9, 2019

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.

@mtojek mtojek added Team:Integrations Label for the Integrations team [zube]: In Review review labels Dec 9, 2019
@zube zube bot removed the review label Dec 9, 2019
@mtojek mtojek requested a review from a team December 9, 2019 21:36
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

👍 lgtm

@mtojek mtojek requested review from a team and ChrsMark December 10, 2019 08:56
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.

Thanks for working on this! Just a suggestion on the modules info method renaming.

metricbeat/mb/registry.go Outdated Show resolved Hide resolved
MetricSets(module string) ([]string, error)
DefaultMetricSets(module string) ([]string, error)
MetricSets(r *Register, module string) ([]string, error)
DefaultMetricSets(r *Register, module string) ([]string, error)
Copy link
Member

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.

Copy link
Contributor Author

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.

@mtojek mtojek requested a review from jsoriano December 11, 2019 15:37
@mtojek mtojek merged commit 1769f67 into elastic:master Dec 11, 2019
mtojek added a commit to mtojek/beats that referenced this pull request Dec 11, 2019
…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)
mtojek added a commit to mtojek/beats that referenced this pull request Dec 11, 2019
…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)
mtojek added a commit that referenced this pull request Dec 12, 2019
…csets (#15011) (#15061)

* 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)
jsoriano added a commit that referenced this pull request Sep 18, 2020
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.
jsoriano added a commit to jsoriano/beats that referenced this pull request Sep 18, 2020
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)
jsoriano added a commit that referenced this pull request Sep 18, 2020
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants