add annotations for metrics #11285

Duncan-tree-zhou · 2024-05-04T10:30:16Z

Initially it's a copy from PR: open-telemetry/opentelemetry-java#4260. I am moving the annotations apis to repo opentelemetry-java-instrumentation , project instrumentation-api-incubator.

orignal issue: #7030

laurit · 2024-05-07T13:14:05Z

@Duncan-tree-zhou do you also plan to provide instrumentation that would use these annotations?

Duncan-tree-zhou · 2024-05-07T15:08:24Z

@Duncan-tree-zhou do you also plan to provide instrumentation that would use these annotations?

Yes, I am working on it. But it's a little tricky to setup the test environment. It takes some time....

Duncan-tree-zhou · 2024-05-07T17:00:27Z

Hi @laurit, I can not run test with JDK 21 after the commit dfc79ebecebe340da2ac911e048150841776046a, it throws failure like:

...
> Task :instrumentation-api-incubator:processResources NO-SOURCE
> Task :custom-checks:compileJava FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':custom-checks:compileJava'.
> error: invalid flag: -Xlint:-this-escape
* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Get more help at https://help.gradle.org.
...

While I can run the test with JDK 17 in commit c92955fa2fb8beae9cf599bb164dd9d9cbd15805 (the one before upgrade sdk to 21).

Not sure if it's a bug. I see the commit was created by you, do you have any suggestion on how to skip the build failure?

laurit · 2024-05-08T05:07:52Z

Hi @laurit, I can not run test with JDK 21 after the commit dfc79ebecebe340da2ac911e048150841776046a, it throws failure like:

...
> Task :instrumentation-api-incubator:processResources NO-SOURCE
> Task :custom-checks:compileJava FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':custom-checks:compileJava'.
> error: invalid flag: -Xlint:-this-escape
* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Get more help at https://help.gradle.org.
...

jdk 21 is now required to build this project, this error happens when you are still using jdk 17

jack-berg

Very nice!

jack-berg · 2024-05-16T17:33:06Z

...ubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/annotations/Counted.java

+   * <p>Unit strings should follow the instrument unit rules: <a
+   * href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument-unit">https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument-unit</a>
+   */
+  String unit() default "1";


Suggested change

String unit() default "1";

String unit() default "{invocation}";

See https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/runtime-telemetry/runtime-telemetry-java8/library/src/main/java/io/opentelemetry/instrumentation/runtimemetrics/java8/Classes.java#L55 for example of another instrument counting things.

is the "{invocation}" a placeholder? I can not understand how it works from the reference, would you like to amplify a bit more on it?

This explains it in more detail: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/general/metrics.md#instrument-units

jack-berg · 2024-05-16T17:38:16Z

...ncubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/annotations/Timed.java

+   * <p>Description strings should follow the instrument description rules: <a
+   * href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument-description">https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument-description</a>
+   */
+  String description() default "1";


Description should probably default to not set.

I aware that the "intrumentation-api-incubator" is not the good place to put the annotations. so I had moved them to "instrumentation-annotations" in another pr #11354. Despite it build failed due to the new instrumentation implementation, I think we can discuss on that one and close this one

jack-berg · 2024-05-16T17:39:55Z

...ncubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/annotations/Timed.java

+   *
+   * <p>Default is millis seconds.
+   */
+  TimeUnit unit() default TimeUnit.MILLISECONDS;


The default unit for durations in opentelemetry is seconds. This also means that this needs to record to a DoubleHistogram instead of a LongHistogram, which makes no difference because long measurements are converted to doubles anyway before export.

breedx-splk · 2024-05-17T20:28:11Z

...ubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/annotations/Counted.java

+ *   <li><b>code.namespace:</b> The fully qualified name of the class whose method is invoked.
+ *   <li><b>code.function:</b> The name of the annotated method, or "new" if the annotation is on a


For anyone wondering, yes these are consistent with the semantic convention registry. 👍🏻

should I put the link in the code or copy the description from the reference?

...ubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/annotations/Counted.java

breedx-splk · 2024-05-17T20:35:28Z

...ubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/annotations/Counted.java

+   * <p>The name of the attribute for the return value of the method call. {@link Object#toString()}
+   * will be called on the return value to convert it to a String.
+   *
+   * <p>By default, the instrument will not have an attribute with the return value.


I think that this should also come with a warning that using this can very likely explode metric dimension cardinality.

It's a good point, and MetricAttribute would have the same problem.

breedx-splk

I'm stoked on these and can't wait to see them in action!

Duncan-tree-zhou · 2024-05-19T14:07:02Z

Hi friends, thanks for your suggestions, I adapt the changes in #11354.

trask · 2024-05-20T19:02:14Z

Hi friends, thanks for your suggestions, I adapt the changes in #11354.

does #11354 replace this PR? if so we can close this one, thanks

add annotations for metrics

7d0a18e

Duncan-tree-zhou requested a review from a team May 4, 2024 10:30

Duncan-tree-zhou mentioned this pull request May 4, 2024

Metrics Annotations #7030

Open

jack-berg reviewed May 16, 2024

View reviewed changes

breedx-splk reviewed May 17, 2024

View reviewed changes

...ubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/annotations/Counted.java Show resolved Hide resolved

breedx-splk reviewed May 17, 2024

View reviewed changes

Duncan-tree-zhou closed this May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add annotations for metrics #11285

add annotations for metrics #11285

Duncan-tree-zhou commented May 4, 2024 •

edited

Loading

laurit commented May 7, 2024

Duncan-tree-zhou commented May 7, 2024 •

edited

Loading

Duncan-tree-zhou commented May 7, 2024 •

edited

Loading

laurit commented May 8, 2024

jack-berg left a comment

jack-berg May 16, 2024

Duncan-tree-zhou May 19, 2024

jack-berg May 20, 2024

jack-berg May 16, 2024

Duncan-tree-zhou May 19, 2024

jack-berg May 16, 2024

breedx-splk May 17, 2024

Duncan-tree-zhou May 19, 2024

breedx-splk May 17, 2024

Duncan-tree-zhou May 19, 2024

breedx-splk left a comment

Duncan-tree-zhou commented May 19, 2024

trask commented May 20, 2024

	String unit() default "1";
	String unit() default "{invocation}";

		* <li><b>code.namespace:</b> The fully qualified name of the class whose method is invoked.
		* <li><b>code.function:</b> The name of the annotated method, or "new" if the annotation is on a

add annotations for metrics #11285

add annotations for metrics #11285

Conversation

Duncan-tree-zhou commented May 4, 2024 • edited Loading

laurit commented May 7, 2024

Duncan-tree-zhou commented May 7, 2024 • edited Loading

Duncan-tree-zhou commented May 7, 2024 • edited Loading

laurit commented May 8, 2024

jack-berg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breedx-splk left a comment

Choose a reason for hiding this comment

Duncan-tree-zhou commented May 19, 2024

trask commented May 20, 2024

Duncan-tree-zhou commented May 4, 2024 •

edited

Loading

Duncan-tree-zhou commented May 7, 2024 •

edited

Loading

Duncan-tree-zhou commented May 7, 2024 •

edited

Loading