Add Context parameter to Enabled for synchronous metrics instruments

[pellared]

Fixes #4256

Towards #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

[jack-berg]

While I don't disagree that a Context argument would be useful on the metric Enabled operation, I disagree with the process in this PR.

Couple of problems:

• There are no prototypes that I can see.
• As I mentioned in the corresponding issue, there is no corresponding SDK behavior that can change as a result of this behavior. @pellared makes the argument that such behavior can be added in the future and adding the argument later would be a burden. He made a similar argument when adding additional parameters to the logger Enabled operation, to which I conceded that its acceptable to break up the API and SDK bits for faster iteration. I regret that comment. 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. Skipping the SDK portion of this (or saying we'll come back to it later) erodes our ability to properly evaluate proposals, and the arguments become hand-wavy "this seems right" and "this could be useful for future hypothetical". Adding corresponding SDK behaviors for proposed API enhancements adds additional burden, but keeps us honest. I propose we put a stop to this loose approach API evolution, and codify the idea I'm discussing as part of spec requirements.

Requesting changes to ensure this doesn't get merged before we have a chance to talk about this topic in the spec SIG.

[pellared]

He made a similar argument when adding additional parameters to the #4203 (comment) 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 #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: open-telemetry/opentelemetry-go#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 #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 #4256 (comment).

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:

• Add Context parameter to Enabled for metrics instruments #4256 (comment)
• Add Context parameter to Enabled for metrics instruments #4256 (comment)

[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]

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]

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

[github-actions]

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