-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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(tracing): Add context update methods to Span and Transaction #3192
Conversation
size-limit report
|
@@ -14,7 +14,7 @@ export class Transaction extends SpanClass implements TransactionInterface { | |||
*/ | |||
private readonly _hub: Hub = (getCurrentHub() as unknown) as Hub; | |||
|
|||
private readonly _trimEnd?: boolean; | |||
private _trimEnd?: boolean; |
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.
Since this is kind of an internal bit, is there a reason we'd want people to be able to muck with it? Same goes for traceId
, parentSpanId
, and the two timestamps. It seems like we could dump/update a subset of the properties rather than all of them, and then we wouldn't need this change.
(To be fair, this same question applies to the original beforeNavigate
- perhaps we should only pass a partial context there.)
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 agree that a user shouldn't mess with these but I think the reason they are exposed, and that we should expose them (but not document them) for downstream SDKs like React Native and Capacitor to use.
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 guess it depends on if there's a use case for toContext
beyond user-generated functions like beforeNavigate
. If wrapper SDKs use them themselves, then yeah, okay, but there's also no reason they can't modify the object directly, is there? I thought the only reason to do this was because we're trying to match beforeNavigate
's signature. No?
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.
That is true, the initial reason that I came to this was to mimic beforeNavigate
and the RN SDK could not set _trimEnd
as it was private.
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.
Then I'd vote for:
- Do whatever you need to to
_trimEnd
to make the RN SDK able to do its job. - Separately, have the methods above dump and read in only a subset of the data (whatever it makes sense to have a user touch in
beforeNavigate
). It occurs to me this also maybe could be used in what's passed totracesSampler
.
None of this is that critical, though, so your call.
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.
Hmm so I think the updateWithContext
methods in this PR should be left as is, and the dumping should be done by beforeNavigate
and tracesSampler
separately
what's missing to get this PR merged/released? it's currently a blocker for the RN SDK. |
Description
Adds
toContext
andupdateWithContext
to spans and transactions. These methods allow you to convert a span/transaction instance back into a context object, and then update it with a new context object.Note: In this case
_trimEnd
on transaction is no longer read-only. This means ifupdateWithContext
changestrimEnd
after a transaction has been finished it would not be considered, would this be an issue?Motivation
There is a use case for this in Performance for React Native where the instrumentation would call a method that would have the same outward facing API as
beforeNavigate
on JS,(context: TransactionContext) => TransactionContext
after a transaction has been created due to a limitation with our implementation to instrument React Navigation V5.yarn lint
) & (yarn test
).