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

Reporters: report Timer sums in the Timer duration units #1309

Merged

Conversation

michaelpj
Copy link
Contributor

Reporting sums for Timers is very useful, however they are currently always reported in nanoseconds, instead of in the Timer's duration units. This is because we inherit the handling of getSum from Meter, where it has a slightly different meaning.

There are a couple of unsatisfying bits:

  • JmxTimer loses some precision in reporting the sum, because we want to be able to report fractional values, but JmxMeter assumes that it can be represented as a long. We could probably just change JmxMeter to report a double.
  • GraphiteReporter.reportTimer delegates to a reportMetered, which will do the wrong thing with the sum.

@arteam arteam merged commit e5e8a40 into dropwizard:5.0-development May 2, 2018
@arteam
Copy link
Member

arteam commented May 2, 2018

Thanks! This is a very nice PR and I agree that for Timers we should report the sum in a reporter's duration unit. Regarding your questions:

  • I think it makes sense to change getSum of JmxMeterMBean to double, because it will allow to report sums more precise in the reporter's duration unit. It's a new field, so we don't have any requirements to keep compatibility.

  • In GraphiteReporter we could add a new parameter to the reportMetered method and check whether we shoud convert it:

 private void reportMetered(MetricName name, Metered meter, long timestamp, boolean isSumADuration) throws IOException {
        sendIfEnabled(COUNT, name, meter.getCount(), timestamp);
        sendIfEnabled(SUM, name, isSumADuration ? convertDuration(meter.getSum()) : meter.getSum(), timestamp);

I would gladly review both changes as separate PRs.

@michaelpj
Copy link
Contributor Author

Thanks for the quick review! I've put up the suggested follow-up PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants