fix(core): Normalize trace context #5171

lforst · 2022-05-27T10:01:58Z

Trace context is currently not normalized before being sent, however the data field in trace context is "free form" and user provided, so we need to at least normalize that. We're also fixing a bug in dropUndefinedKeys where references to the original object were kept in the "cloned"/returned object.

Most likely resolves #2809

Ref: https://getsentry.atlassian.net/browse/WEB-940

This is probably not the cleanest solution. I evaluated doing the normalization a bit deeper in the stack, but ran into problems with not having the normalizeDepth options available and more importantly trace context data being mutated again after the normalization.

github-actions · 2022-05-27T10:09:58Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.43 KB (-3.53% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)	60.2 KB (-6.83% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.24 KB (-3.32% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	53.82 KB (-7.15% 🔽)
@sentry/browser - Webpack (gzipped + minified)	19.99 KB (-13.96% 🔽)
@sentry/browser - Webpack (minified)	65.19 KB (-20.22% 🔽)
@sentry/react - Webpack (gzipped + minified)	20.02 KB (-14.01% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified)	43.76 KB (-8.93% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.46 KB (-2.38% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	23.98 KB (-2.04% 🔽)

vladanpaunovic

Normalizing data looks good to me.

I am afraid that this won't resolve the actual issue as in my test app, I am already normalizing the data and still getting the same error.

lforst · 2022-05-30T08:08:35Z

I am afraid that this won't resolve the actual issue as in my test app, I am already normalizing the data and still getting the same error.

That would suck. IIRC we identified the error happening in axio's non-error callback. Can you try normalizing the response object here before passing it to startChild? If that doesn't resolve the issue we need to keep investigating.

Edit: Discussed the issue with @vladanpaunovic in person and wrapping the response in normalize resolved the issue. This makes us confident that this change will indeed fix the underlying issue.

lobsterkatie

This looks good!

lobsterkatie · 2022-06-01T06:58:31Z

packages/core/src/baseclient.ts

+    // event.spans[].data may contain circular/dangerous data so we need to normalize it
+    if (event.spans) {
+      normalized.spans = event.spans.map(span => {
+        // We cannot use the spread operator on span here because that overwrites the `toJSON` method


Is this because the toJSON method is non-enumerable, and spread only copies over enumerable properties? If so, can we change the comment to say that?

Yup, good point, thanks! --> 50b4b33

packages/integration-tests/suites/public-api/startTransaction/circular_data/subject.js

lobsterkatie · 2022-06-01T07:01:24Z

packages/integration-tests/suites/public-api/startTransaction/circular_data/test.ts

+
+  expect(eventData.contexts).toMatchObject({
+    trace: {
+      data: { lays: { contains: { lays: { contains: '[Circular ~]' } } } },


Why does this go two layers deep before noticing the circularity?

When I first wrote the test I didn't really care but now I looked a little bit closer and noticed that this is actually a pretty bad bug within dropUndefinedKeys which I produced in #5163.

I adjusted tests to test for the faulty behaviour (06a2b2d) and probably fixed it (3223048). Sadly it's not very straight forward. May I ask you to take another look at the changes? Sorry about the confusion.

For future readers. This was now done in #5201

AbhiPrasad · 2022-06-02T13:12:48Z

packages/core/src/baseclient.ts

+    }
+
+    // event.spans[].data may contain circular/dangerous data so we need to normalize it
+    if (event.spans) {


should we gate this with a tracing debug flag?

I wouldn't do that. The tracing flag currently only guards code that sets up automatic instrumentation, i.e. side effect code that makes use of tracing. This allows tracing to be treeshaken if it's not in use. It can still be used, even if the flag is set to false. If we guard this here and users call startTransaction and pass in circular data, they're not gonna have a good time. Good consideration though.

Makes sense!

AbhiPrasad · 2022-06-02T13:20:57Z

packages/utils/src/object.ts

@@ -4,7 +4,6 @@ import { WrappedFunction } from '@sentry/types';

 import { htmlTreeAsString } from './browser';
 import { isElement, isError, isEvent, isInstanceOf, isPlainObject, isPrimitive } from './is';
-import { memoBuilder, MemoFunc } from './memo';


maybe we can eliminate this since it's only used in 2 places? Can do in another PR

We might be able to. Agree that when we get to it we should do this in another PR.

timfish · 2022-06-16T16:38:50Z

It looks like this change completely clears out other contexts if contexts.trace exists:

sentry-javascript/packages/core/src/baseclient.ts

Lines 499 to 501 in f4e3cf4

    
           if (event.contexts && event.contexts.trace) { 
        
             normalized.contexts = {}; 
        
             normalized.contexts.trace = event.contexts.trace;

Running the Electron e2e tests after this change and all the device/os/etc contexts are stripped out. Is this intensional?

lobsterkatie · 2022-06-16T16:51:26Z

No, and I am actually in the process of fixing it for other reasons. Stay tuned!

lforst · 2022-06-16T20:12:30Z

It looks like this change completely clears out other contexts if contexts.trace

Oh god. Looking at this is painful. Stupid mistake, sorry about that. Btw it appears #5218 also aims to fix this (?).

lforst mentioned this pull request May 27, 2022

React Navigation routing performance tracking: 'Maximum call stack size exceeded' in the /utils library #5172

Closed

3 tasks

lforst marked this pull request as ready for review May 27, 2022 15:04

vladanpaunovic approved these changes May 30, 2022

View reviewed changes

lforst changed the base branch from 7.x to master May 30, 2022 13:31

lforst force-pushed the lforst-fix-unnormalized-tracecontext branch from 29f0932 to 17af3a0 Compare May 30, 2022 13:32

lobsterkatie approved these changes Jun 1, 2022

View reviewed changes

lforst requested a review from lobsterkatie June 1, 2022 09:59

AbhiPrasad approved these changes Jun 2, 2022

View reviewed changes

lforst mentioned this pull request Jun 3, 2022

fix(utils): Fix faulty references in dropUndefinedKeys #5201

Merged

lforst added 8 commits June 3, 2022 11:53

fix(core): Normalize trace context

f51fe68

Normalize free form data in spans

6720cca

Add integration tests

37691cd

Fix tests

2ff6337

Add documentation

febd6cd

Fix bug in dropUndefinedKeys that kept references to original data

7ec680f

Undo changes to dropUndefinedKeys

0256842

Fix rebasing stuff

4d6a919

lforst force-pushed the lforst-fix-unnormalized-tracecontext branch from 6710bac to 4d6a919 Compare June 3, 2022 12:00

lforst merged commit 453b7ad into master Jun 3, 2022

lforst deleted the lforst-fix-unnormalized-tracecontext branch June 3, 2022 12:16

Lms24 mentioned this pull request Jun 15, 2022

ref(tracing): Sync baggage data in Http and envelope headers #5218

Merged

sacummings91 mentioned this pull request Jun 16, 2022

Context not being reliably set in withScope. Potential v7 regression. #5261

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): Normalize trace context #5171

fix(core): Normalize trace context #5171

lforst commented May 27, 2022 •

edited

github-actions bot commented May 27, 2022 •

edited

vladanpaunovic left a comment

lforst commented May 30, 2022 •

edited

lobsterkatie left a comment

lobsterkatie Jun 1, 2022

lforst Jun 1, 2022

lobsterkatie Jun 1, 2022

lforst Jun 1, 2022

lforst Jun 3, 2022

AbhiPrasad Jun 2, 2022

lforst Jun 3, 2022

AbhiPrasad Jun 3, 2022

AbhiPrasad Jun 2, 2022

lforst Jun 3, 2022

timfish commented Jun 16, 2022

lobsterkatie commented Jun 16, 2022

lforst commented Jun 16, 2022

fix(core): Normalize trace context #5171

fix(core): Normalize trace context #5171

Conversation

lforst commented May 27, 2022 • edited

github-actions bot commented May 27, 2022 • edited

size-limit report 📦

vladanpaunovic left a comment

Choose a reason for hiding this comment

lforst commented May 30, 2022 • edited

lobsterkatie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timfish commented Jun 16, 2022

lobsterkatie commented Jun 16, 2022

lforst commented Jun 16, 2022

lforst commented May 27, 2022 •

edited

github-actions bot commented May 27, 2022 •

edited

lforst commented May 30, 2022 •

edited