ref(tracing): Include transaction in DSC if transaction source is not an unparameterized URL #5392
Conversation
… an unparameterized URL
packages/tracing/src/transaction.ts
Outdated
| const { segment: user_segment } = (scope && scope.getUser()) || {}; | ||
|
|
||
| const source = this.metadata.source; | ||
| const transaction = source && source !== 'url' && source !== 'unknown' ? this.name : undefined; |
There was a problem hiding this comment.
Was debating whether to include unknown in this condition. According to the spec Relay treats this value equally as transactions without a source. Which IMO means we shouldn't add the transaction name to the DSC.
Can remove it though if we agree that we never set unknown as a source value
There was a problem hiding this comment.
For now I think this logic is fine. I would even argue that we should remove 'unknown' from the TransactionSource type, as it's only a fallback value that relay uses for older SDKs, which our's isn't because we're always on the new version (🤔).
If we decide to remove 'unknown' from TransactionSource we should do that before the next release, because that's breaking.
There was a problem hiding this comment.
Yes that's a very good point. Thoughts about removing 'unknown' @AbhiPrasad?
size-limit report 📦
|
packages/tracing/src/transaction.ts
Outdated
| const { segment: user_segment } = (scope && scope.getUser()) || {}; | ||
|
|
||
| const source = this.metadata.source; | ||
| const transaction = source && source !== 'url' && source !== 'unknown' ? this.name : undefined; |
There was a problem hiding this comment.
For now I think this logic is fine. I would even argue that we should remove 'unknown' from the TransactionSource type, as it's only a fallback value that relay uses for older SDKs, which our's isn't because we're always on the new version (🤔).
If we decide to remove 'unknown' from TransactionSource we should do that before the next release, because that's breaking.
| @@ -162,8 +162,6 @@ export type TransactionSource = | |||
| | 'route' | |||
| /** Name of the view handling the request */ | |||
| | 'view' | |||
| /** This is the default value set by Relay for legacy SDKs. */ | |||
| | 'unknown' | |||
There was a problem hiding this comment.
This PR re-introduces the
transactionfield in the Dynamic Sampling Context (DSC). However, its presence is now determined by the transaction source which was introduced in #5367.As of this PR, we add the
transactionfield, if the source indicates that the tranasaction name is not an unparameterized URL.While making the changes, I decided to clean up the previously commented out references to user_id (from #5363) in both, source code and tests. Given that user_id was ditched from DS, we probably won't need them going forward.
Additionally, the PR (once again) adjusts our unit and integration tests to reflect this change. Repurposed some DSC<=>
sendDefaultPiitests that we previously skipped to now cover the transaction<=>transaction source dependence.Develop Spec is updated here: getsentry/develop#635
Ultimately, this PR also removes the
'unknown'field from theTransactionSourcetype because it is only used by Relay and SDKs shouldn't set it.resolves: #5383