ref(tracing): Remove transaction name and user_id from DSC#5363
Merged
ref(tracing): Remove transaction name and user_id from DSC#5363
Conversation
temporary fix to no longer propagate potential PII
Lms24
commented
Jul 5, 2022
packages/integration-tests/suites/tracing/envelope-header-no-pii/test.ts
Outdated
Show resolved
Hide resolved
Contributor
size-limit report 📦
|
lobsterkatie
reviewed
Jul 5, 2022
packages/integration-tests/suites/tracing/envelope-header-no-pii/test.ts
Outdated
Show resolved
Hide resolved
| baggage: expect.stringContaining( | ||
| 'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' + | ||
| 'sentry-public_key=public,sentry-trace_id=', | ||
| 'sentry-environment=prod,sentry-release=1.0,sentry-public_key=public,sentry-trace_id=', |
Member
There was a problem hiding this comment.
Tiny tiny nit: If we're commenting stuff out in the other tests rather than modifying said stuff, I'd leave the old value here, commented out (in addition to the new value, of course).
(Same in the other spots where this applies.)
Comment on lines
+6
to
+7
| // Skipping this test because right now we're not including user_id at all | ||
| test.skip('Includes user_id in baggage if sendDefaultPii is set to true', async () => { |
Member
There was a problem hiding this comment.
Since, as discussed in the meeting earlier, we're not going to be using sendDefaultPII to control this (now or ever), we should pull it from any tests it's in.
We can leave this test, maybe, since something will eventually control that, but if so I'd probably do something like
Suggested change
| // Skipping this test because right now we're not including user_id at all | |
| test.skip('Includes user_id in baggage if sendDefaultPii is set to true', async () => { | |
| // TODO: Skipping this test because right now we're rethinking the mechanism for including such data | |
| test.skip('Includes user_id in baggage if <optionTBA> is set to true', async () => { |
and add a similar TODO to the companion file which sets up the test.
packages/tracing/src/transaction.ts
Outdated
| transaction: this.name, | ||
| ...(hub.shouldSendDefaultPii() && { user_id }), | ||
| // transaction: this.name, | ||
| // ...(hub.shouldSendDefaultPii() && { user_id }), |
Member
There was a problem hiding this comment.
See above re: sendDefaultPII.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This patch temporarily removes the
user_idandtransaction(name) fields from the dynamic sampling context, meaning they will no longer propagated with outgoing requests via the baggage Http header or sent to sentry via thetraceenvelope header.We're taking this temporary measure to ensure that for the moment PII is not sent to third parties. Developer spec is update in getsentry/develop#631.