Skip to content
This repository was archived by the owner on Aug 14, 2024. It is now read-only.

Add transaction back to DSC specification#635

Merged
lforst merged 5 commits intomasterfrom
lforst-dsc-transaction-name-quality
Jul 8, 2022
Merged

Add transaction back to DSC specification#635
lforst merged 5 commits intomasterfrom
lforst-dsc-transaction-name-quality

Conversation

@lforst
Copy link
Contributor

@lforst lforst commented Jul 8, 2022

Resolves #634

@lforst lforst requested review from AbhiPrasad and Lms24 July 8, 2022 10:11
@lforst lforst self-assigned this Jul 8, 2022
@vercel
Copy link

vercel bot commented Jul 8, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
develop ✅ Ready (Inspect) Visit Preview Jul 8, 2022 at 0:57AM (UTC)

Lms24
Lms24 approved these changes Jul 8, 2022
View reviewed changes
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good general description for "good quality" transaction names.

Wondering if the dependence for transaction being added to the DSC on the transaction source should be a hard requirement instead of a suggestion. But as long as Relay doesn't make sampling decisions based on the source (which IIRC and according to the spec) I think we can leave this open to SDK implementors.

@lforst @Lms24
Fix typo and denglish
03be687 
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@lforst
Copy link
Contributor Author

lforst commented Jul 8, 2022 •
edited
Loading

Wondering if the dependence for transaction being added to the DSC on the transaction source should be a hard requirement instead of a suggestion. But as long as Relay doesn't make sampling decisions based on the source (which IIRC and according to the spec) I think we can leave this open to SDK implementors.

Good input!

The reason I wrote it like this and also didn't make it a hard-requirement to have the transaction on the DSC based on transaction source, is because not all SDKs have the problem of "low quality transaction names" like the JS SDK with URLs and additionally not all SDKs will have to provide a transaction source until relay makes it required.
In my head, transaction source and inclusion of TXs on DSC are two different concepts, but the one can support the decision making of the other one.

@Lms24
Copy link
Member

Lms24 commented Jul 8, 2022

Yeah, makes sense to leave it as is for now

jjbayer
jjbayer reviewed Jul 8, 2022
View reviewed changes
@marandaneto marandaneto mentioned this pull request Jul 8, 2022
Closed
6 tasks
@Lms24 Lms24 mentioned this pull request Jul 8, 2022
Merged
@lforst @jjbayer
Add suggestion for identifiers
69d0f83 
Co-authored-by: Joris Bayer <joris.bayer@sentry.io>
romtsn
romtsn reviewed Jul 8, 2022
View reviewed changes
romtsn
romtsn reviewed Jul 8, 2022
View reviewed changes
@marandaneto marandaneto mentioned this pull request Jul 8, 2022
Merged
4 tasks
lforst and others added 2 commits July 8, 2022 14:54
@lforst @romtsn
s/iff/if/
46107ff 
Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>
@lforst @romtsn
s/iff/if/
eeff8b3 
Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>
@lforst lforst merged commit f005182 into master Jul 8, 2022
@lforst lforst deleted the lforst-dsc-transaction-name-quality branch July 8, 2022 14:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AbhiPrasad AbhiPrasad Awaiting requested review from AbhiPrasad

4 more reviewers

@jjbayer jjbayer jjbayer left review comments

@romtsn romtsn romtsn left review comments

@untitaker untitaker untitaker approved these changes

@Lms24 Lms24 Lms24 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lforst lforst

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Including transaction field in Dynamic Sampling Context must depend on transaction source

5 participants

@lforst @Lms24 @untitaker @jjbayer @romtsn