Micrometer: Broken graphite metrics names with tags

[davide-baldo]

Graphite tags always generate broken names due to DropWizard behavior.

From DropWizard, GraphiteReporter [1]:
graphite.send(prefix(name, type.getCode()), format(value), timestamp);
where
name="my.metric;tag=value"
type.getCode() = "p99"

final string sent to graphite: "my.metric;tag=valuep99"

From graphite logs
08/05/2020 08:21:00 :: [tagdb] Tagging component.api.setStatus;host=mail.example.com.mean, component.api.sendTextMessage;host=mail.example.com.p95, component.api.join_meeting;host=mail.example.com.m1_rate, component.api.getConversations;host=mail.example.com.mean

Unless we patch DropWizard side i can't see how this code may work

[1] https://github.com/dropwizard/metrics/blob/v4.1.7/metrics-graphite/src/main/java/com/codahale/metrics/graphite/GraphiteReporter.java#L351

[davide-baldo]

@fitzoh @shakuzen
What do you think it's the best approach?

[fitzoh]

Yeah, saw your comment yesterday, should be able to dig in this weekend.
Hoping I can find an easy fix, if not the easy first step is probably updating defaults

[fitzoh]

I opened a PR (https://github.com/dropwizard/metrics/pull/1576) to dropwizard metrics that should allow us to work around this.
No idea what kind of time frame we will be looking at for that to be merged or for a new release to be cut.

I agree with @workless that there isn't a clean way to fix this without modifying the dropwizard code.

The options I can think of:

• Wait for the dropwizard metrics maintainers to merge/cut a new release, then modify on the micrometer side
• fork/publish a modified version of the graphite reporter and work off that
• vendor the modified version of the graphite reporter in this repo and use that until an official release is out
• revert the default and/or add a note and/or remove the feature from micrometer until the dropwizard metrics release is available

Either way, the Spring Boot release team should probably be made aware, as I believe this feature is present in v2.3.0.RC1 (cc @snicoll)

[jkschneider]

@fitzoh It may be the reason we need to cut ties with Dropwizard as a passthrough for Graphite. We had a similar situation with the Ganglia registry a while back.

[fitzoh]

For what it's worth, the upstream PR was merged already. I opened a draft PR (#2083) with a failing test to cover this scenario. I'm planning on adding a fix to that branch which pulls in a SNAPSHOT version of dropwizard metrics, then dropping the SNAPSHOT once a new dropwizard version is out.

[fitzoh]

PR has been updated to work with the Dropwizard SNAPSHOT, should be good to go once a release is cut.

[davide-baldo]

Wow, that was fast, kudo really, first time seeing a fix this fast.

In the medium-term if it's ok i would like to open a PR(or new repo?) to integrate graphite directly without dropwizard for different reasons:

• the output formats are as simple as you can get, and can be handled in few hundred lines
• by default many unspecified metrics are sent, if you specify a percentile it gets _added_ to the list, I had to rewrite GraphiteReporter to disable unwanted metrics
• maintaining dropwizard wrapper seems to be way more complex than supporting graphite
• metrics seems to have different behavior than expected (counters are absolute, and maybe more)
• in general, a it's better to stay in-line with micromter guidelines as much as possible

[shakuzen]

@workless Thanks for the offer to allow supporting Graphite in Micrometer without Dropwizard passthrough. I'm in favor of the change going forward, but it might be a bit tricky compatibility-wise, since Dropwizard is part of the public API for our current Graphite registry. We would have to figure out how to deal with this so we don't break users while allowing an upgrade path.

[davide-baldo]

@shakuzen What do you think about adding a new micrometer package while maintaining the old one, adding 'back-compatibility' options per users request over time until removing the old package "feel safe".

[jkschneider]

@workless Not a fan of adding another module, since most users would never adopt. Ultimately, the compatibility problem comes down to a single constructor;

GraphiteMeterRegistry(
   GraphiteConfig config, 
   Clock clock, 
   HierarchicalNameMapper nameMapper, 
   MetricRegistry metricRegistry,  // this and...
   GraphiteReporter reporter.        // this are the problem
)

Ultimately, we can deprecate this and then discuss whether Micrometer should provide an optional dependency on Dropwizard or not. In the case this constructor is used, I think we'd have to basically delegate to some legacy implementation that still extends from DropwizardMeterRegistry. But I'd like for the user to not have to know about the distinct types.

[fitzoh]

New Dropwizard Metrics release is out and the PR should be good to go.

[shakuzen]

With the method added to the metrics-graphite library in 4.1.8, a workaround is now available for uses wishing to use the Graphite tag support. Namely, they should upgrade metrics-graphite to 4.1.8 or later and provide a GraphiteReporter with the new option set, such as will be done in the default GraphiteReporter in Micrometer 1.6.0:

GraphiteReporter.forRegistry(metricRegistry)
    .withClock(new DropwizardClock(clock))
    .convertRatesTo(config.rateUnits())
    .convertDurationsTo(config.durationUnits())
    .addMetricAttributesAsTags(config.graphiteTagsEnabled()) // this fixes #2069
    .build(getGraphiteSender(config));

I will note the above in the release notes for 1.5.2. As for migrating the implementation to not depend on Dropwizard, please feel free to open a separate issue to track that @workless.