Skip to content

Fix possible crash during the telemetry processing#1184

Merged
0xnm merged 1 commit into
developfrom
nogorodnikov/fix-possible-crash-during-telemetry-processing
Dec 12, 2022
Merged

Fix possible crash during the telemetry processing#1184
0xnm merged 1 commit into
developfrom
nogorodnikov/fix-possible-crash-during-telemetry-processing

Conversation

@0xnm

@0xnm 0xnm commented Dec 9, 2022

Copy link
Copy Markdown
Member

What does this PR do?

It is possible to have a crash like this:

java.lang.IllegalStateException: Service already shutdown
	at com.lyft.kronos.internal.ntp.SntpServiceImpl.ensureServiceIsRunning(SntpService.kt:142)
	at com.lyft.kronos.internal.ntp.SntpServiceImpl.currentTime(SntpService.kt:98)
	at com.lyft.kronos.internal.KronosClockImpl.getCurrentTime(KronosClockImpl.kt:19)
	at com.lyft.kronos.KronosClock$DefaultImpls.getCurrentTimeMs(Clock.kt:39)
	at com.lyft.kronos.internal.KronosClockImpl.getCurrentTimeMs(KronosClockImpl.kt:8)
	at com.datadog.android.core.internal.time.KronosTimeProvider.getServerOffsetMillis(KronosTimeProvider.kt:25)
	at com.datadog.android.telemetry.internal.TelemetryEventHandler.handleEvent(TelemetryEventHandler.kt:48)
	at com.datadog.android.rum.internal.monitor.DatadogRumMonitor.handleEvent$dd_sdk_android_debug(DatadogRumMonitor.kt:399)
	at com.datadog.android.rum.internal.monitor.DatadogRumMonitor.sendErrorTelemetryEvent(DatadogRumMonitor.kt:347)
	at com.datadog.android.telemetry.internal.Telemetry.error(Telemetry.kt:21)
	at com.datadog.android.log.internal.logger.TelemetryLogHandler.handleLog(TelemetryLogHandler.kt:22)
	at com.datadog.android.log.internal.logger.InternalLogHandler.handleLog(InternalLogHandler.kt:40)
	at com.datadog.android.log.Logger.internalLog$dd_sdk_android_debug(Logger.kt:596)
	at com.datadog.android.log.Logger.internalLog$dd_sdk_android_debug$default(Logger.kt:583)
	at com.datadog.android.log.Logger.log(Logger.kt:168)
	at com.datadog.android.log.internal.utils.LogUtilsKt.errorWithTelemetry(LogUtils.kt:45)
	at com.datadog.android.log.internal.utils.LogUtilsKt.errorWithTelemetry$default(LogUtils.kt:40)
	at com.datadog.android.core.internal.persistence.file.batch.BatchFileOrchestrator.isRootDirValid(BatchFileOrchestrator.kt:136)
	at com.datadog.android.core.internal.persistence.file.batch.BatchFileOrchestrator.getRootDir(BatchFileOrchestrator.kt:90)
	at com.datadog.android.core.internal.persistence.file.advanced.ConsentAwareFileMigrator.migrateData(ConsentAwareFileMigrator.kt:55)
	at com.datadog.android.core.internal.persistence.file.advanced.ConsentAwareFileMigrator.migrateData(ConsentAwareFileMigrator.kt:19)
	at com.datadog.android.core.internal.persistence.file.advanced.ConsentAwareFileOrchestrator.handleConsentChange(ConsentAwareFileOrchestrator.kt:88)
	at com.datadog.android.core.internal.persistence.file.advanced.ConsentAwareFileOrchestrator.onConsentUpdated(ConsentAwareFileOrchestrator.kt:74)
	at com.datadog.android.core.internal.privacy.TrackingConsentProvider.notifyCallbacks(TrackingConsentProvider.kt:59)
	at com.datadog.android.core.internal.privacy.TrackingConsentProvider.setConsent(TrackingConsentProvider.kt:40)
	at com.datadog.android.v2.core.DatadogCore.setTrackingConsent(DatadogCore.kt:156)
	at com.datadog.android.Datadog.initialize(Datadog.kt:73)
	at com.datadog.android.nightly.utils.MiscUtilsKt.initializeSdk(MiscUtils.kt:98)
	at com.datadog.android.nightly.utils.MiscUtilsKt.initializeSdk$default(MiscUtils.kt:91)
	at com.datadog.android.nightly.rum.RumViewTrackingE2ETests$rum_activity_view_tracking_strategy$1.invoke(RumViewTrackingE2ETests.kt:57)
	at com.datadog.android.nightly.rum.RumViewTrackingE2ETests$rum_activity_view_tracking_strategy$1.invoke(RumViewTrackingE2ETests.kt:48)
	at com.datadog.android.nightly.utils.MeasureUtilsKt.measureSdkInitialize(MeasureUtils.kt:27)
	at com.datadog.android.nightly.rum.RumViewTrackingE2ETests.rum_activity_view_tracking_strategy(RumViewTrackingE2ETests.kt:48)

It happens because we trigger the task of the consent update check, which is async and at the same time SDK is shutdown. Since we let existing tasks to complete during the executor shutdown, it may be the case when there is no folder anymore, so error is triggered which is processed by telemetry.

And since Kronos was shutdown, we get an exception trying to get server offset.

This change relies on the DatadogContext instead, which is created only if feature is still there. Features are removed before CoreFeature is stopped, so I hope this change should fix the issue.

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 requested a review from a team as a code owner December 9, 2022 10:26
@codecov-commenter

codecov-commenter commented Dec 9, 2022

Copy link
Copy Markdown

Codecov Report

Merging #1184 (4ff1089) into develop (84835ea) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1184      +/-   ##
===========================================
- Coverage    82.25%   82.23%   -0.02%     
===========================================
  Files          353      353              
  Lines        11777    11775       -2     
  Branches      2005     2005              
===========================================
- Hits          9686     9682       -4     
+ Misses        1474     1473       -1     
- Partials       617      620       +3     
Impacted Files Coverage Δ
.../main/kotlin/com/datadog/android/rum/RumMonitor.kt 76.60% <ø> (-0.49%) ⬇️
...ndroid/telemetry/internal/TelemetryEventHandler.kt 69.75% <100.00%> (-1.92%) ⬇️
...android/log/internal/logger/TelemetryLogHandler.kt 85.71% <0.00%> (-14.29%) ⬇️
...droid/rum/tracking/FragmentViewTrackingStrategy.kt 75.00% <0.00%> (-3.85%) ⬇️
...d/v2/core/internal/storage/FileEventBatchWriter.kt 95.24% <0.00%> (-2.38%) ⬇️
...in/com/datadog/android/log/internal/LogsFeature.kt 87.10% <0.00%> (-1.61%) ⬇️
...android/rum/internal/ndk/DatadogNdkCrashHandler.kt 86.21% <0.00%> (-0.99%) ⬇️
...android/v2/core/internal/DatadogContextProvider.kt 81.54% <0.00%> (ø)
...rc/main/java/com/datadog/opentracing/DDTracer.java 56.49% <0.00%> (+0.42%) ⬆️
... and 3 more

@0xnm
0xnm merged commit 67e6ba4 into develop Dec 12, 2022
@0xnm
0xnm deleted the nogorodnikov/fix-possible-crash-during-telemetry-processing branch December 12, 2022 12:16
@xgouchet xgouchet added this to the 1.16.0 milestone Dec 13, 2023
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.

4 participants