Skip to content

feat(develop): Add Tracing without Performance documentation #11441

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Nov 6, 2024
Merged

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Sep 26, 2024

This PR adds a develop docs page to our SDK->Telemetry->Tracing section for "Tracing without Performance" (TwP). TWP has been around for some time and most (all?) SDKs support it today. However, we didn't specify the behaviour in develop docs yet. This page specifies the expected high-level behaviour on continuing and propagating traces in TwP as well as attaching trace data to events and envelopes.

This specification is based on the previously existing internal Notion doc as well as some discussions we recently had around TwP and its implications.

Page Preview

Copy link

vercel bot commented Sep 26, 2024

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

Name Status Preview Comments Updated (UTC)
changelog ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 6, 2024 0:22am
develop-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 6, 2024 0:22am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
sentry-docs ⬜️ Ignored (Inspect) Visit Preview Nov 6, 2024 0:22am

@Lms24 Lms24 changed the title feat: Add Tracing without Performance documentation feat(develop): Add Tracing without Performance documentation Sep 26, 2024
@Lms24 Lms24 requested review from mydea and cleptric September 26, 2024 12:35
@Lms24 Lms24 self-assigned this Sep 26, 2024
@lizokm lizokm self-requested a review September 26, 2024 16:06
Copy link
Contributor

@lizokm lizokm left a comment

Choose a reason for hiding this comment

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

Thanks for writing these!!


</Alert>

### Upgrading to Tracing With Performance
Copy link
Member

Choose a reason for hiding this comment

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

We should not call it like this, maybe just enable tracing or smth.

Comment on lines 63 to 70
To completely opt out of any distributed tracing capabilities, users can pass an empty array (or language-equivalent parameter) to the `tracePropagationTargets` option. This will prevent the SDK from propagating trace information any further.

```js
Sentry.init({
dsn: "___PUBLIC_DSN___",
tracePropagationTargets: [],
});
```
Copy link
Member

Choose a reason for hiding this comment

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

Might want to call out tracesSampleRate: null, too?

Copy link
Member Author

Choose a reason for hiding this comment

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

What should tracesSampleRate: null do? At least in JS this isn't a valid value. If we set tracesSampleRate: undefined TwP is still active.

Copy link
Member

Choose a reason for hiding this comment

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

So null/nil/undefined all has the same behaviour. It does no longer take trace parent headers with -1 into account at all.

Copy link
Member Author

@Lms24 Lms24 Oct 31, 2024

Choose a reason for hiding this comment

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

Sorry but I'm a bit confused:

This section is about disabling tracing without performance, i.e. no longer propagating a trace. As far as I can tell, this is only the case when setting tracePropagationTargets: [].

It does no longer take trace parent headers with -1 into account at all

What does this mean specifically? If an incoming SDK receives a header with -1, and its tracesSampleRate is null|undefined|nil, should the SDK not continue the positive sampling decision? Maybe I'm missing something obvious but I currently don't see this behaviour in the SDK code.

To further add to the confusion, we would like to ensure that in the future, setting tracesSampleRate: undefined behaves in the same way as not setting tracesSampleRate at all. It's a bit intransparent to users to understand that there is difference while in both cases querying options.tracesSamplerate would return undefined.

Copy link
Member

Choose a reason for hiding this comment

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

If this entire page only focuses on TwP, then the section header (Completely Disabling Tracing) confuses me 😬

What does this mean specifically? If an incoming SDK receives a header with -1, and its tracesSampleRate is null|undefined|nil, should the SDK not continue the positive sampling decision?

Yes, this is how it should work today.

We'll properly fix this when we re-work all SDKs towards only spans and the new sampling. tracesSampleRate will only be a float between 0.0 and 1.0.

Copy link
Member Author

@Lms24 Lms24 Nov 4, 2024

Choose a reason for hiding this comment

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

then the section header (Completely Disabling Tracing) confuses me

Fair but I think it's important to specify how you opt out of TwP. There are two options:

  1. Enable regular tracing (with spans) - i.e. specify tracesSampleRate: number and/or tracesSampler.
  2. Completely disabling tracing. Should we rename the section? I was also thinking about "Disabling Tracing without Performance" but this could also entail 1... Open for better suggestions!

Yes, this is how it should work today.

Okay, so I think we're talking about 2 things:

  • Assuming no other tracing options (tracesSampler/enableTracing) are set: TwP should be enabled if tracesSampleRate is not set at all or if tracesSampleRate is set to undefined/null/nil.
    I think that makes sense to me and is what we want to achieve in v9 of the JS SDK anyway. Today, there's a behavioural difference between setting tracesSampleRate: undefined and not setting it at all. But this shouldn't affect the develop spec.

  • Separately to this, we should document that SDKs in TwP mode, that receive incoming traces with -1 (and -0 as well?), should not forward the sampling decision, meaning, propagate sentry-trace with a deferred sampling decision. This should go under another section though because it isn't connected to Disabling TwP.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed in the call yesterday:

  • Removed the "completely disable tracing" section in favour of "disable trace propagation"
  • Noted explicitly that at this time, SDKs will always continue incoming traces in TwP mode. The only way to opt out of this would be to manually start a new trace (but this is admittedly a niche edge case, given that users would probably only do this if they'd also like to send spans).
  • Realized I already added a "historical context" section at the bottom but I now added a link on the top to it. Slightly reworked the historical context section though.

Lms24 and others added 14 commits October 31, 2024 16:12
Copy link
Member

@cleptric cleptric left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

@Lms24 Lms24 merged commit 6521cb9 into master Nov 6, 2024
10 checks passed
@Lms24 Lms24 deleted the lms/feat-twp branch November 6, 2024 12:33
@github-actions github-actions bot locked and limited conversation to collaborators Nov 22, 2024
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