micrometer-metrics / micrometer

An application observability facade for the most popular observability tools. Think SLF4J, but for observability.
https://micrometer.io
Apache License 2.0
4.45k stars 980 forks source link

Allow injecting a function into TimedAspect to create tags based on method result #3058

Open kevinkraus opened 2 years ago

kevinkraus commented 2 years ago

Please describe the feature request. Update TimedAspect to allow injecting a Function to create tags based on method result, similar to the Function injected to create tags based on ProceedingJoinPoint.

Rationale I am using the Timed annotation and would like to include information from the Object returned by the target method in the tags.

Additional context I'm willing to submit a PR for this enhancement.

axel-malagraba-sw commented 2 years ago

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.

marcingrzejszczak commented 9 months ago

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?

github-actions[bot] commented 8 months ago

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.

github-actions[bot] commented 8 months ago

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.

izeye commented 8 months ago

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.

MarinaMoiseenko commented 1 day ago

Hi @jonatan-ivanov,

Would you be interested in help with this?

shakuzen commented 1 day ago

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

jonatan-ivanov commented 1 day ago

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?

shakuzen commented 1 day ago

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.

MarinaMoiseenko commented 20 hours ago

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":