Repository navigation
Added SpanProcessor OnEnding callback - #6367
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6367 +/- ##
============================================
- Coverage 90.05% 90.01% -0.05%
- Complexity 6276 6330 +54
============================================
Files 697 703 +6
Lines 18949 19086 +137
Branches 1858 1881 +23
============================================
+ Hits 17065 17180 +115
- Misses 1312 1331 +19
- Partials 572 575 +3 ☔ View full report in Codecov by Sentry. |
| this.endEpochNanos = endEpochNanos; | ||
| hasEnded = true; | ||
| if (spanProcessor.isOnEndingRequired()) { | ||
| spanProcessor.onEnding(this); |
There was a problem hiding this comment.
I think it's probably a mistake to be calling this under the lock. I believe we only want to do internal state management while the lock is being held, not call not-under-our-control methods provided by users.
There was a problem hiding this comment.
The lock was actually intentionally hold to cover this part of the spec:
The SDK MUST guarantee that the span can no longer be modified by any other thread
before invokingOnEndingof the firstSpanProcessor. From that point on, modifications
are only allowed synchronously from within the invokedOnEndingcallbacks.
This part was added to the spec so that SpanProcessors can be sure that the state they see the span in is actually consistent and not prone to race conditions due to span modifications from the application code.
However, I now switched to a different way of implementing this, because as you said exposing the lock is probably not the best way of achieving the desired safety here.
# Conflicts: # docs/apidiffs/current_vs_latest/opentelemetry-sdk-trace.txt
jack-berg
left a comment
There was a problem hiding this comment.
Couple minor comments, but seems generally good. Thanks!
Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
|
I approve of putting this in a separate interface. But, we should provide documentation on how someone could use this, as well. |
What do you think would be the best place for this? Just a javadoc in |
I think the ideal way to do this is a unit test. Let's add an |
jack-berg
left a comment
There was a problem hiding this comment.
thanks for adding the example. couple of nits but ready otherwise ready to merge IMO
Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
) Related to open-telemetry#4024. ## Changes Add the Development-status `OnEnding` callback to the list of `SpanProcessor` methods that cannot be called after `Shutdown`. `OnEnding` was added after the shutdown lifecycle text was written, but it is currently omitted from the list alongside `OnStart`, `OnEnd`, and `ForceFlush`. This makes its shutdown behavior explicit and avoids giving `OnEnding` different lifecycle semantics. The ambiguity surfaced while implementing `OnEnding` in open-telemetry/opentelemetry-go#8957. * [ ] Related issues * [ ] Related [OTEP(s)](https://github.com/open-telemetry/oteps) * [x] Links to the prototypes * Go: open-telemetry/opentelemetry-go#8957 * Java: open-telemetry/opentelemetry-java#6367 * [x] [`CHANGELOG.md`](https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md) updated * [ ] [Spec compliance matrix](https://github.com/open-telemetry/opentelemetry-specification/blob/main/spec-compliance-matrix/template.yaml) updated if necessary * No update is necessary because `SpanProcessor.OnEnding` is already represented and implementation status is unchanged. * [ ] [Declarative config data model](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/data-model.md#overview) updated if necessary * Not applicable because this does not change SDK configuration.
Implementation for the spec change open-telemetry/opentelemetry-specification#4024.