-
Notifications
You must be signed in to change notification settings - Fork 889
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 ability for SpanProcessor to mutate spans on end #4024
Add ability for SpanProcessor to mutate spans on end #4024
Conversation
Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
What is the benefit of To my understanding both changes are breaking for |
One benefit is that while order of span processor registration matters for
We're working out the details of that in #4030, but I disagree that adding a new method to an SDK plugin interface is a breaking change. Languages vary in terms of how they expose these plugin interfaces and the language features for evolving interfaces, but there it should be possible for all SDKs to accommodate this type of change, albeit with some creativity in some cases. For example, its simple in java to add a new default method to an interface which existing implementers don't need to implement. This doesn't exist in some language like go, but there you can offer a new |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
@pellared I'd say we are ready to merge as this is in-development - but yes, once we want to go stable, we will need a few prototypes actually. WDYT? |
Co-authored-by: Damien Mathieu <42@dmathieu.com>
This has enough approvals and as it is experimental, merging it will help us motivate SIGs to prototype this. Will merge by EOD if there are no concerns. |
…4024) Fixes open-telemetry#1089. In addition to the comments on the issue, this was discussed in the spec SIG Meeting on 2024/23/04: * The filtering use-case explained in [this comment](open-telemetry#1089 (comment)) should rather be solved by the upcoming samplerV2 instead of `SpanProcessor`s due to better conceptual fit * The buffering use-case also explained in [this comment](open-telemetry#1089 (comment)) seems to be not relevant enough to influence the design decision * Apparently there was a discussion around building the `SpanProcessor`s in a chaining fashion during the initial SDK spec design and it was actively decided against it. However, no one could recall the reason why.
Fixes #1089.
In addition to the comments on the issue, this was discussed in the spec SIG Meeting on 2024/23/04:
SpanProcessor
s due to better conceptual fitSpanProcessor
s in a chaining fashion during the initial SDK spec design and it was actively decided against it. However, no one could recall the reason why.Based on this input, I decided to not move the chaining-based solution forward and stay with the original proposal of adding a new callback to be invoked just before a span is ended.
I also decided to name the new callback
OnEnding
instead ofBeforeEnd
as suggested in this comment. The nameOnEnding
is more precise about when the callback is invoked.A big discussion point in the SIG Meeting on 2024/23/04 also was the problem of evolving SDK plugin interfaces without breaking backwards compatibility. This PR contains a proposal to clarify how this should be handled: If the language allows it, interfaces should be directly extended. If not possible, implementations will need to introduce new interfaces and accept them in addition to the existing ones for backwards compatibility. I feel like this allow every language to implement changes to interfaces in the best way possible. Of course, changes to interfaces should still be kept to a necessary minimum.
I also wasn't sure whether this change warrants an addition to the spec-compliance-matrix, so I've left it out for now.
Please leave a comment if you think this is required, then I'll add it.
Changes
Adds a new
OnEnding
callback toSpanProcessor
Add a paragraph on clarifying how languages should deal with interface extensions
Related issues Add BeforeEnd to have a callback where the span is still writeable #1089
Related OTEP(s) #Links to the prototypes (when adding or changing features)
beforeEnd
in that PoC)CHANGELOG.md
file updated for non-trivial changesspec-compliance-matrix.md
updated if necessary