micronaut-micrometer icon indicating copy to clipboard operation
micronaut-micrometer copied to clipboard

Build more tags using method context

Open hrothwell opened this issue 9 months ago • 9 comments

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)

hrothwell avatar May 04 '24 00:05 hrothwell

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 04 '24 00:05 CLAassistant

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 avatar May 06 '24 17:05 hrothwell

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

hrothwell avatar May 06 '24 22:05 hrothwell

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

graemerocher avatar May 07 '24 11:05 graemerocher

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 🤔

hrothwell avatar May 07 '24 15:05 hrothwell

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

graemerocher avatar May 07 '24 15:05 graemerocher

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

hrothwell avatar May 07 '24 17:05 hrothwell

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

graemerocher avatar May 07 '24 19:05 graemerocher

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

hrothwell avatar May 07 '24 22:05 hrothwell

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

graemerocher avatar May 09 '24 12:05 graemerocher

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?

hrothwell avatar May 09 '24 16:05 hrothwell

No can be done in a subsequent PR

graemerocher avatar May 09 '24 20:05 graemerocher

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

hrothwell avatar May 10 '24 14:05 hrothwell