Add Context parameter to Enabled for synchronous metrics instruments

[pellared]

Fixes https://github.com/open-telemetry/opentelemetry-specification/issues/4256

Towards https://github.com/open-telemetry/opentelemetry-specification/issues/4215

Other methods with Context parameter:

• https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#context-interaction
• https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#span-creation
• https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#synchronous-and-asynchronous-instruments
• https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/bridge-api.md#emit-a-logrecord
• https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/bridge-api.md#enabled

[pellared]

He made a similar argument when adding additional parameters to the https://github.com/open-telemetry/opentelemetry-specification/pull/4203#issuecomment-2341410062 operation, to which I conceded that its acceptable to break up the API and SDK bits for faster iteration.

There are no prototypes that I can see.

I just want to clarify that for comparing these cases is not fair as for Logger.Enabled we already have experimental support in OTel Logs SDK with examples for Context usage. See: https://pkg.go.dev/go.opentelemetry.io/otel/sdk/log#example-Processor-Filtering. I am also actively working on https://github.com/open-telemetry/opentelemetry-specification/issues/4207. Moreover all log bridges implemented in Contrib shows us (which you can see as users) that we need this API for Go.

Regarding this issue and PR, the problem for my point of view, as an OTel Go maintainer, is that if https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#enabled is stabilized as it is then it is ambiguous if adding a context.Context to Enabled would be compliant with the specification or not. (sorry for long, complex, maybe even not correct sentence; feel free to edit it)

Our API operations and arguments should (and I'd like to say must) always be grounded in reality, with useful corresponding behaviors in the SDK reference implementations.

From this perspective it seems that it would be breaking (but my interpretation may be wrong). @jmacd here finds that it would be OK and compliant. For me, it is uncertainty.

This is the reason why I created the issue and this PR. Adding a parameter (later) to a method is a breaking change in Go and there is a pattern even documented in the standard library that:

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it.

I have a strong opinion, that in Go, we should not provide Enabled without context.Context.

I do not think it makes sense to add and release experimental API just to convivence that we need context.Context. It would be a huge burden for OTel Go maintainers and contributors and we would even not know how long we would have to deal with this method being experimental. More: https://github.com/open-telemetry/opentelemetry-go/issues/5882

Maybe the resolution is to specify that languages that require passing Context explicitly MAY add Context parameter to any API and SDK operation? I made a similar proposal here.

Side note:

I regret that comment.

We are fine. I think you are doing a very good job and that your feedback is important.

[jack-berg]

I just want to clarify that for comparing these cases is not fair as for Logger.Enabled we already have experimental support in OTel Logs SDK with examples for Context usage. See: https://pkg.go.dev/go.opentelemetry.io/otel/sdk/log#example-Processor-Filtering. I am also actively working on https://github.com/open-telemetry/opentelemetry-specification/issues/4207.

That capability does not exist in the log SDK doc, so it falls under "we'll come back to it later" from my comment.

Regarding this issue and PR, the problem for my point of view, as an OTel Go maintainer, is that if https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#enabled is stabilized as it is then it is ambiguous if adding a context.Context to Enabled would be compliant with the specification or not.

I agree that would be unfortunate. It doesn't seem like stabilization is imminent, and we can / should make the consideration of Context parameter a blocking issue for stability.

I have a strong opinion, that in Go, we should not provide Enabled without context.Context.

Again, I'm not arguing that we shouldn't have a Context parameter - I actually think it makes sense. Just that we need to evolve the API and SDK in lockstep. Someone needs to do the work to explore, prototype, propose what the corresponding SDK behavior that consumes Context looks like.

Maybe the resolution is to specify that languages that require passing Context explicitly MAY add Context parameter to any API and SDK operation? I made a similar proposal https://github.com/open-telemetry/opentelemetry-specification/issues/4256#issuecomment-2417942094.

That doesn't seem right. Should languages with explicit context pass it when obtaining a tracer, meter, logger? Should all the span operations to add / update fields include a context argument?

[jack-berg]

We discussed in the points about process from this comment in the 10/23/24 TC meeting.

The question is: Should we add spec process language requiring that API proposals have corresponding SDK proposals?

There was general consensus that we want to be judicious about adding new process, and rely on approver / TC judgement to evaluate individual cases. But at the same time, there was also consensus that it is generally a good practice to propose API changes with corresponding SDK changes (even if not strictly required). We talked about potentially adding something akin to a style guide or a values document, enumerating the types of things that are generally associated with successful PRs, but without normative language that might land us in process hell.

For this PR in particular, we need to see a prototype. And speaking for myself, I'd like to see a more complete proposal showing the corresponding SDK changes.

[pellared]

For this PR in particular, we need to see a prototype. And speaking for myself, I'd like to see a more complete proposal showing the corresponding SDK changes.

What if we do not have any prototype but we know that for Go it will be very hard to add this parameter in future if it will become needed.

For instance, the Context required for measurements was also a "noop" until exemplars feature was added. (I may be wrong).

Also some references to @jmacd comments:

• https://github.com/open-telemetry/opentelemetry-specification/issues/4256#issuecomment-2417228413
• https://github.com/open-telemetry/opentelemetry-specification/issues/4256#issuecomment-2417923028

[jack-berg]

What if we do not have any prototype but we know that for Go it will be very hard to add this parameter in future if it will become needed.

We should block stabilizing the operation until we resolve whether Context is needed.

[github-actions[bot]]

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

[pellared]

Changing to draft until someone creates a prototype that would also demonstrate how the SDK could handle the passed context.

Disclaimer: I am not working on any prototype, but I feel that this feature is important.

[jack-berg]

Left a comment on the issue about Context parameter of Logger#enabled, which is intertwined with the proposal of this PR.

[github-actions[bot]]

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

[github-actions[bot]]

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