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

Prevent Graphite reporter from modifying user tags #2083

Merged
merged 2 commits into from
May 19, 2020

Conversation

fitzoh
Copy link
Contributor

@fitzoh fitzoh commented May 10, 2020

Context

As noted in #2069, the recently added Graphite tag support doesn't work for most metric types due to the way that the Dropwizard reporter appends metric attributes (min, max, p95, etc).

A metric that is reported this way using the traditional format:
my.timer.host.foo.max
Is currently reported like this using the tag format:
my.timer;host=foo.max

dropwizard/metrics#1576 was recently merged on the reporter side, adding a new flag to the reporter which would cause it to report the example used above as my.timer;host=foo;metricattribute=max.

What this PR does now

This PR adds a failing test to the end to end GraphiteMeterRegistryTest showing what the modified graphite output will eventually be.

It may also serve as a discussion starter around if the upstream changes to the Dropwizard metrics library are appropriate, or if more changes are required/desirable (is a metricattribute tag? Does it need to be more configurable?).

What this PR should do before merging

Pull in a Dropwizard metrics release containing dropwizard/metrics#1576 and ensure that the Graphite reporter is configured to use the tag format when appropriate.

@fitzoh fitzoh force-pushed the graphite-tag-failing-test branch from 6a8abdf to c91289c Compare May 10, 2020 23:12
@fitzoh
Copy link
Contributor Author

fitzoh commented May 10, 2020

Force pushed a minor modification to the test and updated the production code to work with io.dropwizard.metrics:metrics-graphite:4.1.8-SNAPSHOT (built locally, it doesn't appear that SNAPSHOTS are being published).

@fitzoh fitzoh force-pushed the graphite-tag-failing-test branch from 2ee5f4a to fec1410 Compare May 14, 2020 20:51
@fitzoh fitzoh force-pushed the graphite-tag-failing-test branch from fec1410 to 7fe448a Compare May 14, 2020 20:52
@fitzoh fitzoh marked this pull request as ready for review May 14, 2020 20:58
@fitzoh
Copy link
Contributor Author

fitzoh commented May 14, 2020

Dropwizard Metrics release is out and the build is green after forcing a rebuild.

Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

Thanks for the change in dropwizard and corresponding fix here. We'll want to get this in the 1.5.x line also since Graphite tags support is broken there currently. I can handle this on merge.

@shakuzen
Copy link
Member

On further consideration, due to the hard requirement on a recent patch version, it would likely cause more support issues to merge this into 1.5.x. I will make a release note that lets users know how they can work around the issue by providing their own reporter with this property set rather than deferring to the default one we create.

@shakuzen shakuzen added this to the 1.6.0 milestone May 19, 2020
@shakuzen shakuzen added the registry: graphite A Graphite Registry related issue label May 19, 2020
@shakuzen shakuzen merged commit bbcca7c into micrometer-metrics:master May 19, 2020
@shakuzen shakuzen added the release notes Noteworthy change to call out in the release notes label May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
registry: graphite A Graphite Registry related issue release notes Noteworthy change to call out in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants