Uh oh!
There was an error while loading. Please reload this page.
feat(tracing): Add transaction source field - #5367
Merged
Merged
Conversation
This patch adds `source` information to `Transaction`, which is typed by `TransactionSource` in `@sentry/tracing`. This helps track how the name of a transaction was determined, which will be used by the server for server-side controls. For now, we are placing the `source` field under transaction metadata. In the future, we can move this up into a top level API (an argument to `startTransaction` or `transaction.setSource`) if needed, but this should be fine to get us started. For next steps, after this patch gets merged, we will start going through various routing instrumentation frameworks and adding transaction source.
15 tasks
Contributor
size-limit report 📦
|
Lms24
approved these changes
Jul 6, 2022
Lms24
left a comment
Member
There was a problem hiding this comment.
LGTM overall. Had some questions.
One more question: We're using setMetadata for the time being to not expose Transaction.setSource as public API, correct?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
AbhiPrasad
commented
Jul 6, 2022
ContributorAuthor
Yes exactly! We can come back and change this later if needed. |
Lms24
commented
Jul 6, 2022
Member
Thanks for clarifying, makes sense to me! |
Member
FWIW, we don't need to do this, I don't think, because we can just do |
Lms24 added a commit
that referenced
this pull request
Jul 8, 2022
… an unparameterized URL (#5392) This patch re-introduces the `transaction` field in the Dynamic Sampling Context (DSC). However, its presence is now determined by the [transaction source](https://develop.sentry.dev/sdk/event-payloads/transaction/#transaction-annotations) which was introduced in #5367. As of this we we add the `transaction` field back, if the source indicates that the transaction name is not an unparameterized URL (meaning, the source is set and it is not `url`). Additionally, the PR (once again) adjusts our unit and integration tests to reflect this change. Repurposed some DSC<=>`sendDefaultPii` tests that we previously skipped to now cover the transaction<=>transaction source dependence. Did some cleanup of commented out old code and explanations that no longer apply. Remove he `'unknown'` field from the `TransactionSource` type because it is only used by Relay and SDKs shouldn't set it.
18 tasks
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
3 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.
Ref: #5345
This patch adds
sourceinformation toTransaction, which is typed byTransactionSourcein@sentry/tracing. This helps track how the name of a transaction was determined, which will be used by the server forserver-side controls.
For now, we are placing the
sourcefield under transaction metadata. In the future, we can move this up into a top level API (an argument tostartTransactionortransaction.setSource) if needed, but this should be fine to get us started.For next steps, after this patch gets merged, we will start going through various routing instrumentation frameworks and adding transaction source.