Skip to content

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

Merged
merged 8 commits into from
Apr 25, 2024

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Apr 19, 2024

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 our start*Span APIs:

  • experimantal.standalone: if true, the span will be sent as a span envelope instead of a transaction event envelope

Usage example

const fetchSpan = Sentry.startInactiveSpan({ 
  name: 'standalone_segment_span', 
  experimental: { 
    standalone: true,
  } 
}, () => {
// whatever ...
});

For resulting envelopes, see integration tests in this PR

Limitations

  • The two options have currently no effect on Otel-created spans (i.e. Node SDK and friends)
    -> This is fine for now; we'll only use standalone spans in a browser context at the moment
  • The spans go through our sampling logic, just like normal spans but they are not yet passed through any beforeSend* hook or event processor. client.on('spanEnd') however will be emitted just like for a regular span.
  • Standalone spans have no trace envelope header yet. Taking care of this in feat(core): Add trace envelope header to span envelope #11699

Next 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 on startInactiveSpan instead of manually sampling and creating the span.

ref #11562

@Lms24 Lms24 changed the base branch from develop to fn/inp-v8-port April 19, 2024 09:14
@Lms24 Lms24 self-assigned this Apr 19, 2024
@Lms24 Lms24 force-pushed the lms/feat-browser-standalone-segment-spans branch from 4984b79 to 440bab4 Compare April 19, 2024 09:30
@Lms24 Lms24 changed the title feat(browser): Start standalone (segment) spans via start*Span APIs feat(browser): Add Option to start standalone (segment) spans via start*Span APIs Apr 19, 2024
@Lms24 Lms24 changed the title feat(browser): Add Option to start standalone (segment) spans via start*Span APIs feat(browser): Add options to start standalone (segment) spans via start*Span APIs Apr 19, 2024
Copy link
Contributor

github-actions bot commented Apr 19, 2024

size-limit report 📦

Path Size
@sentry/browser 21.65 KB (0%)
@sentry/browser (incl. Tracing) 32.71 KB (+0.24% 🔺)
@sentry/browser (incl. Tracing, Replay) 68.09 KB (+0.17% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 61.47 KB (+0.17% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas) 72.13 KB (+0.16% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback) 84.34 KB (+0.13% 🔺)
@sentry/browser (incl. Feedback) 37.79 KB (0%)
@sentry/browser (incl. sendFeedback) 26.43 KB (0%)
@sentry/browser (incl. FeedbackAsync) 30.93 KB (0%)
@sentry/react 24.33 KB (0%)
@sentry/react (incl. Tracing) 35.66 KB (+0.37% 🔺)
@sentry/vue 25.45 KB (+0.73% 🔺)
@sentry/vue (incl. Tracing) 34.48 KB (+0.31% 🔺)
@sentry/svelte 21.77 KB (0%)
CDN Bundle 23.95 KB (0%)
CDN Bundle (incl. Tracing) 34.03 KB (+0.4% 🔺)
CDN Bundle (incl. Tracing, Replay) 67.71 KB (+0.16% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) 83.58 KB (+0.15% 🔺)
CDN Bundle - uncompressed 70.56 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed 100.99 KB (+0.43% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed 210.6 KB (+0.21% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 257 KB (+0.17% 🔺)
@sentry/nextjs (client) 34.88 KB (+0.38% 🔺)
@sentry/sveltekit (client) 33.26 KB (+0.39% 🔺)
@sentry/node 138.48 KB (+0.01% 🔺)

@Lms24 Lms24 changed the title feat(browser): Add options to start standalone (segment) spans via start*Span APIs feat(core): Add options to start standalone (segment) spans via start*Span APIs Apr 19, 2024
@Lms24 Lms24 force-pushed the lms/feat-browser-standalone-segment-spans branch from 1e622f2 to 4c454d4 Compare April 19, 2024 13:08
Base automatically changed from fn/inp-v8-port to develop April 22, 2024 13:28
@Lms24 Lms24 force-pushed the lms/feat-browser-standalone-segment-spans branch from 4c454d4 to 84d49ca Compare April 23, 2024 08:48
@Lms24 Lms24 marked this pull request as ready for review April 23, 2024 11:41
@Lms24 Lms24 requested review from a team, AbhiPrasad, s1gr1d, mydea and lforst and removed request for a team April 23, 2024 11:42
Lms24 added 3 commits April 23, 2024 13:59
separate standalone and segment

tmp add test helper (no need after rebase)

add tests
@Lms24 Lms24 force-pushed the lms/feat-browser-standalone-segment-spans branch from 84d49ca to 8b3627a Compare April 23, 2024 11:59
@Lms24
Copy link
Member Author

Lms24 commented Apr 24, 2024

I simplified and updated the StartSpanOptions API options to just standalone as discussed with @mydea. Also took care of filtering out standalone spans in a potentially existing parent span tree.

Comment on lines 198 to 199
is_segment: this._isStandaloneSpan || undefined,
segment_id: this._isStandaloneSpan ? this._spanId : undefined,
Copy link
Member

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:

Suggested change
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,

Copy link
Member

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:

Suggested change
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!

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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! 👍

@Lms24 Lms24 requested a review from mydea April 25, 2024 13:54
@Lms24 Lms24 merged commit 84e9c97 into develop Apr 25, 2024
94 checks passed
@Lms24 Lms24 deleted the lms/feat-browser-standalone-segment-spans branch April 25, 2024 14:44
Lms24 added a commit that referenced this pull request Apr 25, 2024
…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
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.

2 participants