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.

Sign up for GitHub

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

Jump to bottom

Enable checks for jolokia documented fields #10979

Merged
merged 7 commits into from
Mar 7, 2019
Merged

Enable checks for jolokia documented fields #10979

merged 7 commits into from
Mar 7, 2019

Conversation

kaiyan-sheng
Copy link
Contributor

@kaiyan-sheng kaiyan-sheng commented Feb 27, 2019 •
edited by alvarolobato
Loading

Fixes #10925

@kaiyan-sheng kaiyan-sheng requested review from a team as code owners February 27, 2019 21:23
sayden
sayden reviewed Feb 27, 2019
View reviewed changes
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.

I might be wrong but I think that when you add a new field and you run make update more files than the ones present here are updated. I think that docs and metricbeat.reference.yml. Can you double check?

@kaiyan-sheng
Copy link
Contributor Author

@sayden this is a field for output metrics seem like, not a config field 😄 that's why metricbeat.reference.yml didn't change.

@kaiyan-sheng kaiyan-sheng added in progress Pull request is currently in progress. Team:Integrations Label for the Integrations team labels Feb 27, 2019
@jsoriano
Copy link
Member

@kaiyan-sheng thanks for starting with this!
I see this is in progress, but take into account that the autodiscover providers are available in all Beats, so these fields should be added to some common place.
Ideally each autodiscover provider should have its own fields.yml file. Docker and Kubernetes didn't need them because they use fields in common with processors, so Jolokia will be the first one.
Take a look to processors, they have a _meta/fields.yml, we could add something similar for autodiscover providers.

@kaiyan-sheng
Copy link
Contributor Author

@jsoriano Thanks for pointing this out! This will be a bigger change than just fixing the test then 😄

@kaiyan-sheng
Copy link
Contributor Author

@jsoriano @sayden Thank you so much for your help! I still can't get this test to run locally 😢 Is it OK to keep this PR as this and in the future to move these fields into fields.yml under autodiscover_jolokia similar to the processor?

ruflin
ruflin reviewed Mar 5, 2019
View reviewed changes
metricbeat/module/jolokia/_meta/fields.yml
@@ -11,3 +11,31 @@
description: >
jolokia contains metrics exposed via jolokia agent
fields:
- name: agent.version
Copy link
Contributor

Choose a reason for hiding this comment

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

Not directly related to this PR but I just realise now that this at one stage should become an ECS field and Metricbeat in this case is the observer. @jsoriano is this correct?

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean setting observer.version to this field and observer.type to jolokia? I guess it could make sense, but yeah, nothing to be done on this PR about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yesish. This agent.version would end up on the top level an observer.version would be the current beat version. It's a more general issue we have but just jumped in my face here :-D

ruflin
ruflin reviewed Mar 5, 2019
View reviewed changes
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

This needs a changelog entry as I would consider it a bug that it was not documented.

jsoriano
jsoriano reviewed Mar 5, 2019
View reviewed changes
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.

Ok to move the fields to some common place in a follow up.

And yes, testing this is a bit hard as it depends on UDP multicast, we might add some tests mocking the network.

metricbeat/docs/fields.asciidoc Outdated Show resolved Hide resolved
metricbeat/module/jolokia/_meta/fields.yml
@@ -11,3 +11,31 @@
description: >
jolokia contains metrics exposed via jolokia agent
fields:
- name: agent.version
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean setting observer.version to this field and observer.type to jolokia? I guess it could make sense, but yeah, nothing to be done on this PR about this.

metricbeat/module/jolokia/_meta/fields.yml Outdated Show resolved Hide resolved
metricbeat/tests/system/test_autodiscover_jolokia.py Show resolved Hide resolved
@jsoriano
Copy link
Member

jsoriano commented Mar 6, 2019

It seems a make update is missing.

@kaiyan-sheng
Copy link
Contributor Author

@jsoriano Yeah! I just ran make update, thanks!

@kaiyan-sheng kaiyan-sheng added review and removed in progress Pull request is currently in progress. labels Mar 6, 2019
ruflin
ruflin approved these changes Mar 7, 2019
View reviewed changes
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

@kaiyan-sheng We should also backport this?

@kaiyan-sheng
Copy link
Contributor Author

@kaiyan-sheng We should also backport this?

Yes, how about 7.0?

@kaiyan-sheng kaiyan-sheng reopened this Mar 7, 2019
@kaiyan-sheng kaiyan-sheng added needs_backport PR is waiting to be backported to other branches. v7.0.0 labels Mar 7, 2019
@kaiyan-sheng kaiyan-sheng merged commit e031688 into elastic:master Mar 7, 2019
@kaiyan-sheng kaiyan-sheng deleted the jolokia_doc branch March 7, 2019 18:37
@kaiyan-sheng kaiyan-sheng removed needs_backport PR is waiting to be backported to other branches. v7.0.0 labels Mar 11, 2019
kaiyan-sheng added a commit that referenced this pull request Mar 12, 2019
@kaiyan-sheng
Cherry-pick to 7.0: Enable checks for jolokia documented fields (#10979…
a8fcb61 
…) (#11144)

* Enable checks for jolokia documented fields (#10979)

* Enable checks for jolokia documented fields
* Remove test fields before assert_fields_are_documented

(cherry picked from commit e031688)

* Fix changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sayden sayden sayden left review comments

@jsoriano jsoriano jsoriano left review comments

@ruflin ruflin ruflin approved these changes

Assignees

@kaiyan-sheng kaiyan-sheng

Labels
review Team:Integrations Label for the Integrations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kaiyan-sheng @jsoriano @ruflin @sayden @alvarolobato