MeterTag #3727

marcingrzejszczak · 2023-03-30T15:13:41Z

Moves the common code from Micrometer Tracing
Adds a setter to TimedAspect (too many constructors already)
This only works for the Timer.Builder not for the LongTaskTimer.Builder (code can be modified to allow that)

micrometer-commons/build.gradle

shakuzen

Mostly minor feedback left in comments. Looks great overall. This should be nice and offers good usability while being flexible.

The main feedback I have is a bikeshed argument about naming. In Micrometer core, I would go with @MeterTag instead of @MetricTag. We use Meter elsewhere there consistently, as far as I know. On the classes in commons, it feels a little odd they have "Tag" in the name even though commons has no tag references and instead has KeyValue. I wonder if we need the "Tag" part in the name in commons. It might be too generic, but what about ValueResolver and ValueExpressionResolver? They are resolving the Value part of KeyValue and we do say tag value also. Maybe it isn't a big deal either way, but it stuck out to me in reviewing.

micrometer-core/src/main/java/io/micrometer/core/aop/MetricTag.java

micrometer-commons/src/main/java/io/micrometer/common/annotation/TagAnnotationHandler.java

shakuzen · 2023-04-04T07:11:55Z

micrometer-core/src/main/java/io/micrometer/core/aop/MetricTag.java

+    String expression() default "";
+
+    /**
+     * Use this bean to resolve the tag value. Has the highest precedence.


"bean" feels a little odd here. The class type given here will be used to resolve the TagValueResolver instance from the function given to the MetricsTagAnnotationHandler.

micrometer-core/src/main/java/io/micrometer/core/aop/MetricTag.java

micrometer-core/src/test/java/io/micrometer/core/aop/TimedAspectTest.java

shakuzen · 2023-04-04T07:31:49Z

micrometer-core/src/main/java/io/micrometer/core/aop/MetricTag.java

+ * <p>
+ * try with the {@link TagValueResolver} bean if the value of the bean wasn't set, try to
+ * evaluate a SPEL expression if there’s no SPEL expression just return a
+ * {@code toString()} value of the parameter


JavaDoc needs to be updated for metrics (it mentions span and SPEL and beans). It would also be good to make a warning that this should not be used with parameters that are going to result in high cardinality values (e.g. user-provided input) and there is no protection provided out-of-the-box for if it is used in such a way.

sonatype-lift · 2023-04-06T13:48:40Z

micrometer-core/src/main/java/io/micrometer/core/aop/TimedAspect.java

+            .tags(EXCEPTION_TAG, exceptionClass)
+            .tags(tagsBasedOnJoinPoint.apply(pjp))
+            .publishPercentileHistogram(timed.histogram())
+            .publishPercentiles(timed.percentiles().length == 0 ? null : timed.percentiles());


NullTernary: This conditional expression may evaluate to null, which will result in an NPE when the result is unboxed.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-04-06T13:48:42Z

micrometer-commons/src/main/java/io/micrometer/common/annotation/AnnotationUtils.java

+        int i = 0;
+        for (Annotation[] parameter : parameters) {
+            for (Annotation parameter2 : parameter) {
+                if (tagClazz.isAssignableFrom(parameter2.getClass())) {


GetClassOnAnnotation: Calling getClass() on an annotation may return a proxy class

Suggested change

if (tagClazz.isAssignableFrom(parameter2.getClass())) {

if (tagClazz.isAssignableFrom(parameter2.annotationType())) {

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-04-06T13:48:44Z

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.¹

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/micrometer-metrics/micrometer/3727.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/micrometer-metrics/micrometer/3727.diff | git apply

Once you're satisfied, commit and push your changes in your project.

You can preview the patch by opening the patch URL in the browser. ↩

marcingrzejszczak · 2023-04-07T10:18:58Z

I messed up the rebasing and I missed this important commit when merging this PR bc59c47 😬

check micrometer-metrics/micrometer#3727 and micrometer-metrics/micrometer@bc59c47

See micrometer-metricsgh-3727

* Polish MeterTag changes See gh-3727 * Update micrometer-commons/src/main/java/io/micrometer/common/annotation/AnnotationUtils.java Co-authored-by: sonatype-lift[bot] <37194012+sonatype-lift[bot]@users.noreply.github.com> --------- Co-authored-by: Marcin Grzejszczak <marcin@grzejszczak.pl> Co-authored-by: sonatype-lift[bot] <37194012+sonatype-lift[bot]@users.noreply.github.com>

marcingrzejszczak mentioned this pull request Mar 30, 2023

Aligning with annotations changes in Micrometer micrometer-metrics/tracing#218

Closed

shakuzen reviewed Apr 3, 2023

View reviewed changes

micrometer-commons/build.gradle Show resolved Hide resolved

marcingrzejszczak mentioned this pull request Apr 3, 2023

@Timed annotation with dynamic tags derived from input arguments and/or output #1732

Closed

marcingrzejszczak marked this pull request as ready for review April 3, 2023 11:51

shakuzen requested a review from jonatan-ivanov April 4, 2023 06:45

shakuzen reviewed Apr 4, 2023

View reviewed changes

marcingrzejszczak force-pushed the issues_#1732 branch from e59a23e to 87c07a3 Compare April 6, 2023 13:31

sonatype-lift bot reviewed Apr 6, 2023

View reviewed changes

marcingrzejszczak added 3 commits April 7, 2023 10:06

WIP on MetricTag

fc44d68

Fixing osgi build

d5ca419

Updated the test

bfd076d

marcingrzejszczak force-pushed the issues_#1732 branch from 87c07a3 to bfd076d Compare April 7, 2023 08:27

shakuzen approved these changes Apr 7, 2023

View reviewed changes

marcingrzejszczak merged commit b78c24c into main Apr 7, 2023
8 checks passed

marcingrzejszczak deleted the issues_#1732 branch April 7, 2023 09:56

marcingrzejszczak added a commit that referenced this pull request Apr 7, 2023

Applied missing commit for #3727 and #1732

bc59c47

marcingrzejszczak added a commit to micrometer-metrics/tracing that referenced this pull request Apr 7, 2023

Following changes from MeterTag PR

13c0038

check micrometer-metrics/micrometer#3727 and micrometer-metrics/micrometer@bc59c47

shakuzen changed the title ~~MetricTag~~ MeterTag Apr 7, 2023

izeye added a commit to izeye/micrometer that referenced this pull request Apr 16, 2023

Polish MeterTag changes

628dfd4

See micrometer-metricsgh-3727

izeye mentioned this pull request Apr 16, 2023

Polish MeterTag changes #3769

Merged

jonatan-ivanov mentioned this pull request Jun 20, 2023

Improve Tag propagation for @Timed #1586

Open

shakuzen mentioned this pull request Apr 24, 2024

fix: AnnotationHandler should read annotations from parent class #4985

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MeterTag #3727

MeterTag #3727

marcingrzejszczak commented Mar 30, 2023

shakuzen left a comment

shakuzen Apr 4, 2023

shakuzen Apr 4, 2023

sonatype-lift bot Apr 6, 2023

sonatype-lift bot Apr 6, 2023

sonatype-lift bot commented Apr 6, 2023

marcingrzejszczak commented Apr 7, 2023

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file

	if (tagClazz.isAssignableFrom(parameter2.getClass())) {
	if (tagClazz.isAssignableFrom(parameter2.annotationType())) {

MeterTag #3727

MeterTag #3727

Conversation

marcingrzejszczak commented Mar 30, 2023

shakuzen left a comment

Choose a reason for hiding this comment

shakuzen Apr 4, 2023

Choose a reason for hiding this comment

shakuzen Apr 4, 2023

Choose a reason for hiding this comment

sonatype-lift bot Apr 6, 2023

Choose a reason for hiding this comment

sonatype-lift bot Apr 6, 2023

Choose a reason for hiding this comment

sonatype-lift bot commented Apr 6, 2023

🛠 Lift Auto-fix

Footnotes

marcingrzejszczak commented Apr 7, 2023