Hi, any news on this? I think that would be really interesting, not just for TimedAspect but for CountedAspect as well. I'm also willing to participate in the implementation.

We already have it since 1.7.0? https://github.com/micrometer-metrics/micrometer/blob/v1.12.1/micrometer-core/src/main/java/io/micrometer/core/aop/TimedAspect.java#L156 Or am I missing sth?

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open.

This seems to suggest adding a function that provides tags from the return value of ProceedingJoinPoint.proceed() while Micrometer currently supports a function that provides tags from ProceedingJoinPoint.

Hi @jonatan-ivanov,

Would you be interested in help with this?

I think we would consider a pull request if you're willing to work on this @MarinaMoiseenko.

Yes please, if you want to file a PR for this, we are happy to review it.
@shakuzen Maybe such a PR would be a good opportunity to have a TagsProvider<T> interface in core?

Maybe such a PR would be a good opportunity to have a TagsProvider<T> interface in core?

I'm not sure. I'd be happy to look at a proposal, but I think it may distract from getting this specific feature merged to do it together. It's not immediately obvious to me at least what the right design for such an interface would be and how it would be incorporated (or wouldn't be incorporated) into existing instrumentation or other parts of the code. I get it feels weird to have unrelated tags provider types for each instrumentation, but I also wonder what would be gained by having a common type. But if someone has a good idea for it, let's look at it.

If you don't mind, couple of questions:

Q1: Wouldn't it be too "global" having ability to set result tags extractor in TimedAspect?

What I mean is that client's functions vary one from another. Therefore having just one common function across all methods to produce tags based on method result could be cumbersome. So, it looks like having more granular approach would be better. Something similar to @MeterTag, but on method level.

For example, to change "@TimedAspect" code from this:

        if (meterTagAnnotationHandler != null) {
            meterTagAnnotationHandler.addAnnotatedParameters(builder, pjp);
        }

to this (roughly):

        if (meterTagAnnotationHandler != null) {
            meterTagAnnotationHandler.addAnnotatedParameters(builder, pjp);
            meterTagAnnotationHandler.addAnnotatedResult(builder, pjp, methodResult); 
        }

Where meterTagAnnotationHandler.addAnnotatedResult is new method. Alternatively a new similar annotation could be created to have "@MeterMethodTag" (for example) and MeterMethodTagAnnotationHandler. But main idea stays the same.
methodResult - will be passed to private Timer.Builder recordBuilder(...) as an argument.

Then usage of it would look like:

        @Timed
        @MeterTag(key = "test", resolver = ValueResolver.class)
        String getAnnotationForTagValueResolver();

        @Timed
        @MeterTags({
            @MeterTag(key = "value1", expression = "'value1: ' + value1"),
            @MeterTag(key = "value2", expression = "'value2: ' + value2"), @MeterTag(key = "value3",
            expression = "'value3: ' + value1.toUpperCase + value2.toUpperCase") })
        public DataHolder getMultipleAnnotationsWithContainerForTagValueExpression() {
            return new DataHolder("zxe", "qwe");
        }

Q2: Should @ Counted have the same feature?

Q3: Would you consider using Builder pattern for TimedAspect if there is still a need to provide TimedAspect with additional settings.

In the main branch there are different methods for setting up different "tags providers":

• based on ProceedingJointPoint through constructor
• based on annotations through setter
  which looks inconsistent.

Q1: Wouldn't it be too "global" having ability to set result tags extractor in TimedAspect?

Probably. This is something we should get feedback from the users who want this feature. I think extending @MeterTag to work with result objects makes sense to me.

Q2: Should @ Counted have the same feature?

Sure. @MeterTag works with @Counted as well, so it would make sense for this new feature to work with it as well.

Q3: Would you consider using Builder pattern for TimedAspect if there is still a need to provide TimedAspect with additional settings.

I don't remember the details, but the history is in Git. The default constructor was added because it made it easier to use LTW, according to #1419. The setter was added with #3727:

Adds a setter to TimedAspect (too many constructors already)

I'm not opposed to a builder pattern but we should consider how it will be used (can it be used?) in the different ways the aspect may end up being configured.