do we really need a separate annotation for each type of instrument attribute? Could this just be "MetricAttribute" and apply to both? Or do you need to separate the attributes for the counter and the attributes for a timer, if you happen to have both of them? I would argue, since timers generate histograms, and histograms implicitly have counts of the total invocations in them, that you wouldn't want to or need to annotation a method with both, unless the use-case is extremely unusual.

Could we perhaps reuse SpanAttribute somehow? Or deprecate it, and introduce a new annotation that would apply to both signals?

@jkwatson true, it does not make sense to use both @Timed and @Counted since timers already include the count. I'm happy to merge TimerAttribute and CounterAttribute into a single MetricAttribute.

I pushed an update replacing TimerAttribute and CounterAttribute with a single MetricAttribute.

Could we perhaps reuse SpanAttribute somehow? Or deprecate it, and introduce a new annotation that would apply to both signals?

A couple days ago somebody asked for the same thing for spans: open-telemetry/opentelemetry-java-instrumentation#5527
Instead of an attribute, perhaps this could be a separate annotation? That's used by both signals (traces and metrics)

There should be an option to use an attribute for a span but not for a metric. For metrics, each set of attributes creates a new time series, and users must be careful to use only attributes that have a limited number of possible different values. For spans that is not an issue. So it might make sense to use arbitrary parameters, return values, and exceptions as Span attributes but not use them as metric attributes.

I guess if we want to have an option to use the return value as a span attribute but not as a metric attribute we don't need an extra annotation, because then we cannot reuse it for spans and metrics anyway. In that case we could just leave the returnValueAttribute parameter.

I think @mateuszrzeszutek was suggesting something like this:

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

Yeah, exactly that -- and this annotation could be used to fill in attributes both in spans and metrics.

I'm not quite sure whether we should have common attributes for both spans and metrics, or whether we should separate them. The difference is that with metrics each new attribute value creates a new time series. Therefore you only want to choose attributes that have a limited number of possible values. With Spans that's not the case.

An example would be a method returning a unique identifier: It would be valid to use the return value as a Span attribute, but it would be a bad idea to use it as a metric attribute.

For method parameters the current proposal is to have two separate annotations: @MetricAttribute and @SpanAttribute. If you want a parameter to be used in both, metric and span, you would need to add both annotations.

If we want to have the same separation for return values, we would end up with something like @MetricReturnAttribute and @SpanReturnAttribute. That's fine. I find it shorter to have this as a parameter of @Counted, @Timed, and maybe @WithSpan, but I have no strong opinion about this.

However, having a common @ReturnAttribute for both metrics and spans would make it impossible to use a high cardinality return value as a Span attribute without using it as a metric attribute at the same time.

What do you think?

Oh, I understand now, yeah that makes sense that you would want control over which attributes are tagged on the span vs tagged on the metric, thx

Instead of class and method, WDYT about using code.namespace and code.function? These attributes are already in the spec (for spans, at least), and already are (or should be) captured by the agent.

Thanks for the heads-up. If that's the standard attribute names from the Spec we should use them.

I pushed an update renaming this to the standard attributes.

Hmm, can we use exception.type and Class#getCanonicalName() instead? It's what Span#recordException() uses.

Thanks for the heads-up. This was inspired by what Micrometer does, but if there are standard attributes in OpenTelemetry we should use them.

I pushed an update using exception.type and the canonical name.

Is this method necessary? The same thing can be achieved by a View if somebody wants that; and turning off the exception capturing does not seem that useful.

I'm not sure if there is a situation where Exceptions can result in cardinality explosion because a large number of different Exception classes is thrown. If this can happen it would make sense to have a way to disable the exception attribute. If we are sure that Exception classes are always low cardinality we could remove this.

It's technically possible to have hight cardinality here (like autogenerating exception classes at runtime 🙈 ), but it's very low likely to happen -- and again, you can use Views to filter this attribute out if you somehow happen to run into such scenario.

I pushed an update removing the disableExceptionAttribute parameter.

since method.invocations.total hasn't been spec'd, maybe we shouldn't have a default, or maybe we should move the metrics annotations to an alpha package for now?

Moving to an alpha package sounds good. Could you let me know how to do this? I don't know the conventions too well.

just curious, do you have a particular use case in mind for these additional attributes?

Something that comes to my mind would be this:

  @Timed(attributes = {"paymentType", "CreditCard"})
  public void processPayment(CreditCard creditCard) {
    // ...
  }

  @Timed(attributes = {"paymentType", "DebitCard"})
  public void processPayment(DebitCard debitCard) {
    // ...
  }

Does this make sense?

on the instrumentation side, we don't populate metric attributes when they are n/a, I definitely don't understand some of the nuances of metric design, is populating "None" needed for some reason?

Good catch. The reason why it's there is because that's what Micrometer does, but I guess not populating it would be better.

In Prometheus missing attributes are treated the same way as empty attributes. I pushed an update saying that the attribute is only present if an Exception is thrown. Is this ok, or would it be better to always keep the attribute but with an empty value?

I think @mateuszrzeszutek was suggesting something like this:

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

Should this default to "1" ?

OTEL usually defaults to milliseconds, e.g. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md