feat(core): Add captureSpan pipeline and helpers#19197
feat(core): Add captureSpan pipeline and helpers#19197Lms24 merged 9 commits intolms/feat-span-firstfrom
captureSpan pipeline and helpers#19197Conversation
Codecov Results 📊Generated by Codecov Action |
Codecov Results 📊✅ 25 passed | ⏭️ 5 skipped | Total: 30 | Pass Rate: 83.33% | Execution Time: 13.33s All tests are passing successfully. Generated by Codecov Action |
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| export { logSpanEnd, logSpanStart } from './logSpans'; | ||
|
|
||
| // Span Streaming | ||
| export { captureSpan } from './spans/captureSpan'; |
There was a problem hiding this comment.
Q: Would it make sense to call this captureStreamingSpan?
There was a problem hiding this comment.
Yup we can do that. No strong feelings tbh since users shouldn't interact with this at all.
There was a problem hiding this comment.
Actually, if you don't mind, I want to keep this in line with logs and metrics as much as possible. Given we don't use this outside of span first I think it's fine to leave it at "captureSpan"
| /** | ||
| * Register a callback for when a span JSON is processed, to add some data to the span JSON. | ||
| */ | ||
| public on(hook: 'processSpan', callback: (streamedSpanJSON: StreamedSpanJSON) => void): () => void; |
There was a problem hiding this comment.
Q: Also here - would it make sense to call this processStreamingSpan?
There was a problem hiding this comment.
Given that client hooks are not really deemed for public use and processStreamingSpan is slightly more bundle size (the hook names can't be minified), I'd tend towards keeping it short. There's also processLog and processMetric so we have a consistent pattern for the "new" telemetry items.
|
hmm I'm starting to wonder if the "StreamedSpan" naming pattern is a mistake to be honest. Prefixing every new API with "streamed" not only makes the names longer and increases bundle size more, but also means it's more annoying to migrate back to "Span" when we completely remove transactions. |
| status: getSimpleStatusMessage(this._status), | ||
| attributes: this._attributes, | ||
| // spread to avoid mutating the original object when later processing the span | ||
| attributes: { ...this._attributes }, |
There was a problem hiding this comment.
I'm wondering if this comes with big performance impacts. What cases are we protecting against?
There was a problem hiding this comment.
Basically this case:
const attributes = {'sentry.source': 'route'};
startSpan({name: 'span1', attributes}, () => {});
startSpan({name: 'span2', attributes}, () => {});If we don't clone the attributes, the second span would be started with all the mutations we applied to the attributes object of the first span during captureSpan. I agree there's for sure a perf impact but I don't see an alternative. Unless Object.keys or similar low level ops are more performant 🤔 (though these come with a bundle size tradeoff).
There was a problem hiding this comment.
Right, I wonder if people share attributes like that. Is that not an issue already today?
If it's already like this and we haven't gotten complaints, maybe we can just not clone and do it once someone complains? 😅
There was a problem hiding this comment.
I'd say it definitely is an issue today for regular spans, but us actively deleting sentry.source makes it a bit worse for span streaming. Actually, let's kill two birds with one stone here and:
- remove the deletion. We can just send both,
sentry.sourceandsentry.span.sourceattributes, I don't care 😅 - remove the spread and wait for the complaints
This PR adds the `captureSpan` pipeline, which takes a `Span` instance, processes it and ultimately returns a `SerializedStreamedSpan` which can then be enqueued into the span buffer. ref #17836
This PR adds the `captureSpan` pipeline, which takes a `Span` instance, processes it and ultimately returns a `SerializedStreamedSpan` which can then be enqueued into the span buffer. ref #17836
This PR adds the
captureSpanpipeline, which takes aSpaninstance, processes it and ultimately returns aSerializedStreamedSpanwhich can then be enqueued into the span buffer.ref #17836