SDKs Need to Forward DSC on error events

[JoshFerge]

picking up from #41 -- the requirements specified for DSC forwarding do not seem to be implemented, at least for node.js.
#41 (comment)

Perhaps something was lost in the ether here:
From my comment above
the relevant backend SDKs need to forward the DSC along with errors if performance is enabled.
I just tried propagating the replay_id on the node.js express backend, and we are not forwarding the DSC along with error events.
It appears we only add the DSC to envelopes if the event.type is transaction
https://github.com/getsentry/sentry-javascript/blob/88d94aff7cbe3cca07de76b4b8df7f971674be23/packages/utils/src/envelope.ts#L245
re-opening.
(on a sidenote, i can confirm i'm seeing replay_ids along with backend transactions. nice!)

[mattjohnsonpint]

Hi. When you say requirements, what specifically are you referring to? The only requirements I know of for DSC are the dev docs, which describe DSC specifically with regard to performance, and only for transactions. If SDKs need to start sending DSC for errors, we need to design for that and update the spec. Are other SDKs already doing this?

(Also, is DSC really still experimental? Or have we just not updated the spec?)

Thanks.

[JoshFerge]

I suppose that this issue getsentry/team-sdks#5 (couldn't find this friday) is perhaps the solution here. I think this idea seems cleaner as its less changes, but if a bunch of breaking changes are needed anyways 🤷🏼. @HazAT

[cleptric]

@JoshFerge The Python, PHP and Go SDKs still use the /store endpoint for error events. This means we can not attach a trace (DSC) envelope header to them today.

[AbhiPrasad]

Node fix: getsentry/sentry-javascript#7819

[bruno-garcia]

This change is about adding the DSC info on envelopes sending errors. As opposed to only doing it on transactions.
It doesn't require traces outside transactions. For SDKs that already use the /envelope endpoint, it's a small change. See @AbhiPrasad's PR on NodeJS as a reference (+24 -17 lines):

https://github.com/getsentry/sentry-javascript/pull/7820/files

[mattjohnsonpint]

Ok. So this can be done independently, but is a prerequisite for completing getsentry/team-sdks#5. Got it.

[mattjohnsonpint]

Somebody should still update the dev docs to indicate that DSC is for both errors and transactions, instead of just transactions.

[mattjohnsonpint]

For .NET, see getsentry/sentry-dotnet#2330

[mattjohnsonpint]

There are still some unknowns to figure out before we can move forward:

1. Adding the DSC to all envelope headers sentry-javascript#7819 adds DSC to error events only if there's an active transaction, then uses that transaction to create the DSC. It's unclear what we would do if there's not an active transaction.
2. trace_id is currently "strictly required" by the dev docs, which we won't have if there's not an active transaction, until Project: Tracing without Performance team-sdks#5 is implemented. Should it be omitted if there's no transaction? And if so, is it still ok that the envelope header is named trace?
3. sample_rate is also "strictly required", and is the traces sample rate. It's unclear whether in the case of an error without a transaction if that should be omitted, or should it be the errors sample rate, or should the errors sample rate be placed in a different field as to not conflate with the traces sample rate?

If the intent is that DSC is added to error events only when there's an active transaction, we should clarify that.

[mattjohnsonpint]

This was discussed at Client Infra TSC today. The consensus was:

• For now, DSC will not be sent on all error events, but rather it will be sent on error events that occur when there is an active transaction.
• The trace_id, sample_rate, transaction, etc. will flow from the transaction, in the same way that it would for a transaction event.
• Later, when Project: Tracing without Performance team-sdks#5 is implemented, we'll be able to sent DSC for error events when a transaction is not present. In that case, the transaction_name would be omitted. It's still TBD what to do with sample_rate for that case.

[cleptric]

It's still TBD what to do with sample_rate for that case.

We omit it.

[stephanie-anderson]

This has been handled with Tracing without Performance.