Metricbeat can call Jolokia with GET requests. (#8566) #9226

manios · 2018-11-25T15:03:47Z

This pull request provides Metricbeat the option to call Jolokia using HTTP GET requests.

It adds a new http_method parameter to config.yml which by default will have the value POST and can accept GET as well.
Proxy requests are not supported.

For more information, please refer to #8566.

Thanks in advance for your time.

elasticmachine · 2018-11-25T15:03:49Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

jsoriano

Thanks @manios for the great effort here! 🎉 This is looking quite good, I have added some suggestions, let me know what you think.

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

metricbeat/module/jolokia/jmx/config.go

metricbeat/module/jolokia/jmx/data.go

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

metricbeat/module/jolokia/jmx/config.go

jsoriano

Thanks for addressing these comments! I have added some more comments. The most important ones are that I think that the Builders shouldn't be called Builders anymore 🙂 and that probably the event mapping functions could be methods of the "builders" directly.

jsoriano · 2018-11-29T16:28:14Z

metricbeat/module/jolokia/jmx/config.go

+
+// This replacer is responsible for adding a "!" before special characters in GET request URIs
+// For more information refer: https://jolokia.org/reference/html/protocol.html
+var mbeanGetEssapeReplacer = strings.NewReplacer("\"", "!\"", ".", "!.", "!", "!!", "/", "!/")


Essape wanted to be Escape?

Hi @jsoriano ! Thanks for noticing! Fixed! 👍

jsoriano · 2018-11-29T17:29:31Z

metricbeat/module/jolokia/jmx/jmx.go

-	}
-	http.SetMethod("POST")
-	http.SetBody(body)
+	jolokiaHTTPBuild := NewJolokiaHTTPRequestBuiler(config.HTTPMethod)


This is not a builder anymore, it is a Fetcher now 🙂

Hi @jsoriano ! Thanks for noticing! Renamed to interface to JolokiaHTTPRequestFetcher and structs to JolokiaHTTPGetFetcher and JolokiaHTTPPostFetcher. 👍

jsoriano · 2018-11-29T17:36:20Z

metricbeat/module/jolokia/jmx/config.go

+	eventMapper := NewEventMapper(httpReqs[0].HTTPMethod)
+
+	// Map response to Metricbeat events
+	events, err := eventMapper.EventMapping(resBody, mapping)


Is there any reason to don't make these EventMapping methods of JolokiaHTTPPostBuilder and JolokiaHTTPGetBuilder directly?

Hi @jsoriano ! Indeed, there is no reason. I moved EventMapping methods to JolokiaHTTPGetFetcher and JolokiaHTTPPostFetcher. 👍

jsoriano · 2018-11-30T14:21:56Z

jenkins, test this please

jsoriano · 2018-11-30T14:22:29Z

It LGTM, thanks @manios for your work on this! Let's wait for jenkins before merging.

metricbeat/module/jolokia/jmx/config.go

jsoriano · 2018-11-30T14:32:23Z

jenkins, test this again please

jsoriano · 2018-11-30T15:00:17Z

@manios I think that the failing check in jenkins gets fixed by updating the branch with master, could you try?

manios · 2018-11-30T16:18:05Z

Hi @jsoriano ! I rebased with the latest changes from master. Is it ok now? Thanks for your assistance ! 👍

jsoriano · 2018-11-30T16:19:00Z

jenkins, test this

manios · 2018-11-30T19:12:28Z

Jenkins tests have passed 🎉 !

jsoriano · 2018-12-02T09:58:08Z

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

+       - attr: Uptime
+         field: uptime
+----
+will create 3 GET requests, one for every MBean. On the contrary, if you use HTTP POST, Metricbeat will create only 1 request to Jolokia.


@manios sorry, one last minor detail here, could you move the whole description about the example before the code block? So it is something like: For example the configuration below with 3 mappings creates 3 GET requests, one for every MBean. On the contrary, if you use HTTP POST, Metricbeat creates only 1 request to Jolokia.

Hi @jsoriano ! Done! 👍

jsoriano · 2018-12-02T18:26:47Z

jenkins, test this one last time

…c#9226) Add a new `http_method` parameter to Jolokia module which by default will have the value POST and can accept GET as well. Fixes elastic#8566. (cherry picked from commit 463ce3e)

jsoriano · 2018-12-04T08:57:26Z

Hi @manios, I have merged this, it was missing a changelog entry but I am adding one on #9374. This feature will probably get released on Metricbeat 6.6.0. Thanks! 🎉

manios · 2018-12-04T11:00:51Z

Hi @jsoriano ! Thank you for your time and your fruitful review! I hope I can contribute more in this project! 🎉

…yslog_host * elastic/master: Metricbeat can call Jolokia with GET requests. (elastic#8566) (elastic#9226) Add some missing references to PRs in changelog (elastic#9358)

…ts. (#8566) (#9375) Add a new `http_method` parameter to Jolokia module which by default will have the value POST and can accept GET as well. Fixes #8566. (cherry picked from commit 463ce3e) Add changelog for #8566 (cherry picked from commit d434b47) Co-authored-by: Christos Manios <maniopaido@gmail.com>

jsoriano added module review Metricbeat Metricbeat labels Nov 26, 2018

jsoriano requested changes Nov 26, 2018

View reviewed changes

ruflin added the Team:Integrations Label for the Integrations team label Nov 27, 2018

jsoriano reviewed Nov 28, 2018

View reviewed changes

metricbeat/module/jolokia/jmx/config.go Outdated Show resolved Hide resolved

jsoriano reviewed Nov 29, 2018

View reviewed changes

jsoriano approved these changes Nov 30, 2018

View reviewed changes

jsoriano reviewed Nov 30, 2018

View reviewed changes

metricbeat/module/jolokia/jmx/config.go Show resolved Hide resolved

manios added 6 commits November 30, 2018 16:16

Metricbeat can call Jolokia with GET requests. (#8566)

ff39024

#8566 Documentation and config fixes for Jolokia.

5d921b9

#8566 Fix documentation list number.

a9f5099

Make fetching from Jolokia more generic (#8566)

9cc07e1

Move mappers to fetcher classes and renaming refactoring (#8566)

1cd6437

Fix formatting issue which broke Travis build. (#8566)

9ecf7b1

jsoriano reviewed Dec 2, 2018

View reviewed changes

Move example description above the code block. (#8566)

38807b6

alvarolobato assigned manios Dec 3, 2018

jsoriano merged commit 463ce3e into elastic:master Dec 4, 2018

jsoriano mentioned this pull request Dec 4, 2018

Add missing changelog for #9226 #9374

Merged

jsoriano mentioned this pull request Dec 4, 2018

Cherry-pick #9226 to 6.x: Metricbeat can call Jolokia with GET requests. (#8566) #9375

Merged

jsoriano added the v6.6.0 label Dec 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metricbeat can call Jolokia with GET requests. (#8566) #9226

Metricbeat can call Jolokia with GET requests. (#8566) #9226

manios commented Nov 25, 2018

elasticmachine commented Nov 25, 2018

jsoriano left a comment

jsoriano left a comment •

edited

Loading

jsoriano Nov 29, 2018

manios Nov 29, 2018

jsoriano Nov 29, 2018

manios Nov 29, 2018

jsoriano Nov 29, 2018

manios Nov 29, 2018

jsoriano commented Nov 30, 2018

jsoriano commented Nov 30, 2018

jsoriano commented Nov 30, 2018

jsoriano commented Nov 30, 2018

manios commented Nov 30, 2018

jsoriano commented Nov 30, 2018

manios commented Nov 30, 2018

jsoriano Dec 2, 2018

manios Dec 2, 2018

jsoriano Dec 2, 2018

jsoriano commented Dec 2, 2018

jsoriano commented Dec 4, 2018

manios commented Dec 4, 2018

Metricbeat can call Jolokia with GET requests. (#8566) #9226

Metricbeat can call Jolokia with GET requests. (#8566) #9226

Conversation

manios commented Nov 25, 2018

elasticmachine commented Nov 25, 2018

jsoriano left a comment

Choose a reason for hiding this comment

jsoriano left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano commented Nov 30, 2018

jsoriano commented Nov 30, 2018

jsoriano commented Nov 30, 2018

jsoriano commented Nov 30, 2018

manios commented Nov 30, 2018

jsoriano commented Nov 30, 2018

manios commented Nov 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano commented Dec 2, 2018

jsoriano commented Dec 4, 2018

manios commented Dec 4, 2018

jsoriano left a comment •

edited

Loading