open-telemetry / opentelemetry-java-instrumentation

OpenTelemetry auto-instrumentation and instrumentation libraries for Java
https://opentelemetry.io
Apache License 2.0
1.94k stars 845 forks source link

Metrics Annotations #7030

Open fstab opened 2 years ago

fstab commented 2 years ago

Frameworks like Spring Boot, MicroProfile, and Dropwizard offer annotations like @Timed or @Counted for creating metrics from method calls.

The benefit is that developers can provide metrics for their business logic while still separating business code from metrics tracking.

@Timed
@Counted
public void processPayment() {
    // business code here
}

The OpenTelemetry Java auto instrumentation supports the @WithSpan annotation for tracing, but has nothing equivalent for metrics yet.

Brainstorming of a few ideas:

Moreover, it would be awesome to integrate this with @WithSpan so that develpers can say "I want to time this method AND have a Span". However, it should also be possible to use tracing without metrics, because histograms may introduce a lot of cardinality, and users might just want to create a Span without creating a histogram.

What do you think, is it worthwhile to put some more thoughts into this?

jkwatson commented 2 years ago

I think things that are a strict SDK responsibility (bucket definitions for example) should not end up being referenced in an annotation, since the annotations should be a pure API artifact.

Aside from that detail, I think having something like this would be a great idea.

fstab commented 2 years ago

Thanks for the quick response.

If a user wants to time something that takes microseconds they certainly want to use different buckets than if they time something that takes milliseconds or even seconds.

How would a user do that? Are there alternatives to configuring buckets in the annotation?

jkwatson commented 2 years ago

At the moment, all configuration of how histograms work (and metrics aggregations in general) is the responsibility of the application operator via SDK configuration, and is not a concern assigned to instrumentation authors. In the future, we may have what we've been calling a "hint" API that would allow instrumentation call sites to inform the SDK of what they would prefer, but that is not currently specified.

fstab commented 2 years ago

Thanks a lot for the explanation, this makes sense. In that case the annotations would be quite simple, as they only provide configuration options for the metric name and attributes.

I could open a PR to add them to extensions/annotations/ so that we can continue the discussion based on concrete examples. What do you think?

jkwatson commented 2 years ago

Thanks a lot for the explanation, this makes sense. In that case the annotations would be quite simple, as they only provide configuration options for the metric name and attributes.

I could open a PR to add them to extensions/annotations/ so that we can continue the discussion based on concrete examples. What do you think?

That would be fine. Since these annotations are generally implemented by the instrumentation folks, I'd love some feedback on this proposal from @trask @mateuszrzeszutek @anuraaga and others.

trask commented 2 years ago

I support! please tag @open-telemetry/java-instrumentation-approvers on your PR and we can discuss specifics there

fstab commented 2 years ago

Opened PR open-telemetry/opentelemetry-java#4260.

trask commented 1 year ago

@zeitlinger this could be a good use case for a convenience API until we implement an annotation-driven approach

linking to https://github.com/open-telemetry/opentelemetry-java-contrib/pull/759

zeitlinger commented 1 year ago

While this issue could be implemented with methods, it's quite unrelated (and increasing the scope of) https://github.com/open-telemetry/opentelemetry-java-contrib/pull/759 - but it definitely shows that it's worth evolving the otel apis.

Duncan-tree-zhou commented 5 months ago

Hi there, I think it's a very helpful feature for user. Is anyone already working on it? If no, I would like to pick it up.

danielgblanco commented 5 months ago

I can support @Duncan-tree-zhou on any questions he may have during the contribution process. Thanks Duncan!

zeitlinger commented 5 months ago

I don't think anyone is working on it right now.

Duncan-tree-zhou commented 5 months ago

Opened PR open-telemetry/opentelemetry-java#4260.

copy the PR from opentelemetry-java to repo opentelemetry-java-instrumentation.

https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/11285

Duncan-tree-zhou commented 5 months ago

Hi here, I need some help.

When I am writing the test cases, I found a bug on the current annotation api design. the problem is caused by the default name. see the following cases:


public class CountedExample {

  @Counted
  public void method1() {
  }

  @Counted(description = "I am the description.")
  public void method2() {
  }

  @Counted(unit = "kb")
  public void method3() {
  }

}

for method1 it request to create a LongCounter with name "method.invocation.count", empty description and empty unit. for method2 it request to create a LongCounter with name "method.invocation.count", description "I am the description." and empty unit. for method3 it request to create a LongCounter with name "method.invocation.count", empty description and unit "kb".

So if I create a metric instance for method1, then I can not create the metric for method2 because they are have got a same metric name but with different properties. If we use the same default name for each method with @Counted, the cases above would occur frequently.

There are at lease two way to reduce the above cases:

  1. do not allow user to set metric properties, or allow user to set the counted, timed metrics in a global configuration.
  2. fill a method-specified name as the default metric name. for example, {package name}.{class name}.{method name}.count or {class name}.{method name}.count

I think the second one would be more friendly to the user. No sure if there are any other better solution.

trask commented 5 months ago

it seems like there are two different use cases:

  1. store all metrics under the same generic name method.invocation.count, in which case we don't want user to override description or unit, and we do want to automatically add code.namespace and code.function attribute
  2. user supplies their own metric name, in which case we want them to be able to override description and unit, and we probably do not want to automatically add code.namespace and code.function

maybe we should just target use case 2 for now?

or maybe a separate annotation for use case 1, e.g. @SimpleCounted?

Duncan-tree-zhou commented 5 months ago

How about to use {code.namespace}.{code.function}.count as the default value, it could avoid most of the ambiguous definition problem when the description or unit is not empty. And we could still automatically add code.namespace and code.function attributes, I guest user can drop it in view configuration if they don't want it?

Duncan-tree-zhou commented 5 months ago

the other solution is to use {code.namespace}.{code.function}.count as default value when the description or unit is not empty(suitable for case2 scenario). And to use method.invocation.count when both description and unit are empty(suitable for case1 scenario).