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

Improve Jolokia docs and remove proxy restriction #7969

Merged
merged 3 commits into from
Aug 20, 2018
Merged

Improve Jolokia docs and remove proxy restriction #7969

merged 3 commits into from
Aug 20, 2018

Conversation

dedemorton
Copy link
Contributor

@dedemorton dedemorton commented Aug 15, 2018 •
edited
Loading

Summary of changes:

Closes #6897 and #4344.

NOTE: I plan to backport this change to 6.4 only (unless there is a strong objection and reviewers think this must be in 6.3)

@dedemorton dedemorton added docs review needs_backport PR is waiting to be backported to other branches. labels Aug 15, 2018
@dedemorton dedemorton requested a review from ruflin August 15, 2018 01:03
@dedemorton dedemorton added the in progress Pull request is currently in progress. label Aug 15, 2018
@dedemorton dedemorton removed the request for review from ruflin August 15, 2018 01:05
@dedemorton
Copy link
Contributor Author

Sorry, this isn't ready for review after all. I just noticed another old issue related these docs. I'll keep this PR open and add those changes: #4344

@dedemorton dedemorton changed the title Remove restriction now that metricset supports authentication when using Jolokia proxy Improve Jolokia docs and remove proxy restriction Aug 16, 2018
dedemorton
dedemorton commented Aug 16, 2018
View reviewed changes
metricbeat/module/jolokia/_meta/docs.asciidoc
https://jolokia.org/reference/html/agents.html[Jolokia agents] running on a
target JMX server or dedicated proxy server. The default metricset is `jmx`.

To collect metrics, {beatname_uc} communicates with a Jolokia HTTP/REST
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took some of this language from #4344, but I found some of the language used there to be a bit redundant.

dedemorton
dedemorton commented Aug 16, 2018
View reviewed changes
metricbeat/module/jolokia/jmx/_meta/docs.asciidoc Outdated
@@ -1,58 +1,67 @@
This is the `jmx` metricset of the Jolokia module.
The `jmx` metricset collects metrics from
https://jolokia.org/reference/html/agents.html[Jolokia agents].

[float]
=== Features and configuration
Tested with Jolokia 1.3.4.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should this say 1.5.0 because it seems to contradict the earlier statement about the module.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this should be 1.5.0 too.

@dedemorton dedemorton added needs_backport PR is waiting to be backported to other branches. and removed needs_backport PR is waiting to be backported to other branches. in progress Pull request is currently in progress. labels Aug 16, 2018
@dedemorton dedemorton requested a review from ruflin August 16, 2018 01:13
@ruflin ruflin requested a review from jsoriano August 16, 2018 08:54
@ruflin
Copy link
Member

ruflin commented Aug 16, 2018

Expert around jolokia is @jsoriano so I requested his review on this one.

jsoriano
jsoriano requested changes Aug 16, 2018
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.

Thanks @dedemorton for the changes here, it is looking quite good 🙂, just some comments about the use of namespace.

metricbeat/module/jolokia/jmx/_meta/docs.asciidoc Outdated
@@ -1,58 +1,67 @@
This is the `jmx` metricset of the Jolokia module.
The `jmx` metricset collects metrics from
https://jolokia.org/reference/html/agents.html[Jolokia agents].

[float]
=== Features and configuration
Tested with Jolokia 1.3.4.
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this should be 1.5.0 too.

metricbeat/module/jolokia/jmx/_meta/docs.asciidoc Outdated
---
----
<1> The `namespace` setting is required.
<2> This field will be called `jolokia.jmx.uptime` in the output event because
Copy link
Member

Choose a reason for hiding this comment

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

Actually, after configuring the namespace as metrics, the field will be called jolokia.metrics.uptime. Probably the namespace was added to have events with the monitored service in the name (e.g. if monitoring tomcat with jolokia having the metrics under jolokia.tomcat). Maybe it worths to add a line about the use of namespace.

metricbeat/module/jolokia/jmx/_meta/docs.asciidoc

[float]
=== Limitations
No authentication against Jolokia is supported yet.
Copy link
Member

Choose a reason for hiding this comment

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

The truth is that I haven't tested that, but indeed it should work as it uses the same http helper as other modules.

@jsoriano
Copy link
Member

I agree with backporting it to 6.4 and 6.x branches, I don't think it is needed in 6.3, if it is backported there take into account that proxy is supported since 6.4.

@dedemorton
Copy link
Contributor Author

@jsoriano This is ready for you to review now. Tweaked the wording a bit and changed the namespace to match the one used in the event example at the end of the topic. (You don't see the example in the review here because the example lives in a different file.)

@jsoriano jsoriano merged commit 25940cc into elastic:master Aug 20, 2018
dedemorton added a commit to dedemorton/beats that referenced this pull request Aug 31, 2018
@dedemorton
Improve Jolokia docs and remove proxy restriction (elastic#7969)
c862ff4
@dedemorton dedemorton mentioned this pull request Aug 31, 2018
Merged
dedemorton added a commit to dedemorton/beats that referenced this pull request Aug 31, 2018
@dedemorton
Improve Jolokia docs and remove proxy restriction (elastic#7969)
be710d7
@dedemorton dedemorton mentioned this pull request Aug 31, 2018
Merged
@dedemorton dedemorton removed the needs_backport PR is waiting to be backported to other branches. label Aug 31, 2018
dedemorton added a commit that referenced this pull request Sep 4, 2018
@dedemorton
Backport to 6.4: #7969 and #8172 (#8183)
fe1d6b3 
* Improve Jolokia docs and remove proxy restriction (#7969)

* Update links to point to new GS location (#8172)

* Run make update
dedemorton added a commit that referenced this pull request Sep 4, 2018
@dedemorton
Backport to 6.x: #7969 and #8172 (#8191)
18bb8bd 
* Improve Jolokia docs and remove proxy restriction (#7969)

* Update links to point to new GS location (#8172)

* Run make update
@dedemorton dedemorton mentioned this pull request May 4, 2019
Closed
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
@dedemorton
Backport to 6.4: elastic#7969 and elastic#8172 (elastic#8183)
6d2570e 
* Improve Jolokia docs and remove proxy restriction (elastic#7969)

* Update links to point to new GS location (elastic#8172)

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

@jsoriano jsoriano jsoriano approved these changes

@ruflin ruflin Awaiting requested review from ruflin

Assignees
No one assigned
Labels
docs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dedemorton @ruflin @jsoriano