Skip to content

Fix the regression for the TelemetryErrorEvent with throwable#2325

Merged
mariusc83 merged 1 commit into
developfrom
mconstantin/fix-regression-on-telemetry-with-throwable-event
Oct 18, 2024
Merged

Fix the regression for the TelemetryErrorEvent with throwable#2325
mariusc83 merged 1 commit into
developfrom
mconstantin/fix-regression-on-telemetry-with-throwable-event

Conversation

@mariusc83

@mariusc83 mariusc83 commented Oct 16, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

What does this PR do?

When we refactor the Telemetry handling logic we missed a case when the Log.Error is reported with a throwable. Main reason why we missed this is that our integration tests were not reporting the problem in the CI (they were failing but this was not visible in the CI due to an issue in the shell environment on MacOs runners). This issue was currently discovered by @ambushwork while manually running the integration tests on his end.

There were 2 main problems caused by this regression was that in case this method was used fun error(message: String, throwable: Throwable):

  1. The stacktrace and kind were not correctly resolved from the Throwable.
  2. In case 2 consecutive telemetry error events would be sent: first with kind=null and second with throwable because of the way we were resolving the identifier they were both considered same event and second one would not be sent.

The impact is quite high as it is affecting all our telemetry error logs that are reporting only the Throwable.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

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)

@codecov-commenter

codecov-commenter commented Oct 16, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.40%. Comparing base (774c798) to head (7fa8445).
Report is 6 commits behind head on develop.

Files with missing lines Patch % Lines
...droid/internal/telemetry/InternalTelemetryEvent.kt 66.67% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #2325      +/-   ##
===========================================
- Coverage    70.41%   70.40%   -0.01%     
===========================================
  Files          739      739              
  Lines        27536    27538       +2     
  Branches      4614     4616       +2     
===========================================
- Hits         19388    19387       -1     
+ Misses        6866     6862       -4     
- Partials      1282     1289       +7
Files with missing lines Coverage Δ
...ore/src/main/kotlin/com/datadog/android/Datadog.kt 76.25% <ø> (ø)
.../android/error/internal/DatadogExceptionHandler.kt 87.84% <ø> (ø)
...com/datadog/android/internal/utils/ThrowableExt.kt 100.00% <ø> (ø)
...og/android/rum/internal/anr/ANRDetectorRunnable.kt 81.82% <ø> (ø)
...roid/rum/internal/domain/scope/RumResourceScope.kt 94.95% <ø> (ø)
.../android/rum/internal/domain/scope/RumViewScope.kt 95.31% <ø> (+0.27%) ⬆️
...ndroid/telemetry/internal/TelemetryEventHandler.kt 89.19% <100.00%> (ø)
...dog/android/telemetry/internal/TelemetryEventId.kt 100.00% <100.00%> (ø)
.../trace/internal/handlers/AndroidSpanLogsHandler.kt 87.76% <ø> (ø)
...datadog/android/okhttp/trace/TracingInterceptor.kt 76.03% <ø> (-0.63%) ⬇️
... and 1 more

... and 24 files with indirect coverage changes

@mariusc83
mariusc83 force-pushed the mconstantin/fix-regression-on-telemetry-with-throwable-event branch 2 times, most recently from cd8067a to ab5dca2 Compare October 16, 2024 18:43
@mariusc83 mariusc83 self-assigned this Oct 17, 2024
@mariusc83
mariusc83 force-pushed the mconstantin/fix-regression-on-telemetry-with-throwable-event branch from ab5dca2 to ebd0dba Compare October 17, 2024 09:20
@mariusc83
mariusc83 marked this pull request as ready for review October 17, 2024 09:23
@mariusc83
mariusc83 requested review from a team as code owners October 17, 2024 09:23
ambushwork
ambushwork reviewed Oct 17, 2024
View reviewed changes
Comment thread dd-sdk-android-internal/src/main/java/com/datadog/android/internal/utils/ByteArrayExt.kt Outdated
Comment thread ...roid-internal/src/main/java/com/datadog/android/internal/telemetry/InternalTelemetryEvent.kt Outdated
@mariusc83
mariusc83 force-pushed the mconstantin/fix-regression-on-telemetry-with-throwable-event branch 2 times, most recently from 9122d0c to 9e16282 Compare October 17, 2024 12:27
@mariusc83
mariusc83 force-pushed the mconstantin/fix-regression-on-telemetry-with-throwable-event branch from 9e16282 to 7fa8445 Compare October 17, 2024 13:21
@mariusc83
mariusc83 merged commit 25dee6b into develop Oct 18, 2024
@mariusc83
mariusc83 deleted the mconstantin/fix-regression-on-telemetry-with-throwable-event branch October 18, 2024 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ambushwork ambushwork ambushwork approved these changes

Assignees

@mariusc83 mariusc83

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mariusc83 @codecov-commenter @ambushwork