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

Send DSC on all envelopes #921

Merged
merged 1 commit into from
May 5, 2023
Merged

Send DSC on all envelopes #921

merged 1 commit into from
May 5, 2023

Conversation

bruno-garcia
Copy link
Member

@bruno-garcia bruno-garcia commented Apr 25, 2023

@vercel
Copy link

vercel bot commented Apr 25, 2023

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

Name Status Preview Comments Updated (UTC)
develop ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 25, 2023 11:36pm

@mattjohnsonpint
Copy link
Contributor

Cool.

Also, is dsc really still "experimental"? 🤔

@mattjohnsonpint
Copy link
Contributor

Also:

  • We don't (yet) have trace_id for errors, but it is still listed under "Strictly Required Values"

  • sample_rate is also listed as strictly required, but also in the envelope header section it is described as being the traces sample rate. For errors, should it be the errors sample rate? That's usually 100%, but could possibly be set lower. Does the back-end handle them interchangeably? Or perhaps for errors, this should be omitted?

@mattjohnsonpint
Copy link
Contributor

mattjohnsonpint commented Apr 26, 2023

Also, getsentry/sentry-javascript#7820 doesn't actually put DSC on all error events, only those error events that have an active transaction. Is that the intent for all SDKs, or is that wrong?

CC @AbhiPrasad

Copy link
Contributor

@mattjohnsonpint mattjohnsonpint left a comment

Choose a reason for hiding this comment

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

Let's hold on this until we get clarification on the previously mentioned points. I'll ask at TSC. Thanks.

@AbhiPrasad
Copy link
Member

getsentry/sentry-javascript#7820 was intentional - we don't have tracing without performance support in the JS SDK yet, so we can only extract DSC from from an active transaction (right now the transaction is the carrier of the DSC in all the SDKs)

When we add tracing without performance support, we'll start adding DSC to every single envelope, regardless of active transaction.

@bruno-garcia
Copy link
Member Author

We don't (yet) have trace_id for errors, but it is still listed under "Strictly Required Values"

Then don't send it. This is only when a transaction is happening (performance is a requirement)

sample_rate is also listed as strictly required, but also in the envelope header section it is described as being the traces sample rate. For errors, should it be the errors sample rate? That's usually 100%, but could possibly be set lower. Does the back-end handle them interchangeably? Or perhaps for errors, this should be omitted?

Send the transaction sample rate, nothing changes.

@mattjohnsonpint
Copy link
Contributor

Discussed at TSC and agreed. See getsentry/team-webplatform-meta#45 (comment)

So for now, this just needs to be updated to say that a transaction must be available.

@bruno-garcia
Copy link
Member Author

So for now, this just needs to be updated to say that a transaction must be available.

Right now a transaction must be available in the current implementation. But the goal is for that requirement not to exist. So I'll avoid mentioning this requirement here and the need to remove it later

@bruno-garcia bruno-garcia merged commit 6efe2cb into master May 5, 2023
@bruno-garcia bruno-garcia deleted the envelope/send-on-dsc branch May 5, 2023 18:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants