feat(core): Add beforeSendSpan hook (#11886)
#11918
Conversation
andreiborza
force-pushed
the
ab/before-send-span
branch
from
351ea5e to
3789160
Compare
size-limit report 📦
|
packages/core/src/envelope.ts
Outdated
| }; | ||
| const items = spans.map(span => createSpanEnvelopeItem(spanToJSON(span))); | ||
|
|
||
| const beforeSend = client && client.getOptions().beforeSendSpan; |
There was a problem hiding this comment.
| const beforeSend = client && client.getOptions().beforeSendSpan; | |
| const beforeSendSpan = client && client.getOptions().beforeSendSpan; |
let's call it that, I was confused for a sec because we also have an actual beforeSend hook (for error events) 😅
There was a problem hiding this comment.
Right, not sure why I named it that way 😅 ...
packages/core/src/envelope.ts
Outdated
| if (beforeSend) { | ||
| items = spans.map(span => createSpanEnvelopeItem(beforeSend(spanToJSON(span) as SpanJSON))); | ||
| } else { | ||
| items = spans.map(span => createSpanEnvelopeItem(spanToJSON(span))); | ||
| } |
There was a problem hiding this comment.
I'm not even sure if there is a difference, but feels like it could be a micro-bundlesize-improvement: What if we do this in a single loop, something like this:
const items = spans.map(span => {
const spanJson = spanToJSON(span) as SpanJSON;
return beforeSend ? beforeSend(spanJson) : spanJson;
});added benefit is that we save a let items that we need to re-assign 🤔 but no strong feelings, it probably doesn't matter either way.
There was a problem hiding this comment.
I was considering that but thought it wastes cycles checking the same thing over and over again. Not sure how big spans gets tho. Thoughts?
There was a problem hiding this comment.
yeah, you're probably right!
What about a combination of both:
const convertToSpanJson = beforeSendSpan
? (span: Span) => beforeSendSpan(spanToJSON(span) as SpanJSON))
: (span: Span) => spanToJSON(span) as SpanJSON;
const items = spans.map(convertToSpanJson);
andreiborza
force-pushed
the
ab/before-send-span
branch
3 times, most recently
from
a7df1ef to
ff1193c
Compare
packages/core/src/baseclient.ts
Outdated
| return beforeSendTransaction(event, hint); | ||
| if (isTransactionEvent(event)) { | ||
| if (event.spans && beforeSendSpan) { | ||
| event.spans = event.spans.map(span => beforeSendSpan(span)); |
There was a problem hiding this comment.
Just a note for ourselves: Wait with merging this until we verified with the performance team that this is OK, if this is undefined, for example.
andreiborza
force-pushed
the
ab/before-send-span
branch
from
222bcb0 to
8698a17
Compare
andreiborza
force-pushed
the
ab/before-send-span
branch
from
8698a17 to
bf687e4
Compare
packages/core/src/envelope.ts
Outdated
| * @returns A SpanEnvelope or null if all spans have been dropped | ||
| */ | ||
| export function createSpanEnvelope(spans: SentrySpan[]): SpanEnvelope { | ||
| export function createSpanEnvelope(spans: SentrySpan[], client?: Client): SpanEnvelope | null { |
There was a problem hiding this comment.
technically speaking this is breaking (as we export it from @sentry/core) 🤔 it is unlikely that a user is using this, but... hmm, not sure - maybe it is "safer" to still always return an envelope, and then check (outside) to only send this envelope if there are any spans inside of it?
There was a problem hiding this comment.
Good point, thanks!
I changed it to check the span items on the returned envelope before sending it and pulled everything down into the helper method for sending.
… span envelope based on items length
No description provided.