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

Added SpanProcessor OnEnding callback #6367

Merged
merged 18 commits into from
Sep 4, 2024

Conversation

JonasKunz
Copy link
Contributor

@JonasKunz JonasKunz commented Apr 9, 2024

Implementation for the spec change open-telemetry/opentelemetry-specification#4024.

@JonasKunz JonasKunz changed the title [PoC] Added SpanProcessor beforeEnd callback Added SpanProcessor beforeEnd callback Aug 7, 2024
Copy link

codecov bot commented Aug 7, 2024

Codecov Report

Attention: Patch coverage is 97.36842% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.01%. Comparing base (3e8092d) to head (80aa059).
Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
.../main/java/io/opentelemetry/sdk/trace/SdkSpan.java 96.42% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

@JonasKunz JonasKunz marked this pull request as ready for review August 7, 2024 11:15
@JonasKunz JonasKunz requested a review from a team August 7, 2024 11:15
this.endEpochNanos = endEpochNanos;
hasEnded = true;
if (spanProcessor.isOnEndingRequired()) {
spanProcessor.onEnding(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@JonasKunz JonasKunz Aug 13, 2024

Choose a reason for hiding this comment

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

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 invoking OnEnding of the first SpanProcessor. From that point on, modifications
are only allowed synchronously from within the invoked OnEnding callbacks.

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.

@JonasKunz JonasKunz changed the title Added SpanProcessor beforeEnd callback Added SpanProcessor OnEnding callback Aug 14, 2024
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.

Couple minor comments, but seems generally good. Thanks!

JonasKunz and others added 3 commits August 21, 2024 10:21
Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
@jkwatson
Copy link
Contributor

I approve of putting this in a separate interface. But, we should provide documentation on how someone could use this, as well.

@JonasKunz
Copy link
Contributor Author

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 ExtendedSpanProcessor or do we have some markdown files somewhere where this would fit in nicely?

@jack-berg
Copy link
Member

What do you think would be the best place for this? Just a javadoc in ExtendedSpanProcessor or do we have some markdown files somewhere where this would fit in nicely?

I think the ideal way to do this is a unit test. Let's add an ExtendedSpanProcessorUsageTest akin to what we do in the api incubator. No obvious place to link to the usage test at the moment, but let's start by at least demonstrating its usage in clear terms.

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.

thanks for adding the example. couple of nits but ready otherwise ready to merge IMO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants