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

Feature/jolokia proxy mode #1031

Closed
wants to merge 5 commits into from

Conversation

saiello
Copy link
Contributor

@saiello saiello commented Apr 14, 2016

Added jolokia proxy mode.

saiello added 2 commits April 15, 2016 00:50
code refactor to use same prepareRequest method
for both 'agent' and 'proxy' mode
@saiello saiello force-pushed the feature/jolokia-proxy-mode branch from ebc2722 to a0097ee Compare April 14, 2016 23:02
@saiello saiello mentioned this pull request Apr 14, 2016

## List of servers exposing jolokia read service
# This specifies the mode used
# mode = "proxy"
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the options besides "proxy"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently any value different to "proxy", make the input plugin executing in agent mode.

@sparrc
Copy link
Contributor

sparrc commented Apr 15, 2016

This looks like a good change to the jolokia plugin, but it is a breaking change as well.

Will the POST requests to jolokia work correctly for all users out of the box?

@saiello saiello force-pushed the feature/jolokia-proxy-mode branch from a0097ee to 161b946 Compare April 15, 2016 22:26
@saiello
Copy link
Contributor Author

saiello commented Apr 15, 2016

POST requests are more powerful and have less escaping issue than GET ones.
https://jolokia.org/reference/html/protocol.html#request-response

Yes it is a breaking change in configuration, but it is trivial to change the old 'jmx' with new Mbean/Attribute/Path parts.

Would you make it working even after update keeping the old configuration?

@sparrc
Copy link
Contributor

sparrc commented Apr 15, 2016

No, I think it's OK if we make it a breaking change. I just wanted to confirm

@@ -132,37 +226,37 @@ func (j *Jolokia) Gather(acc telegraf.Accumulator) error {
tags["port"] = server.Port
Copy link
Contributor

Choose a reason for hiding this comment

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

@saiello I should have caught this in the original plugin, but here you are overwriting the telegraf-wide host tag, could you change these tags:

  • server
  • port
  • host

to:

  • server_name
  • server_port
  • server_host

@saiello saiello force-pushed the feature/jolokia-proxy-mode branch 2 times, most recently from 6d6f36a to 6de99c5 Compare April 18, 2016 21:50
@sparrc
Copy link
Contributor

sparrc commented Apr 18, 2016

@saiello you need to run go fmt ./...

@saiello saiello force-pushed the feature/jolokia-proxy-mode branch from 6de99c5 to 12b056c Compare April 18, 2016 21:58
@sparrc
Copy link
Contributor

sparrc commented Apr 18, 2016

thanks @saiello, please rebase your changes and update CHANGELOG.md with this change, as well as writing up something about it being a breaking change under "Release Notes", and then I can merge.

Please also ping me when you're done with this, as I don't get notifications on file changes

@sparrc
Copy link
Contributor

sparrc commented Apr 18, 2016

this change will now also close #1050

@saiello saiello force-pushed the feature/jolokia-proxy-mode branch from 12b056c to 5dbd788 Compare April 21, 2016 07:50
@saiello
Copy link
Contributor Author

saiello commented Apr 21, 2016

@sparrc I edited the release note. Would you require some kind of "migration guide" in jolokia README?

@darsh221
Copy link

@sparrc When this will be merged to master?

@sparrc
Copy link
Contributor

sparrc commented Apr 27, 2016

by the end of the week

@darsh221
Copy link

Cool. Thank you for the update.

@sparrc sparrc closed this in 18636ea Apr 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants