opentelemetry-java icon indicating copy to clipboard operation
opentelemetry-java copied to clipboard

Add Metrics Annotations

Open fstab opened this issue 2 years ago • 10 comments

Add annotations @Timed and @Counted for automatically instrumenting method invocations.

The annotations are similar to the annotations offered by Micrometer, MicroProfile metrics, and Dropwizard metrics.

The default attributes class, method, and exception are similar to the default attributes provided by Micrometer's annotations.

Here's an example of what a @Counted metric would look like in Prometheus format:

method_invocations_total{class="com.example.demo.Example", method="example", exception="None"} 1.0

This PR is a follow-up to the discussion in #4251.

fstab avatar Mar 12 '22 13:03 fstab

Codecov Report

Merging #4260 (eaa90a3) into main (5f32636) will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    open-telemetry/opentelemetry-java#4260   +/-   ##
=========================================
  Coverage     90.04%   90.04%           
  Complexity     5057     5057           
=========================================
  Files           580      580           
  Lines         15573    15573           
  Branches       1497     1497           
=========================================
  Hits          14022    14022           
  Misses         1090     1090           
  Partials        461      461           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5f32636...eaa90a3. Read the comment docs.

codecov[bot] avatar Mar 12 '22 13:03 codecov[bot]

Do we have a planned version for this to be merge for? This seems like a valuable feature. Hoping it wasn't abandoned.

tylerlance avatar Jun 02 '22 13:06 tylerlance

It feels strange accepting additions to the stable extension annotations module without knowing what an implementation of a corresponding annotation processor looks like.

Can opentelemetry-extension-annotations move to opentelemetry-java-instrumentation to live alongside the annotation processor implementation?

If not, can we we put this in opentelemetry-extension-incubator while we build confidence in the API / implementation?

jack-berg avatar Jun 02 '22 15:06 jack-berg

My initial impulse is that opentelemetry-extension-incubator is a better fit because users would explicitly use these annotations in their code. Should I move it?

fstab avatar Jun 02 '22 16:06 fstab

I moved it to extensions/incubator/. The new annotations are now in package io.opentelemetry.extension.incubator.annotations.

From the discussion above I think the only unresolved point is how to use return values as attributes.

Current implementation:

  @Timed(returnValueAttribute = "process.result")
  public int process() {
    return 3;
  }

Alternative proposal:

  @Timed
  @ReturnAttribute("process.result")
  public int process() {
    return 3;
  }

I think the idea behind the alternative proposal is that @ReturnAttribute could work for both, spans and metrics. However, there is a fundamental difference between spans and metrics: For metrics, each attribute value creates a new time series. If you use a high cardinality return value as a metric attribute, you might overload the monitoring backend. For spans, high cardinality attributes are not an issue.

For that reason I think we should provide a way to say that a return value will be a span attribute, but not a metric attribute. With returnValueAttribute being a property of the @Timed and @Counted annotations, it is clear that this is only for the metric but does not affect any spans.

Anyway, I'm happy to change this if consensus is that we should go with a dedicated @ReturnAttribute annotation.

fstab avatar Jun 11 '22 15:06 fstab

@fstab sorry about the delay on this. We're going to discuss at today's OTel java office hours and will respond with a recommendation on next steps ASAP.

jkwatson avatar Jun 16 '22 23:06 jkwatson

Hi @fstab just following up on @jkwatson's comment. Here's where we're at:

  • We don't think opentelemetry-java is the right long term home for these annotations, given that the primary implementation lives in opentelemetry-java-instrumentation. We'd like to make opentelemetry-java-instrumentation the long term home of annotations.
  • Modules in opentelemetry-java-instrumentation generally live in io.opentelemetry.instrumentation.* package, versus io.opentelemetry.* for opentelemetry-java projects. In order to continue following this pattern while not breaking compatibility for the annotations module, we're going to start a new annotations module in opentelemetry-java-instrumentation, deprecating the opentelemetry-java opentelemetry-extension-annotations module until we ultimately stop publishing it starting with the next major version.
  • We'd like to not get further away from this target, and merging this until the span annotations have been moved to opentelemetry-java-instrumentation. At that point, we can open a PR adding these annotations to that new module, and simultaneously include an implementation for the annotations.

Again, sorry for the delay.

jack-berg avatar Jun 17 '22 22:06 jack-berg

I've kicked off the discussion in the instrumentation repo https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/6245, and added the topic of naming to the SIG agenda for tomorrow. Once we close on naming, I'll do the initial copy of existing annotations over to the instrumentation repo.

trask avatar Jun 30 '22 01:06 trask

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar Jul 22 '22 20:07 github-actions[bot]

hey @fstab! we've finished moving the @WithSpan instrumentation annotations to the instrumentation repo (https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation-annotations). we're keeping the existing annotations around in this repository (and still supporting them in auto-instrumentation for backwards compatibility), but would like to add new features going forwards in the new module now

trask avatar Aug 01 '22 16:08 trask

I'm going to close this because now that we've moved forward with the decision to move annotations to opentelemetry-java-instrumentation.

jack-berg avatar Aug 12 '22 16:08 jack-berg

@fstab Could you summarize what the current status of your (great) idea with annotations for metrics is?

cptkng avatar Apr 01 '23 20:04 cptkng