Uh oh!
There was an error while loading. Please reload this page.
feat(core): Deprecate Span.transaction in favor of getRootSpan - #10134
Merged
Conversation
Lms24
marked this pull request as ready for review
January 10, 2024 12:36
Contributor
size-limit report 📦
|
Span.tranasction in favor of getRootSpanSpan.transaction in favor of getRootSpanlforst
approved these changes
Jan 10, 2024
| this._trimEnd = transactionContext.trimEnd; | ||
| // this is because transactions are also spans, and spans have a transaction pointer | ||
| // TODO (v8): Replace this with another way to set the root span |
Contributor
There was a problem hiding this comment.
Is there even a todo for v8 here? Won't we get rid of the transaction class?
MemberAuthor
There was a problem hiding this comment.
Hmm I mean we somehow still need to construct a transaction for the event payload so I guess it's not gonna vanish completely. We'll just need some way to set and find the root span/txn of a span in v8.
Lms24force-pushed
the
lms/feat-code-getRootSpan
branch
2 times, most recently
from
January 10, 2024 17:16
4ffb363 to
f6ce2a9CompareLms24
enabled auto-merge (squash)
January 10, 2024 17:17
Lms24
disabled auto-merge
January 10, 2024 17:17
Lms24force-pushed
the
lms/feat-code-getRootSpan
branch
from
January 11, 2024 11:49
f6ce2a9 to
c2c20b1Compare
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR deprecates the
transactionfield on theSpaninterface and class. Instead, we'll store the span hierarchy in a different format external to the span class. In node-experimental, we use a WeakMap referencing the parent span as a data structure which we might need to generally do in v8. This, however, is a breaking change, so for now we deprecate the field but use it in the replacement utility function for v7.Replacing most reads of
span.transactionwas fairly easy - only setting the root span is still done on the deprecated field.