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

Added class and method tags to Timed/Counted interceptors. #749

Closed

Conversation

donbeave
Copy link
Contributor

Hi!

I'm using a micrometer heavily, and currently, whenever I need to measure some method usage (timer/counter) I always need to do something like this:

public class UserRepository {

    @Timed(value = "repository.timed", extraTags = {"class", "UserRepository", "method", "findById"})
    public Optional<User> findById(String id) {
        // ...
    }

    // ...
}

But it's very verbose and requires manually adding class names and method name tags for each method. I think it will be better if TimedInterceptor/CountedInterceptor will be able to add this information when it requires. My current implementation is adding these tags for all timers/counters, which I'm not sure is a good solution for everybody, maybe it's better to add an ability to control this behavior inside the annotation, something like this:

public @interface Timed {

    // ...

    /**
     * List of additional tags that will be added to extra tags during intercepting the annotated method.
     */
    ExtraTag[] includeExtraTags() default {};

    // ...

}
public enum ExtraTag {

    SIMPLE_CLASS_NAME,
    CLASS_NAME,
    METHOD_NAME

}

And usage will be:

    @Timed(value = "repository.timed", includeExtraTags = {ExtraTag.CLASS_NAME, ExtraTag.METHOD_NAME})
    public Optional<User> findById(String id) {
        // ...
    }

I like this approach more, but it requires to copy Timed/Counted annotations from the micrometer repository and keeping them here in the micronaut repository as alternatives with this extension.

@CLAassistant
Copy link

CLAassistant commented Apr 30, 2024

CLA assistant check
All committers have signed the CLA.

@graemerocher
Copy link
Contributor

Needs more tests

@donbeave
Copy link
Contributor Author

Needs more tests

Haven't added tests since I'm not sure which approach is better. Do I need to leave it as is, just add class/method tags for all, or do I need to introduce alternative Micronaut's annotations with extended property includeExtraTags?

@graemerocher
Copy link
Contributor

I think what would make more sense if if we add some strategy interface. Something like:

@Timed(tagStrategy=MyStrategy.class)

Then

class MyStrategy implement TaggingStrategy {
       void customise(ExecutableMethod<?, ?> method, Tagger tagger) {
             tagger.tag("methodName", method.getMethodName());
        }
}

One could then add whatever customisations needed in a meta annotation

@sdelamo sdelamo added the type: enhancement New feature or request label May 3, 2024
@sdelamo
Copy link
Contributor

sdelamo commented May 3, 2024

@donbeave another option would be for you to create your own annotation and then map it to @Timed with the extra tags.

see: https://docs.micronaut.io/4.4.6/api/io/micronaut/inject/annotation/AnnotationMapper.html https://docs.micronaut.io/4.4.6/guide/#_aliasing_mapping_annotations

@hrothwell
Copy link
Contributor

I'm a little late here but I have started something similar that would likely also accomplish this but also not change the underlying tags for everyone who uses this library: #753

@graemerocher
Copy link
Contributor

Resolved by #764

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants