RUMM-3506 Collect the batch deleted telemetry by mariusc83 · Pull Request #1577 · DataDog/dd-sdk-android

mariusc83 · 2023-08-16T12:03:33Z

What does this PR do?

In this PR we are following up on the already merged PR in the dd-sdk-ios repository in which we are sending a special batch_deleted telemetry log whenever a batch was deleted in the core. More details in this RFC.
An upcoming PR will also add the batch_closed related telemetry following the details in the same RFC.

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)

ncreated · 2023-08-16T16:32:44Z

+        return when (featureName) {
+            Feature.RUM_FEATURE_NAME -> "rum"
+            Feature.LOGS_FEATURE_NAME -> "logs"
+            Feature.TRACING_FEATURE_NAME -> "traces"


Ref. (in RFC):

track – one of: rum | logs | trace | sr (small letters)

Suggested change

Feature.TRACING_FEATURE_NAME -> "traces"

Feature.TRACING_FEATURE_NAME -> "trace"

ncreated · 2023-08-16T16:36:03Z

+    }
+
+    private fun resolveFileAge(file: File): Long {
+        return dateTimeProvider.getServerTimestamp() - file.asTimestamp()


question/ 💡 Do both getServerTimestamp() and file.asTimestamp() use the same time source? The first sounds to be NTP-fixed server time vs the other is likely a device time. If that's the case, this could go even negative.

oh that's a good catch...was actually counting on both using the same time provider. Need to check that.

ncreated · 2023-08-16T16:39:23Z

+            UPLOADER_WINDOW_KEY to filePersistenceConfig.recentDelayMs,
+
+            BATCH_REMOVAL_KEY to deletionReason.toString(),
+            IN_BACKGROUND_KEY to false


We mock it with false on iOS because we don't have background uploads in there. It's different for Android - let's get the real value in there 🚀💪🙏.

Not really sure how I could get that value honestly here, will try to see.

codecov-commenter · 2023-08-22T12:24:49Z

Codecov Report

Merging #1577 (d307f67) into develop (3bcb7ae) will increase coverage by 0.07%.
Report is 6 commits behind head on develop.
The diff coverage is 89.22%.

@@             Coverage Diff             @@
##           develop    #1577      +/-   ##
===========================================
+ Coverage    83.37%   83.45%   +0.07%     
===========================================
  Files          437      442       +5     
  Lines        14910    15035     +125     
  Branches      2230     2252      +22     
===========================================
+ Hits         12431    12546     +115     
  Misses        1885     1885              
- Partials       594      604      +10

Files Changed	Coverage Δ
...n/kotlin/com/datadog/android/api/InternalLogger.kt	`100.00% <ø> (ø)`
...roid/core/internal/data/upload/NoOpDataUploader.kt	`0.00% <0.00%> (ø)`
...tadog/android/core/internal/persistence/Storage.kt	`100.00% <ø> (ø)`
...om/datadog/android/telemetry/internal/Telemetry.kt	`12.50% <0.00%> (-2.88%)`	⬇️
...d/core/internal/data/upload/v2/NoOpDataUploader.kt	`50.00% <50.00%> (ø)`
...al/persistence/file/batch/BatchFileOrchestrator.kt	`93.15% <70.00%> (-1.14%)`	⬇️
...core/internal/data/upload/v2/DataUploadRunnable.kt	`96.67% <71.43%> (-3.33%)`	⬇️
...dog/android/core/internal/metrics/RemovalReason.kt	`75.00% <75.00%> (ø)`
...tlin/com/datadog/android/core/SdkInternalLogger.kt	`91.21% <87.50%> (+2.05%)`	⬆️
...in/com/datadog/android/core/internal/SdkFeature.kt	`89.66% <89.66%> (-1.43%)`	⬇️
... and 12 more

... and 13 files with indirect coverage changes

0xnm

very nice work! I left some questions.

0xnm · 2023-08-22T12:48:49Z

+        val uploadFrequency = resolveUploadFrequency()
+        val uploadConfiguration = DataUploadConfiguration(uploadFrequency)


why do we need it outside of is StorageBackedFeature check? if feature doesn't store anything, it shouldn't have anything to upload.

here I needed to discuss with you, as I wanted to do that but was not sure of the order of the methods there and why it was done like that. I will explain you in a call.

0xnm · 2023-08-23T06:43:02Z

+
+    private val trackName: String? = resolveTrackName(featureName)
+
+    override fun sendBatchDeletedMetric(batchFile: File, deletionReason: RemovalReason) {


Suggested change

override fun sendBatchDeletedMetric(batchFile: File, deletionReason: RemovalReason) {

override fun sendBatchDeletedMetric(batchFile: File, removalReason: RemovalReason) {

0xnm · 2023-08-23T06:43:55Z

+            UPLOADER_WINDOW_KEY to filePersistenceConfig.recentDelayMs,
+
+            BATCH_REMOVAL_KEY to deletionReason.toString(),
+            // todo: RUMM-952 add inBackground flag value


Suggested change

// todo: RUMM-952 add inBackground flag value

// TODO: RUMM-952 add inBackground flag value

0xnm · 2023-08-23T06:47:54Z

    /**
     * Utility to update the state of a batch, asynchronously.
     * @param batchId the id of the Batch to confirm
+     * @param removalReason the reason why the batch is being removed [RemovalReason]


Suggested change

* @param removalReason the reason why the batch is being removed [RemovalReason]

* @param removalReason the reason why the batch is being removed

0xnm · 2023-08-23T07:02:05Z

+        forge.addFactory(HttpRedirectStatusForgeryFactory())
+        forge.addFactory(HttpClientRateLimitingStatusForgeryFactory())
+        forge.addFactory(HttpClientErrorForgeryFactory())
+        forge.addFactory(HttpServerErrorForgeryFactory())
+        forge.addFactory(InvalidTokenErrorStatusForgeryFactory())
+        forge.addFactory(NetworkErrorStatusForgeryFactory())
+        forge.addFactory(RequestCreationErrorStatusForgeryFactory())
+        forge.addFactory(SuccessStatusForgeryFactory())
+        forge.addFactory(UnknownErrorStatusForgeryFactory())
+        forge.addFactory(UnknownStatusForgeryFactory())


cannot it all be in a single UploadStatus factory?

well I would have loved that too but unfortunately sometimes I need specific UploadStatus types in my tests.

0xnm · 2023-08-23T07:02:30Z

+        forge.addFactory(RemovalReasonFlushedForgeryFactory())
+        forge.addFactory(RemovalReasonPurgedForgeryFactory())
+        forge.addFactory(RemovalReasonInvalidForgeryFactory())
+        forge.addFactory(RemovalReasonObsoleteForgeryFactory())
+        forge.addFactory(RemovalReasonIntakeCodeForgeryFactory())
+        forge.addFactory(RemovalReasonForgeryFactory())


same, cannot it be in a single RemoveReason factory or we need such separation?

Same as above

xgouchet · 2023-08-16T12:21:19Z

    internal var uploadScheduler: UploadScheduler = NoOpUploadScheduler()
    internal var fileOrchestrator: FileOrchestrator = NoOpFileOrchestrator()
-
-    // region SDK Feature


You shouldn't delete this

yup...missed that

xgouchet · 2023-08-23T07:51:03Z

+                storage.confirmBatchRead(
+                    batchId,
+                    RemovalReason.IntakeCode(uploadStatus.code)
+                ) { confirmation ->


I probably missed that when we worked on v2, shouldn't we have the following? Otherwise seems like we only delete batches in cases of successful upload

confirmation.markAsRead(deleteBatch = !uploadStatus.shouldRetry)

That is a good point, in the DataUploadRunnable we are indeed relying on the status.shouldRetry in the markAsRead call.

I actually wanted to ask you about this, good point @xgouchet

0xnm

nice work!

mariusc83 self-assigned this Aug 16, 2023

mariusc83 force-pushed the mconstantin/rumm-3506/collect-batch-deleted-telemetry branch 5 times, most recently from ba9b604 to ecf976a Compare August 16, 2023 14:24

ncreated reviewed Aug 16, 2023

View reviewed changes

mariusc83 force-pushed the mconstantin/rumm-3506/collect-batch-deleted-telemetry branch from ecf976a to 19fa0a0 Compare August 22, 2023 10:59

mariusc83 changed the base branch from develop to mconstantin/rumm-924/add-the-internalloggger-metrics-method August 22, 2023 10:59

mariusc83 force-pushed the mconstantin/rumm-3506/collect-batch-deleted-telemetry branch from 19fa0a0 to 52a9fa1 Compare August 22, 2023 12:06

mariusc83 marked this pull request as ready for review August 22, 2023 12:17

mariusc83 requested a review from a team as a code owner August 22, 2023 12:17

mariusc83 force-pushed the mconstantin/rumm-924/add-the-internalloggger-metrics-method branch from cfde8e0 to 35e8143 Compare August 22, 2023 12:51

mariusc83 force-pushed the mconstantin/rumm-3506/collect-batch-deleted-telemetry branch from 52a9fa1 to 9104519 Compare August 22, 2023 12:58

mariusc83 force-pushed the mconstantin/rumm-924/add-the-internalloggger-metrics-method branch 4 times, most recently from b2af158 to c2f62d2 Compare August 22, 2023 14:30

0xnm reviewed Aug 23, 2023

View reviewed changes

mariusc83 force-pushed the mconstantin/rumm-924/add-the-internalloggger-metrics-method branch 2 times, most recently from ac0ffa6 to 71a10da Compare August 23, 2023 07:33

xgouchet reviewed Aug 23, 2023

View reviewed changes

mariusc83 force-pushed the mconstantin/rumm-3506/collect-batch-deleted-telemetry branch from 9104519 to d307f67 Compare August 23, 2023 09:29

mariusc83 force-pushed the mconstantin/rumm-924/add-the-internalloggger-metrics-method branch from 71a10da to 409b4e0 Compare August 23, 2023 09:31

Base automatically changed from mconstantin/rumm-924/add-the-internalloggger-metrics-method to develop August 23, 2023 11:07

Introduce the DataUploadConfiguration

a551354

mariusc83 force-pushed the mconstantin/rumm-3506/collect-batch-deleted-telemetry branch from d307f67 to e3ef1be Compare August 23, 2023 11:09

mariusc83 requested review from 0xnm and xgouchet August 23, 2023 11:15

0xnm approved these changes Aug 23, 2023

View reviewed changes

xgouchet approved these changes Aug 23, 2023

View reviewed changes

RUMM-3506 Collect the batch deleted telemetry

979430f

mariusc83 force-pushed the mconstantin/rumm-3506/collect-batch-deleted-telemetry branch from e3ef1be to 979430f Compare August 23, 2023 14:37

mariusc83 merged commit edf843e into develop Aug 23, 2023

mariusc83 deleted the mconstantin/rumm-3506/collect-batch-deleted-telemetry branch August 23, 2023 15:08

xgouchet added this to the 2.1.0 milestone Dec 13, 2023

	Feature.TRACING_FEATURE_NAME -> "traces"
	Feature.TRACING_FEATURE_NAME -> "trace"

		val uploadFrequency = resolveUploadFrequency()
		val uploadConfiguration = DataUploadConfiguration(uploadFrequency)


		private val trackName: String? = resolveTrackName(featureName)

		override fun sendBatchDeletedMetric(batchFile: File, deletionReason: RemovalReason) {

	// todo: RUMM-952 add inBackground flag value
	// TODO: RUMM-952 add inBackground flag value

	* @param removalReason the reason why the batch is being removed [RemovalReason]
	* @param removalReason the reason why the batch is being removed

Uh oh!

Conversation

mariusc83 commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mariusc83 commented Aug 16, 2023 •

edited

Loading

codecov-commenter commented Aug 22, 2023 •

edited

Loading