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
feat(node-experimental): Keep breadcrumbs on transaction #8967
Conversation
size-limit report 📦
|
9d97db3
to
2b49b16
Compare
This allows us to circumvent the scope/hub nesting issues we get with OTEL-backed hub forking.
2b49b16
to
5859e84
Compare
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.
Let's experiment with this, I'd be interested to see what the data looks like.
return transaction; | ||
} | ||
|
||
return new Proxy(transaction as TransactionWithBreadcrumbs, { |
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.
l: we can proxy patch startTransaction
(hub extension) so that we don't have to rely on getActiveTransaction
being used all the time.
Can skip this for now just to get it merged.
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.
Good idea, wrote something up here: #9010
This implements an experimental way to handle breadcrumbs for the POTEL world.
This patches stuff so that we get the breadcrumbs from the root transaction, instead of from the scope. This is how it works:
Scope.addBreadcrumb()
to fetch the current transaction, and if there is one, add breadcrumbs thereScope.getBreadcrumbs()
to fetch breadcrumbs from current transaction (if there is one)Proxy
to allow to store breadcrumbs on them._prepareEvent
on the experimental node Client to ensurecaptureContext
worksHow does that work with breadcrumbs in practice
This means that for each isolated request (=that creates its own transaction), we'll keep the breadcrumbs in there. So anything that happens inside of this branch will get all breadcrumbs from this transaction.
This works well 98% of the time, in my testing. For most scenarios, this resulted in the most correct (subjectively speaking) breadcrumbs for both transactions as well as errors.
Note that one scenario where this may not work as expected is with parallel
withScope()
calls, e.g.:In this scenario,
parallel error 2
will also showconsole.log(scope1-...)
.For me, this is an acceptable case for now.
On the other hand, here are some cases where it works as expected:
Overall, I think it works much better than the status quo of the POTEL-backed version.
TODO: What if no transactions?
With the current implementation, this means that if you do not enable tracing, you'll get "incorrect" breadcrumbs again (=the old behavior).
We actually always initialize OTEL (even if tracing is not enabled), but we do not setup any default performance integrations, thus do not generate any transactions by default etc. Probably OK for now (=it will still kind of work, but not great), something to think about later.