RUMM-3103 remove global sdk core #1339
Conversation
Codecov Report
@@ Coverage Diff @@
## feature/sdkv2 #1339 +/- ##
=================================================
- Coverage 81.69% 81.42% -0.26%
=================================================
Files 373 373
Lines 13159 13139 -20
Branches 2201 2210 +9
=================================================
- Hits 10749 10698 -51
- Misses 1761 1784 +23
- Partials 649 657 +8
|
| import com.datadog.android.v2.core.internal.net.DataUploader | ||
| import java.util.LinkedList | ||
| import java.util.Queue | ||
| import java.util.concurrent.CountDownLatch | ||
| import java.util.concurrent.TimeUnit | ||
|
|
||
| @Suppress("DEPRECATION") // TODO RUMM-3103 remove deprecated references | ||
| internal class UploadWorker( |
There was a problem hiding this comment.
Isn't UploadWorker instance created by androidx.work library using the reflection? In this case it won't know how to inject this.
There was a problem hiding this comment.
Oh indeed that's an issue :/
| val coreFeature = (Datadog.globalSdkCore as? DatadogCore)?.coreFeature | ||
| if (coreFeature != null) { | ||
| val idled = (coreFeature.persistenceExecutorService as? ThreadPoolExecutor) | ||
| if (sdkCore is InternalSdkCore) { |
There was a problem hiding this comment.
Maybe we can use force cast here instead? because if sdkCore is not InternalSdkCore by some reason (say we did some refactoring), then we won't notice the bug that sometimes crash may be not submitted (or it will be flaky and hard to debug). Throwing is ok here imo, because it is us who create SDK instances. wdyt?
There was a problem hiding this comment.
Throwing an exception in the uncaughtException is a very bad practice IMHO. If there's another crash reporter around, it will report our SDK issue and not the original crash.
Maybe another option would be to have a method in the publicSdkCore that does what we want here (wait till idle or something similar).
There was a problem hiding this comment.
Let's keep it like that then, I'm not quite a fan of pulling this internal thing to the public SdkCore.
Regarding throwing an exception: my assumption is that if it is thrown, it will always be caught by our integration tests before leaking to the release, because it is us only who are creating the cores.
There was a problem hiding this comment.
My concern is just to avoid places in the SDK where something gets unnoticed if it is not as what we expect, this can lead to the hard debugging, especially if it happens on the client side.
...ation/src/androidTest/kotlin/com/datadog/android/sdk/integration/rum/ResourceTrackingTest.kt
Outdated
Show resolved
Hide resolved
...tegration/src/androidTest/kotlin/com/datadog/android/sdk/rules/MockServerActivityTestRule.kt
Show resolved
Hide resolved
.../androidTest/kotlin/com/datadog/android/sdk/integration/log/ConsentPendingGrantedLogsTest.kt
Show resolved
Hide resolved
...nted/nightly-tests/src/androidTest/kotlin/com/datadog/android/nightly/logs/LoggerE2ETests.kt
Outdated
Show resolved
Hide resolved
library/dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/AndroidTracer.kt
Outdated
Show resolved
Hide resolved
...tegration/src/androidTest/kotlin/com/datadog/android/sdk/rules/MockServerActivityTestRule.kt
Outdated
Show resolved
Hide resolved
xgouchet
changed the title
| // 2. we introduce FIFO queue also to avoid the bottleneck: if some feature batch cannot | ||
| // be uploaded we put retry task to the end of queue, so that batches of other features | ||
| // have a chance to go. | ||
| val sdkCore = Datadog.getInstance() as? InternalSdkCore |
There was a problem hiding this comment.
When we create uploadWorkRequest we can add the name of the instance it is created for and then read it here and get the instance needed.
xgouchet
force-pushed
the
xgouchet/RUMM-3103/remove_globalSdkCore
branch
from
c76c781
to
1559b91
Compare
What does this PR do?
Remove all references to the globalSdkCore and general static fields of the Datadog class.