-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(core): Add afterEnvelope and flushTraceSpans client hooks
#23136
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,12 @@ export const spanStreamingIntegration = defineIntegration(() => { | |
| } | ||
| buffer.add(captureSpan(span, client)); | ||
| }); | ||
|
|
||
| // 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 => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is fine since browser ships its own
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Lms24 just tried to move it over to 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. |
||
| buffer.flush(traceId); | ||
| }); | ||
| }, | ||
| }; | ||
| }) satisfies IntegrationFn; | ||
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.
I'm not fully sure why we need this hook yet. Is the idea that we basically do
?
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.
Yes exactly. No sure if there is a better solution for this