-
-
Notifications
You must be signed in to change notification settings - Fork 7
rfc(feature): Strict Trace Propagation #137
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
Draft
mydea
wants to merge
3
commits into
main
Choose a base branch
from
rfc/strict-trage-propagation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| - Start Date: 2024-07-05 | ||
| - RFC Type: feature | ||
| - RFC PR: https://github.com/getsentry/rfcs/pull/137 | ||
| - RFC Status: draft | ||
|
|
||
| # Summary | ||
|
|
||
| Today, Sentry SDKs will continue any trace they get via e.g. a `sentry-trace` header. However, it is possible to receive incoming traces from external organizations, in which case it is incorrect to continue the trace. | ||
|
|
||
| We should implement a way to avoid incorrectly continuing such a trace. The easiest way to do this is to restrict trace continuation to happen for the same Organization only. | ||
|
|
||
| # Motivation | ||
|
|
||
| Today, if some external service (e.g. Zappier) makes an API call to a customers API, and includes a `sentry-trace` header, the API will continue the trace, even if that belongs to a completely different Sentry Org/Project. This is incorrect and that trace _should not_ be continued, but instead a new trace should be started in this case. | ||
|
|
||
| # Background | ||
|
|
||
| This problem has existed forever basically, but now is the time to finally fix it. | ||
|
|
||
| # Supporting Data | ||
|
|
||
| Example issue where this happens: https://github.com/getsentry/sentry-javascript/discussions/12774 | ||
|
|
||
| # Options Considered | ||
|
|
||
| This RFC proposes the following Option (A): | ||
|
|
||
| ## Option A: (Optionally) pass the Org in baggage header & use it to restrict | ||
|
|
||
| We should add a new option to all Sentry SDKs, `org`. This optional option takes an org slug, similar to sentry-cli. For example: | ||
|
|
||
| ```js | ||
| Sentry.init({ | ||
| dsn: 'xxx', | ||
| org: 'sentry', | ||
| }); | ||
| ``` | ||
|
|
||
| When this option is set, it will propagate the org in the baggage entry as `sentry-org=sentry`. By default, nothing is done with this. | ||
|
|
||
| In addition to this, users can opt-into strict behavior by setting `strictTracePropagation: true` in their `init` code: | ||
|
|
||
| ```js | ||
| Sentry.init({ | ||
| dsn: 'xxx', | ||
| org: 'sentry', | ||
| strictTracePropagation: true, | ||
| }); | ||
| ``` | ||
|
|
||
| When this is enabled, the SDK should not continue any trace that does not continue a matching `sentry-org` baggage entry. | ||
|
|
||
| Clarifications on this: | ||
|
|
||
| * If `strictTracePropagation: true` is configured, but no `org` is defined, the SDK may warn. | ||
| * If `strictTracePropagation: true` is configured, but an incoming request has no baggage header at all (but it _has_ a `sentry-trace` header), it should _not_ be continued. | ||
|
|
||
| For now, this will be opt-in, but SDKs are free to switch the default to `true` in a major release. In onboarding for new projects, we should include `strictTracePropagation: true` by default. | ||
|
|
||
| In addition to allowing to manually configure the `org`, SDKs may also infer from other places. For example, if a bundler plugin is used we may infer it from there, if possible. | ||
|
|
||
| We may also adjust the DSN structure (which is currently being reworked anyhow) to also include the DSN. In this case, new DSNs may infer the org from there, ensuring that users do not need to manually set the `org` in addition to `dsn`. | ||
|
|
||
| ## Option B | ||
|
|
||
| Another option could be to configure something similar to `tracePropagationTargets` that leads to the SDK only continuing traces for allow-listed URLs. But this requires configuration by the user (and it may also change over time, ...). | ||
|
|
||
| # Drawbacks | ||
|
|
||
| This adds two new options to the SDKs, but they are both optional and opt-in. No current behavior will change for the time being, until a major (where possibly the default may change). | ||
|
|
||
| # Unresolved questions | ||
|
|
||
| - TODO | ||
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.
Uh oh!
There was an error while loading. Please reload this page.