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

Build more tags using method context #753

Conversation

hrothwell
Copy link
Contributor

Aiming to solve #752 and potentially also make #612 possible

Any beans implementing TagsBasedOnMethodInvocationContext are injected and additional tags are computed using buildTags(MethodInvocationContext context)

@CLAassistant
Copy link

CLAassistant commented May 4, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change. You would need to add a new constructor (the new one should be annotated with @Inject while keeping and deprecating the old one (protected TimedInterceptor(MeterRegistry meterRegistry, ConversionService conversionService)) removing the @Inject annotation from it

Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add also documentation for this feature.

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

Just now realizing this is probably also helpful / can support @Counted annotations as well, so will move a few more things around to support that

@hrothwell hrothwell requested review from graemerocher and sdelamo May 6, 2024 22:48
@hrothwell
Copy link
Contributor Author

hrothwell commented May 6, 2024

bringing attention to @graemerocher 's comment here, this seems like something that could be done here as well? Perhaps not all @Timed or @Counted annotations really need/want all tags provided by beans in the context. My initial idea was your implementation of the tagger would be able to decide if it wants to tag it or not, but explicitly saying "add additional tags using this impl" in the annotation metadata seems more convenient to me. Thoughts?

The only thing I am unsure of is if you wanted your tagger to have some other bean injected how that would work. I feel there is a way to do it, I am just not familiar enough with that

@graemerocher
Copy link
Contributor

you can inject it into the constructor of the tagger, for example if you need the MeterRegistry you just add a constructor argument for it

@hrothwell
Copy link
Contributor Author

you can inject it into the constructor of the tagger, for example if you need the MeterRegistry you just add a constructor argument for it

Even if doing something like @Timed(strategies = [MyTagger class]) as you alluded to in that other PR comment? I like that idea of being able to explicitly call out what taggers to use in the annotations, I'm just not sure how to put that together 🤔

@graemerocher
Copy link
Contributor

would be a matter of adding a filter to the stream based on the types that are resolved from the annotation

@hrothwell
Copy link
Contributor Author

would be a matter of adding a filter to the stream based on the types that are resolved from the annotation

that makes sense, would that then mean we need to create our own annotations to replace @Timed and @Counted though? Or an annotation that works side by side with them? Definitely something I am interested in doing just not sure on the how, probably also isn't needed as part of this PR and could come later

@graemerocher
Copy link
Contributor

yeah I guess we would have our own version kind of like how we have both Jakarta @Transactional and our own one

@hrothwell
Copy link
Contributor Author

yeah I guess we would have our own version kind of like how we have both Jakarta @Transactional and our own one

another thought that came to mind: could you create a new annotation that is just used alongside the existing ones? Wouldn't introduce a breaking change, but maybe something like:

@Timed(value = "requests")
@AddTags(taggers = [MyTagger.class])
fun timedRequest() = println("time me")

Doesn't require updating the @Timed metric for users, can be retrieved from the AnnotationMetadata that is already pulled, etc

@sdelamo sdelamo removed their request for review May 8, 2024 13:41
@hrothwell hrothwell requested a review from sdelamo May 8, 2024 14:28
@sdelamo sdelamo requested a review from graemerocher May 9, 2024 10:18
@graemerocher
Copy link
Contributor

I would probably call it @MetricOptions or something in case it could be used for something else

@hrothwell
Copy link
Contributor Author

I would probably call it @MetricOptions or something in case it could be used for something else

is this something that would be wanted for/in this PR or just something that could come later down the line?

@graemerocher
Copy link
Contributor

No can be done in a subsequent PR

@hrothwell
Copy link
Contributor Author

hrothwell commented May 10, 2024

Am I expected to merge this PR? I don't seem to have the right authorization to do so with approvals present, and not sure if @sdelamo change requests need to be cleared prior

@sdelamo sdelamo changed the base branch from 5.5.x to 5.6.x May 13, 2024 09:50
@sdelamo sdelamo merged commit d43366c into micronaut-projects:5.6.x May 13, 2024
6 checks passed
graemerocher pushed a commit that referenced this pull request May 28, 2024
…pply to metrics (#764)

Follow up to #753

New annotation MetricOptions to hold metadata for additional information that might be used in building metrics
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.

Add support for Micrometers Metertag Allow additional tags to be programmatically added to @Timed metric
4 participants