fix(tracing): Allow unsampled transactions to be findable by getTransaction()
#2952
+39
−16
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lobsterkatie
changed the title
getTransaction()getTransaction()
lobsterkatie
force-pushed
the
kmclb-fix-get-transaction
branch
from
dc821bf to
4ae3af2
Compare
size-limit report
|
lobsterkatie
force-pushed
the
kmclb-fix-get-transaction
branch
from
4ae3af2 to
a4ba5ce
Compare
lobsterkatie
added a commit
that referenced
this pull request
Jan 12, 2022
…4394) In #2952, a new way of pulling the currently-active transaction off of the scope was introduced. That new way is no longer so new, and the legacy way was left in place at that time only out of an (over-)abundance of caution. It's time we removed it, both for ease of reading and for the few bytes we'll gain in bundle size reduction.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the
scope.getTransaction()method relies on the presence of a span recorder in order to retrieve the currently active transaction. (This is because we assume that the transaction holding the current span on the scope (which is likely the transaction itself, but could be one of its child spans) will always be in the 0th position in the span recorder, allowing us to grab it even if the span on the scope is one of its children.)The problem with that is that transactions with
sampled = false(and all of their child spans) are missing a span recorder. (Why store all those spans if you know already you're going to ditch them?) Therefore, under the current method, they can't be found.This PR adds a new pointer to each span, pointing at the transaction holding it. When considered as a span, the current transaction is also contained by itself, so its reference is circular. It then passes the pointer down to its children, and they to their children... you get the point. That way, every span can immediately get to its containing transaction, sampled or not.
It also changes the way
getTransaction()works, to use that pointer. (It falls back to the old behavior if the new way doesn't find anything. That said, if you comment out the old behavior, removing the fallback, all tests still pass.)As a bonus, this also fixes the bug of sampling decision not getting passed through the
sentry-traceheader if it happened to befalse. (Because we couldn't find an unsampled transaction, it became equivalent to there being no transaction at all, and no headers were sent.)