Uh oh!
There was an error while loading. Please reload this page.
feat(tracing): Add context update methods to Span and Transaction - #3192
Conversation
size-limit report
|
Uh oh!
There was an error while loading. Please reload this page.
| private readonly _hub: Hub = (getCurrentHub() as unknown) as Hub; | ||
| private readonly _trimEnd?: boolean; | ||
| private _trimEnd?: boolean; |
There was a problem hiding this comment.
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.
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.
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.
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.
Then I'd vote for:
- Do whatever you need to to
_trimEndto 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.
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
marandaneto
commented
Jan 29, 2021
what's missing to get this PR merged/released? it's currently a blocker for the RN SDK. |
Description
Adds
toContextandupdateWithContextto 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
_trimEndon transaction is no longer read-only. This means ifupdateWithContextchangestrimEndafter 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
beforeNavigateon JS,(context: TransactionContext) => TransactionContextafter a transaction has been created due to a limitation with our implementation to instrument React Navigation V5.yarn lint) & (yarn test).