feat: attach threads/stacktraces

[marandaneto]

📢 Type of change

• Bugfix
• New feature
• Enhancement
• Refactoring

📜 Description

feat: attach stacktraces

💡 Motivation and Context

attach stacktraces is disabled by default, but it should have a flag to opt-in.
right now is enabled by default and there's no opt-out.

💚 How did you test it?

unit test.

📝 Checklist

• I reviewed submitted code
• I added tests to verify changes
• All tests passing

🔮 Next steps

[codecov-io]

Codecov Report

Merging #267 into master will decrease coverage by 0.05%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master     #267      +/-   ##
============================================
- Coverage     57.32%   57.27%   -0.06%     
  Complexity      581      581              
============================================
  Files            73       73              
  Lines          2779     2785       +6     
  Branches        237      235       -2     
============================================
+ Hits           1593     1595       +2     
- Misses         1068     1072       +4     
  Partials        118      118

Impacted Files	Coverage Δ	Complexity Δ
...core/src/main/java/io/sentry/core/SentryLevel.java	100% <100%> (ø)	1 <0> (ø)	⬇️
...re/src/main/java/io/sentry/core/SentryOptions.java	79.16% <100%> (-1.01%)	58 <2> (+1)
...-core/src/main/java/io/sentry/core/Breadcrumb.java	89.09% <100%> (ø)	21 <1> (ø)	⬇️
.../main/java/io/sentry/core/SentryThreadFactory.java	90% <60%> (-6.3%)	10 <1> (+1)
...c/main/java/io/sentry/core/MainEventProcessor.java	75% <75%> (+1.31%)	9 <0> (-1)	⬇️
...core/src/main/java/io/sentry/core/SentryEvent.java	61.6% <0%> (-0.9%)	40% <0%> (-1%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9499deb...9053be6. Read the comment docs.

[bruno-garcia]

One might come and ask not to have all threads sent at all. Which would happen for captureException even if attachStacktrace false.
Maybe we can add a new setting called attachAllTreads (with a better name) to control that?

And not have attachStacktrace at all?

[marandaneto]

One might come and ask not to have all threads sent at all. Which would happen for captureException even if attachStacktrace false.
Maybe we can add a new setting called attachAllTreads (with a better name) to control that?

And not have attachStacktrace at all?

I'd agree that either we change its name from attachStacktrace to attachThreads or to still send threads, but without stacktraces, not sure which one makes more sense to the final user.

I believe captureException should not be affected at all, as the unified API defines only for messages.
https://docs.sentry.io/error-reporting/configuration/?platform=javascript#attach-stacktrace

@HazAT do you have opinions?

[HazAT]

Let's leave it like this, for now, it's very rare that a user would like to have all threads mostly they are interested in the thread they called this from.

[bruno-garcia]

@marandaneto Adding Threads (themselves and their stack traces) adds a lot of data and I can see people might be asking an opt-out for that.
In the case of Exception, that has a stacktrace itself, so if someone opts out to sending threads, it'll always get the exception stacktrace (as other SDKs work).
attachStacktrace is usually to add a single stack trace, for the current thread (the one called captureMessage). If we have a single options: attachThreads (default to true), we give the behavior of attachStacktraces for free but a way to also opt-out of it.

@HazAT Right now we're adding all threads. I believe our guideline says we should opt-in to features like this and let users opt-out if they don't want it. I'm proposing not adding attachStacktrace option but only attachThreads instead to be able to turn it off. Essentially we're attaching stacktraces for all events anyway unless user opts out.

[marandaneto]

@marandaneto Adding Threads (themselves and their stack traces) adds a lot of data and I can see people might be asking an opt-out for that.

I think it should be opt-in and not opt-out, as you just stated, it's a lot of data and mostly irrelevant if it's just a captureMessage, but I understand that the guidelines say the opposite, so I'm fine with it too.

In the case of Exception, that has a stacktrace itself, so if someone opts out to sending threads, it'll always get the exception stacktrace (as other SDKs work).
attachStacktrace is usually to add a single stack trace, for the current thread (the one called captureMessage). If we have a single options: attachThreads (default to true), we give the behavior of attachStacktraces for free but a way to also opt-out of it.

@HazAT Right now we're adding all threads. I believe our guideline says we should opt-in to features like this and let users opt-out if they don't want it. I'm proposing not adding attachStacktrace option but only attachThreads instead to be able to turn it off. Essentially we're attaching stacktraces for all events anyway unless user opts out.

So I agree that we need an attachThreads too, so users may opt-out of it, same for attachStacktrace, however attachStacktrace is disabled by default on the unified API docs, so we should decide about the default state of both and if we make difference between captureMessage and captureException

[bruno-garcia]

however attachStacktrace is disabled by default on the unified API docs

We can leave it off by default too, but the attachThreads (non "unified") enabled by default.
It's nice to give a rich event by default, users will see it on the first event coming in, they can always come and turn it off. If we don't turn this on, most people won't go through the settings and check what they are and turn them on.

[marandaneto]

however attachStacktrace is disabled by default on the unified API docs

We can leave it off by default too, but the attachThreads (non "unified") enabled by default.
It's nice to give a rich event by default, users will see it on the first event coming in, they can always come and turn it off. If we don't turn this on, most people won't go through the settings and check what they are and turn them on.

implemented as discussed, the different thing is, it's detached to captureMessage and/or captureEvent/captureException, based on the options, it will attach or not threads/stacktraces.
attachThreads enabled by default, attachStacktraces disabled though.

[bruno-garcia]

awesome!