Add Context parameter to Enabled for metrics instruments

[pellared]

We should add Context as a parameter similarly to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/bridge-api.md#enabled.

Context is needed for allowing "context related" behavior. For instance, it enables to user to disable the instrument for some key/value in the context has been set. It can also used to see if the call is within a span. Related PR: #4203

Created here.

[jack-berg]

Why?

[pellared]

Why?

Description updated. Thanks

[jack-berg]

There's no corresponding SDK behavior that would allow the enabled API to change its response based on context.

[pellared]

There's no corresponding SDK behavior that would allow the enabled API to change its response based on context.

A custom API implementation (e.g. SDK wrapper) could change its response based on context.
The SDK can add such behavior in future.

I find that adding is important for Go that require passing Context explicitly. Adding it later to the API would be a burden.
For reference: https://go.dev/wiki/CodeReviewComments#contexts.

CC @open-telemetry/go-maintainers, @open-telemetry/collector-maintainers

[svrnm]

@open-telemetry/technical-committee since there is precedence for this already, we think that we can move forward with this, any objections? (cc @pellared @jpkrohling @danielgblanco)

[mx-psi]

Agree with @pellared that adding this is important for Go

[jmacd]

IMO there is no need to modify the specification here.

To me, this is a Go-specific question. Golang uses explicit context, and all API calls should have context independent of whether OTel specifications explicitly mention context. There is no need to modify the OTel specification to add a Context parameter in Golang. Most OTel APIs rely on implicit context, like being able to get a thread-local context object. Golang does not have thread-local state, so we always expect a Context argument.

It shouldn't require a lot of imagination to see how context is useful for customized SDK purposes. A custom metric SDK could record metric events to the active span when the context is sampled, for example.

[pellared]

OTel APIs rely on implicit context

This is why in almost all places Context is added as required for languages requiring explicit context. I would prefer to have the specification consistent.

[jmacd]

@pellared I can't find what you're referring to. In the metrics api.md the word "Context" appears very little, certainly not as a required parameter anywhere. For metrics, we see:

  [Measurements](#measurement) recorded by synchronous instruments can be
  associated with the [Context](../context/README.md).

this implies that Enabled() should accept a context.

In the trace api.md, Context is used but it refers to the OTel context concept, not the Golang Context object.

Moreover, if you look at the one operation that absolutely requires a context, tracer.Start(), we see it is permitted to be implicit:

  The API MAY also have an option for implicitly using
  the current Context as parent as a default behavior.

This seems to set a precedent that context can be implicit. The definition of Context itself also repeats this:

Depending on the language, its usage may be either explicit or implicit.

I still believe it is not necessary to modify the specification in this case.

[pellared]

@pellared I can't find what you're referring to.

@jmacd, I added all these in the description of #4262

Context is used but it refers to the OTel context concept

Is there any reason that adding the OTel concept of context would be bad in this scenario? Especially given that in most languages it is always implicitly available.

I still believe it is not necessary to modify the specification in this case.

I prefer the specification to be less ambiguous. I would rather change the specification and call out that adding a Context parameter MAY be added to any operation if the language does not support implicit Context and requires passing Context explicitly.

[MrAlias]

+1 for making our specification explicit in its meaning.

[dmathieu]

+1 as well, for explicitness, but also consistency. When tracing mentions the context, we should also mention it for similar appropriate cases with other signals.