feat(core): Add afterEnvelope and flushTraceSpans client hooks - #23136
feat(core): Add afterEnvelope and flushTraceSpans client hooks#23136JPeer264 wants to merge 1 commit into
afterEnvelope and flushTraceSpans client hooks#23136Conversation
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
I briefly asked myself, instead of only flushing the span buffer, should we simply call Sentry.flush? Does anything speak against flushing everything in the queue at this point? Just a question. "Yes" is a perfectly fine answer :)
|
|
||
| // Lets runtimes flush a single trace eagerly (e.g. the Cloudflare SDK draining | ||
| // a trace the moment its segment ends), without exposing the buffer itself. | ||
| client.on('flushTraceSpans', traceId => { |
There was a problem hiding this comment.
this is fine since browser ships its own spanStreamingIntegration and thus we don't bloat a browser bundle with this logic.
There was a problem hiding this comment.
Counter question. Is it safe to flush at any point in time?
FWIW I'm currently testing if we can get rid of this option actually.
There was a problem hiding this comment.
@Lms24 just tried to move it over to Sentry.flush directly and it seems under load on deployed workers only half of the traces were received.
After putting some thoughts into it, it might be even bad (even if it would work) to run flush for all traces, as the flush is bound per request, which means one request could potentially suffer performance wise.
size-limit report 📦
|
| try { | ||
| return await this._transport.send(envelope); | ||
| const result = await this._transport.send(envelope); | ||
| this.emit('afterEnvelope', envelope); |
There was a problem hiding this comment.
I'm not fully sure why we need this hook yet. Is the idea that we basically do
cloudflareClient.on('afterEnvelope', () => {
// potentially with a setTimeout for a grace period or something similar
cloudflareClient.emit('flushTraceSpans', traceId);
})?
There was a problem hiding this comment.
Yes exactly. No sure if there is a better solution for this
|
Ah I guess the concern is flushing other concurrent traces at the same time? Actually this is fair because it would unnecessarily strain network traffic. And add more perf overhead if other telemetry gets flushed, too. |
`afterEnvelope` fires after the transport has accepted an envelope. Runtimes that flush eagerly (e.g. the Cloudflare SDK's cached client) use it to drain the transport buffer at capture time instead of waiting for an invocation boundary. `flushTraceSpans` lets a runtime flush a single trace's span bucket from the span streaming buffer ahead of the regular flush points, without draining the whole buffer mid-invocation and fragmenting concurrent traces. Both are no-ops for runtimes that never emit them.
0d559d6 to
0a80fe6
Compare
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
thx for answering my questions, sounds good to me, let's 
2 participants
This adds hooks that are required for #23083. We need a way to listen to specific events that are happening after responses. The
afterEnvelope, gives us the missing hook for envelopes andflushTraceSpansadds the functionality to flush a trace on demand. These hooks are only required for Cloudfare. I tried to make its ownspanStreamingIntegrationonly for the Cloudflare SDK, but it wouldn't have worked without duplicating the entire code, becauseflushTraceSpansneeds access tobufferdirectly.Clanker desc:
afterEnvelopefires after the transport has accepted an envelope. Runtimes that flush eagerly (e.g. the Cloudflare SDK's cached client) use it to drain the transport buffer at capture time instead of waiting for an invocation boundary.flushTraceSpanslets a runtime flush a single trace's span bucket from the span streaming buffer ahead of the regular flush points, without draining the whole buffer mid-invocation and fragmenting concurrent traces.