Micrometer: Improve Tag propagation for @Timed

[jonatan-ivanov]

Currently, there isn't an easy way to add tags to a Timer if it was created through the TimedAspect using the @Timed annotation.

I’m thinking about implementing two things:

1. A class (ExtraTagsPropagation) with a few static methods (like SLF4J's MDC or Spring Cloud Sleuth's ExtraFieldPropagation) which add tags to a ThreadLocal and TimedAspect could get extra tags from it, e.g.:

@Timed
void testMethod(String param) {
    ExtraTagsPropagation.add("testTag", someValue);
    ...
}

1. An annotation (ExtraTag) what you can put on the method params and TimedAspect could get the extra tags this way:

@Timed
void testMethod(@MetricTag("testTag") String param) {...}

I created a PoC for this, you can see them in use in EchoService.
The TimedAspect needs to be modified, for the sake of simplicity, the PoC does not do it, instead, it extends its Function: MetricsConfig.

If this is something you would accept, I would be happy to go ahead, polish this a little, modify the TimedAspect, add some docs and tests and open a PR.
What do you think?

[shakuzen]

This is something that's been asked about before. We don't do general context propagation right now. While ThreadLocal should work fine in thread-based runtimes, with other runtimes (e.g. Netty, Reactor) I think there's probably more required.
I'd have to look more into this later.

[jonatan-ivanov]

@shakuzen I might be missing something but since TimedAspect is calling the actual method synchronously (through the joinpoint) if TimedAspect works, ThreadLocal must too since you can't change threads there.
Assuming that the key-value pair is added in that method without tricking with it (adding from an async handler, starting a new thread and adding from it, etc).

[jonatan-ivanov]

@shakuzen Did you have a chance to take a look?
@jkschneider @izeye What do you think?

[jkschneider]

@jonatan-ivanov How do you compare this design to the one Adrian suggested here? I'll admit, this isn't my strength necessarily.

[jonatan-ivanov]

@jkschneider What he suggested is an addition to # 2 in my opening comment. I really like it, it is a more flexible and smarter way of doing it by making it possible to inject your own Object -> String converter.

Let me compare them:

• In the PoC above ExtraTag has the same goal as Param (in Adrian's example) but it is simpler and dummier than that
• The PoC above always converts the value using String.valueOf while Adrian's example makes this configurable by making it possible to inject your own logic by providing an Expander

I think # 1 above (ExtraTagsPropagation) is still useful:

1. If the tag you want to add is not coming from the parameter of the method (but it is a static or member field or something else)
2. If you just want a one-liner and don't want to create a new class implementing the Expander

[jkschneider]

you just want a one-liner and don't want to create a new class implementing the Expander

Can we design this in such a way that we simply provide a default value for Expander that does something useful if not complicated (i.e. "dumb"), and unify this extra properties mechanism under a value + expander model?

[robhillier]

The Feign example does exactly that - by default, the expander is a ToStringExpander, basically, if you don't define anything do a toString on the parameter value

[jonatan-ivanov]

@jkschneider I made a sketch for the annotation-based parameter propagation. Can I get your feedback on how do you like the structure of it and the naming?
https://github.com/micrometer-metrics/micrometer/pull/2121

If it looks good, I'll make it a little more pretty (license, docs), add memorization (see the todo), and tests.

[jonatan-ivanov]

@shakuzen What do you think?

[erajasekar]

@jonatan-ivanov Thanks for the POC implementation.

I have a very similar use case and supporting the proposed solution by @jonatan-ivanov would be great. Any idea if his PR will be merged?