Skip to content
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

RUM-2928: Report all threads in case of crash #1848

Merged

Conversation

0xnm
Copy link
Member

@0xnm 0xnm commented Feb 7, 2024

What does this PR do?

This PR will make all threads to be reported in case of the application crash. Data will be sent in the error.threads property for both Logs and RUM, and will include the following:

  • name - thread name
  • crashed - if this particular thread crashed or not (it will be the thread for which we caught an exception).
  • state - thread state at the moment of the crash, according to the Thread.State
  • stack - stacktrace of the thread at the crash moment, for the crashed thread it will exception stacktrace instead.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@0xnm 0xnm force-pushed the nogorodnikov/rum-2928/report-all-threads-in-case-of-crash branch from 72153f1 to 68aa1ae Compare February 7, 2024 14:10
@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2024

Codecov Report

Merging #1848 (f28e295) into develop (c925c52) will increase coverage by 0.16%.
The diff coverage is 90.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1848      +/-   ##
===========================================
+ Coverage    83.41%   83.58%   +0.16%     
===========================================
  Files          467      470       +3     
  Lines        16530    16567      +37     
  Branches      2498     2492       -6     
===========================================
+ Hits         13788    13846      +58     
+ Misses        2050     2046       -4     
+ Partials       692      675      -17     
Files Coverage Δ
...com/datadog/android/core/feature/event/JvmCrash.kt 100.00% <100.00%> (ø)
...m/datadog/android/core/feature/event/ThreadDump.kt 100.00% <100.00%> (ø)
...in/com/datadog/android/log/internal/LogsFeature.kt 86.15% <100.00%> (+1.87%) ⬆️
...android/log/internal/domain/DatadogLogGenerator.kt 97.92% <100.00%> (+0.16%) ⬆️
...atadog/android/log/internal/domain/LogGenerator.kt 81.82% <100.00%> (+1.82%) ⬆️
...lin/com/datadog/android/rum/internal/RumFeature.kt 93.71% <100.00%> (+1.20%) ⬆️
...g/android/rum/internal/domain/scope/RumRawEvent.kt 100.00% <100.00%> (ø)
.../android/rum/internal/domain/scope/RumViewScope.kt 93.85% <100.00%> (+0.07%) ⬆️
.../android/rum/internal/monitor/DatadogRumMonitor.kt 85.06% <100.00%> (+0.45%) ⬆️
...m/datadog/android/core/internal/utils/ThreadExt.kt 50.00% <50.00%> (ø)
... and 1 more

... and 25 files with indirect coverage changes

@0xnm 0xnm marked this pull request as ready for review February 7, 2024 14:38
@0xnm 0xnm requested review from a team as code owners February 7, 2024 14:38
Copy link
Member

@mariusc83 mariusc83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work !

val thread = it.key
val isCrashedThread = thread == crashedThread
val stack = if (isCrashedThread) {
crashException.loggableStackTrace()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the difference between using crashExceptiom.loggableStackTrace and thread.stackTrae.loggableStackTrace and why not just using the later here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is that one reads a stacktrace from an exception, while the latter captures the current stacktrace of a given thread.
If the thread is the crashedThread, it means that this is the current one, and so capturing the current stacktrace would report the state as the DDExceptionHandler

Copy link
Member Author

@0xnm 0xnm Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, @xgouchet explained it precisely :)

val thread = it.key
val isCrashedThread = thread == crashedThread
val stack = if (isCrashedThread) {
crashException.loggableStackTrace()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is that one reads a stacktrace from an exception, while the latter captures the current stacktrace of a given thread.
If the thread is the crashedThread, it means that this is the current one, and so capturing the current stacktrace would report the state as the DDExceptionHandler

@0xnm 0xnm force-pushed the nogorodnikov/rum-2928/report-all-threads-in-case-of-crash branch from 68aa1ae to f28e295 Compare February 8, 2024 11:21
@0xnm 0xnm merged commit c9f8785 into develop Feb 8, 2024
23 checks passed
@0xnm 0xnm deleted the nogorodnikov/rum-2928/report-all-threads-in-case-of-crash branch February 8, 2024 15:25
@xgouchet xgouchet added this to the 2.6.0 milestone Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants