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

change jolokia input to use bulk requests #2253

Merged
merged 1 commit into from
Apr 18, 2017
Merged

change jolokia input to use bulk requests #2253

merged 1 commit into from
Apr 18, 2017

Conversation

phemmer
Copy link
Contributor

@phemmer phemmer commented Jan 12, 2017

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)

This adjusts the jolokia input to use bulk requests. This significantly increases the performance of it.
In my specific use case, I have a deployment where it takes ~10.4 seconds to gather all the requested jolokia metrics. With this change it now takes ~0.04 seconds (260x faster).

There is one behavioral change as a result of this PR. If the configuration has a syntatically invalid mbean name, instead of just failing to gather that one metric, all metrics will fail. This is because if jolokia can't parse the request, it fails the entire request. Missing mbeans/attributes still behave the way they previously have, in that an error for that specific metric will be logged, but all others will be gathered.
You can find details on this here: jolokia/jolokia#124

Closes #1328

@sparrc sparrc added this to the 1.3.0 milestone Jan 12, 2017
@dylanmei
Copy link
Contributor

This is huge, thanks!

@sparrc
Copy link
Contributor

sparrc commented Jan 12, 2017

looks good, please rebase and remove the changelog update for now, as this will be going into 1.3

@phemmer
Copy link
Contributor Author

phemmer commented Jan 12, 2017

looks good, please rebase and remove the changelog update for now, as this will be going into 1.3

Done

@lhoss
Copy link

lhoss commented Jan 17, 2017

looking fwd then to v1.3 release (any ETA?) !

ps:

If the configuration has a syntatically invalid mbean name, instead of just failing to gather that one metric, all metrics will fail.

I guess this is no real issue, as any such syntactic errors would be detected on the first request, and assuming the reported error/exception message contains enough information about what mbean/string contains the error.
Actually its sort of 'fail fast' (avoids getting surprises later, in case one did not test all the mbeans early).

@danielnelson
Copy link
Contributor

@phemmer Just to make sure, this is still a useful update even though we are planning to merge the new jolokia2 plugin this cycle? If so, can you add back to changelog.

@phemmer
Copy link
Contributor Author

phemmer commented Apr 18, 2017

Does "jolokia2" do bulk requests?
As long as this plugin isn't going away, I'd say the change is still useful yes. It'll be a performance boost for the users still using it.

@danielnelson
Copy link
Contributor

Does "jolokia2" do bulk requests?

According to the PR it does, though I haven't looked at the code.

Let's add it to the changelog then.

@dylanmei
Copy link
Contributor

Yes, jolokia2 does bulk requests also. But the plugin itself is a breaking change, I don't imagine it would get into a 1.3 version of Telegraf.

@danielnelson
Copy link
Contributor

It's not breaking because it is a new plugin, right?

@phemmer
Copy link
Contributor Author

phemmer commented Apr 18, 2017

Changelog updated.

@danielnelson danielnelson merged commit 2542ef6 into influxdata:master Apr 18, 2017
vlamug pushed a commit to vlamug/telegraf that referenced this pull request May 30, 2017
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.

5 participants