fix(event): Move stack trace from exception to threads for message events by krystofwoldrich · Pull Request #6038 · getsentry/sentry-javascript

krystofwoldrich · 2022-10-25T11:58:36Z

Relates to: getsentry/sentry-react-native#2131

Implements: https://develop.sentry.dev/sdk/event-payloads/threads/

github-actions · 2022-10-25T12:10:21Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.61 KB (+0.61% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)	60.81 KB (+0.76% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.31 KB (+1.05% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)	54.37 KB (+1.29% 🔺)
@sentry/browser - Webpack (gzipped + minified)	20.09 KB (+1.13% 🔺)
@sentry/browser - Webpack (minified)	65.91 KB (+1.27% 🔺)
@sentry/react - Webpack (gzipped + minified)	20.11 KB (+1.13% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified)	45.86 KB (+0.16% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	26.36 KB (+0.45% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	24.83 KB (+0.85% 🔺)

AbhiPrasad · 2022-10-25T12:39:54Z

packages/browser/src/eventbuilder.ts

        values: [{ value: input, stacktrace: { frames } }],
      };
+      event.threads = {
+        values: [{ stacktrace: { frames } }],


h: I don't understand how this is fixing the problem here - doesn't this just duplicate the information?

I agree, the PR has a description of what needs to be fixed.

Yes, it is a duplicate, but I don't have enough context on how we plan to solve it. Remove stack trace from exception?

But does the rest make sense? The issue says to add frames to threads.

So, previously when debugging this, when calling captureMessage, the syntheticException was null, this is written in the issue description, so there was no exception nor thread set in the event.
Apparently something changed or I was using an older version of the JS SDK, now the syntheticException isn't null anymore even for captureMessage and message events contain an exception with a stack trace, IMO this is wrong.
captureMessage should contain a thread with the stack trace and not an exception with a stack trace.

@AbhiPrasad can you double-check if that makes sense now? thanks.

…vent

krystofwoldrich · 2022-10-25T15:24:15Z

I removed the duplicate stack trace, now message events have it only in threads.

I don't know if there is any other implementation besides browser and node.

marandaneto · 2022-10-26T08:16:38Z

I removed the duplicate stack trace, now message events have it only in threads.

I don't know if there is any other implementation besides browser and node.

For me it looks good, CI is unhappy tho.

Previous implementation would return empty array of exception frames. Now instead threads are checked and if those are also undefined or empty array the function returns undefined.

…vent

github-actions · 2022-11-22T00:26:35Z

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

marandaneto · 2022-11-22T09:21:51Z

@krystofwoldrich what's the state of this? didn't fix it directly in RN? can we close this or is still needed?

krystofwoldrich · 2022-11-22T12:03:44Z

@marandaneto From my side its done and it still needs to be merged.

At the moment the stack trace of Message is in exception which I think is wrong.

@getsentry/team-web-sdk-frontend Could anyone take another look?

AbhiPrasad · 2022-11-22T12:23:05Z

At the moment the stack trace of Message is in exception which I think is wrong.

If we change this, won't it be a breaking change? This could break users beforeSend callbacks if they are filtering on this. I know we want to break for correct behaviour, but this will cause runtime issues.

krystofwoldrich · 2022-11-30T12:12:43Z

@AbhiPrasad You are right. So, could we add it to the next major, v8?

AbhiPrasad · 2022-11-30T12:19:06Z

So, could we add it to the next major, v8?

Yup we can. Can we update the linked GH issue to reflect this then?

krystofwoldrich · 2022-12-01T12:06:10Z

@AbhiPrasad Sure we can, I just don't know what to update there.

AbhiPrasad · 2022-12-01T12:08:00Z

Went ahead and updated everything - just adding labels and updating the milestone.

krystofwoldrich · 2022-12-01T12:33:22Z

@AbhiPrasad Thanks.

krystofwoldrich · 2022-12-13T12:04:41Z

packages/types/src/event.ts

+  threads?: {
+    values: Thread[];
+  };


Could this be added in JS SDK v7? @AbhiPrasad

AbhiPrasad · 2023-01-30T09:45:10Z

@krystofwoldrich going to close this so we clean up the PR stream, but we can re-open and merge in once we start work on v8.

chore(event): Add threads to browser events

760241a

krystofwoldrich requested review from AbhiPrasad and marandaneto October 25, 2022 11:58

AbhiPrasad reviewed Oct 25, 2022

View reviewed changes

krystofwoldrich marked this pull request as draft October 25, 2022 13:17

krystofwoldrich changed the title ~~chore(event): Add threads to browser events~~ chore(event): Move stack trace from exception to threads for message events Oct 25, 2022

krystofwoldrich changed the title ~~chore(event): Move stack trace from exception to threads for message events~~ fix(event): Move stack trace from exception to threads for message events Oct 25, 2022

krystofwoldrich added 2 commits October 25, 2022 17:20

Remove duplicates, add node implementation, add unit tests

91ecc12

Merge remote-tracking branch 'origin/master' into kw/add-threads-to-e…

e1d43eb

…vent

krystofwoldrich marked this pull request as ready for review October 25, 2022 15:22

krystofwoldrich requested a review from AbhiPrasad October 25, 2022 19:30

Fix dedupe for message events without exception

2fb1b01

marandaneto mentioned this pull request Oct 31, 2022

fix(event): Message event contains stacktrace if attachStacktrace o… getsentry/sentry-react-native#2577

Merged

8 tasks

krystofwoldrich added 2 commits October 31, 2022 12:47

Change frames from event

3307e65

Previous implementation would return empty array of exception frames. Now instead threads are checked and if those are also undefined or empty array the function returns undefined.

Merge remote-tracking branch 'origin/master' into kw/add-threads-to-e…

4aefd4a

…vent

github-actions bot added the Status: Stale label Nov 22, 2022

krystofwoldrich added Status: In Progress and removed Status: Stale labels Nov 22, 2022

AbhiPrasad added the Meta: Breaking label Dec 1, 2022

AbhiPrasad added this to the 8.0.0 milestone Dec 1, 2022

marandaneto mentioned this pull request Dec 7, 2022

attachStacktrace enabled by default on the JS layer getsentry/sentry-react-native#2461

Closed

krystofwoldrich commented Dec 13, 2022

View reviewed changes

krystofwoldrich mentioned this pull request Dec 13, 2022

feat(types): Add threads to Event #6516

Merged

AbhiPrasad closed this Jan 30, 2023

krystofwoldrich added Status: Done and removed Status: In Progress labels Jan 30, 2023

AbhiPrasad deleted the kw/add-threads-to-event branch April 24, 2024 17:47

Uh oh!

Conversation

krystofwoldrich commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

AbhiPrasad Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marandaneto Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

krystofwoldrich Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

marandaneto Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marandaneto Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

krystofwoldrich commented Oct 25, 2022

Uh oh!

marandaneto commented Oct 26, 2022

Uh oh!

github-actions bot commented Nov 22, 2022

Uh oh!

marandaneto commented Nov 22, 2022

Uh oh!

krystofwoldrich commented Nov 22, 2022

Uh oh!

AbhiPrasad commented Nov 22, 2022

Uh oh!

krystofwoldrich commented Nov 30, 2022

Uh oh!

AbhiPrasad commented Nov 30, 2022

Uh oh!

krystofwoldrich commented Dec 1, 2022

Uh oh!

AbhiPrasad commented Dec 1, 2022

Uh oh!

krystofwoldrich commented Dec 1, 2022

Uh oh!

krystofwoldrich Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

AbhiPrasad commented Jan 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

krystofwoldrich commented Oct 25, 2022 •

edited

Loading

github-actions bot commented Oct 25, 2022 •

edited

Loading

AbhiPrasad Oct 25, 2022 •

edited

Loading

marandaneto Oct 25, 2022 •

edited

Loading