-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(core): Add options to start standalone (segment) spans via start*Span
APIs
#11696
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
Conversation
4984b79
to
440bab4
Compare
start*Span
APIsstart*Span
APIs
start*Span
APIsstart*Span
APIs
size-limit report 📦
|
start*Span
APIsstart*Span
APIs
1e622f2
to
4c454d4
Compare
4c454d4
to
84d49ca
Compare
84d49ca
to
8b3627a
Compare
I simplified and updated the |
is_segment: this._isStandaloneSpan || undefined, | ||
segment_id: this._isStandaloneSpan ? this._spanId : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: I think this should be:
is_segment: this._isStandaloneSpan || undefined, | |
segment_id: this._isStandaloneSpan ? this._spanId : undefined, | |
is_segment: (this._isStandaloneSpan && getRootSpan(this) === this) || undefined, | |
segment_id: (this._isStandaloneSpan && getRootSpan(this) === this) ? this._spanId : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually wait no, it should be:
is_segment: this._isStandaloneSpan || undefined, | |
segment_id: this._isStandaloneSpan ? this._spanId : undefined, | |
is_segment: (this._isStandaloneSpan && getRootSpan(this) === this) || undefined, | |
segment_id: this._isStandaloneSpan ? getRootSpan(this).spanContext().spanId : undefined, |
as if we are sending (theoretically) a standalone span that is not the root span, then it should have the root span id as segment_id!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering when a standalone span is not a root (=== segment) span 🤔 Is this for span streaming?
Regardless, I'll make the change. For now, it should lead to the same outcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So as a result of the change, the one test that tests the hypothetical "standalone span while active span tree" scenario now shows that is_segment
would no longer be set and segment_id
is the id of the span tree's root span. Which I guess makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this would be span streaming then basically! So not really anything we'll do now, but "technically correct" 😅
I think the behavior for the test is correct then! The span would be sent on it's own, would not be a segment span, and the segment id would be the id of the root span! 👍
…1788) Refactors how we create INP spans. Previously we'd directly initialize a `SentrySpan` instance with the necessary properties and manually sample the span. Ironically, we heavily discourage directly initializing a `Span` class. With this change we leverage the `standalone: true` flag introduced in #11696. This way, we can also use the built-in sampling functionality and ensure that we only have one logic for creating standalone span envelopes. Also bundle size should decrease slightly 🤞 Adjusted the integration tests to more thoroughly test the INP span envelopes
Updated PR description after changing the API
This PR adds an experimental option to our
StartSpanOptions
API to allow sending a span as a standalone (segment) span via ourstart*Span
APIs:experimantal.standalone
: iftrue
, the span will be sent as a span envelope instead of a transaction event envelopeUsage example
For resulting envelopes, see integration tests in this PR
Limitations
-> This is fine for now; we'll only use standalone spans in a browser context at the moment
beforeSend*
hook or event processor.client.on('spanEnd')
however will be emitted just like for a regular span.trace
envelope header yet. Taking care of this in feat(core): Addtrace
envelope header to span envelope #11699Next up
Once we settled on the API/implementation I'm gonna opt our
http.client
spans into creating standalone spans if there's no active parent span. Then we can also change the INP span creation logic to simply rely onstartInactiveSpan
instead of manually sampling and creating the span.ref #11562