Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Context parameter to Enabled for synchronous metrics instruments #4262

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

@pellared pellared added area:api Cross language API specification issue spec:metrics Related to the specification/metrics directory labels Oct 15, 2024
@pellared pellared marked this pull request as ready for review October 15, 2024 19:05
@pellared pellared requested review from a team as code owners October 15, 2024 19:05
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

pellared commented Oct 16, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

pellared commented Oct 23, 2024

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:

@jack-berg
Copy link
Member

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.

Copy link

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

@pellared
Copy link
Member Author

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.

@pellared pellared marked this pull request as draft October 31, 2024 20:18
@jack-berg
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Context parameter to Enabled for metrics instruments
7 participants