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

RUMM-3506 Collect the batch deleted telemetry #1577

Merged

Conversation

mariusc83
Copy link
Member

@mariusc83 mariusc83 commented Aug 16, 2023

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)

@mariusc83 mariusc83 self-assigned this Aug 16, 2023
@mariusc83 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
return when (featureName) {
Feature.RUM_FEATURE_NAME -> "rum"
Feature.LOGS_FEATURE_NAME -> "logs"
Feature.TRACING_FEATURE_NAME -> "traces"
Copy link
Member

Choose a reason for hiding this comment

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

Ref. (in RFC):

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

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

}

private fun resolveFileAge(file: File): Long {
return dateTimeProvider.getServerTimestamp() - file.asTimestamp()
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

UPLOADER_WINDOW_KEY to filePersistenceConfig.recentDelayMs,

BATCH_REMOVAL_KEY to deletionReason.toString(),
IN_BACKGROUND_KEY to false
Copy link
Member

Choose a reason for hiding this comment

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

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 🚀💪🙏.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@mariusc83 mariusc83 force-pushed the mconstantin/rumm-3506/collect-batch-deleted-telemetry branch from ecf976a to 19fa0a0 Compare August 22, 2023 10:59
@mariusc83 mariusc83 changed the base branch from develop to mconstantin/rumm-924/add-the-internalloggger-metrics-method August 22, 2023 10:59
@mariusc83 mariusc83 force-pushed the mconstantin/rumm-3506/collect-batch-deleted-telemetry branch from 19fa0a0 to 52a9fa1 Compare August 22, 2023 12:06
@mariusc83 mariusc83 marked this pull request as ready for review August 22, 2023 12:17
@mariusc83 mariusc83 requested a review from a team as a code owner August 22, 2023 12:17
@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2023

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

@mariusc83 mariusc83 force-pushed the mconstantin/rumm-924/add-the-internalloggger-metrics-method branch from cfde8e0 to 35e8143 Compare August 22, 2023 12:51
@mariusc83 mariusc83 force-pushed the mconstantin/rumm-3506/collect-batch-deleted-telemetry branch from 52a9fa1 to 9104519 Compare August 22, 2023 12:58
@mariusc83 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
Copy link
Member

@0xnm 0xnm left a comment

Choose a reason for hiding this comment

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

very nice work! I left some questions.

Comment on lines 66 to 67
val uploadFrequency = resolveUploadFrequency()
val uploadConfiguration = DataUploadConfiguration(uploadFrequency)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


private val trackName: String? = resolveTrackName(featureName)

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

Choose a reason for hiding this comment

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

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

UPLOADER_WINDOW_KEY to filePersistenceConfig.recentDelayMs,

BATCH_REMOVAL_KEY to deletionReason.toString(),
// todo: RUMM-952 add inBackground flag value
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -52,10 +53,15 @@ internal interface Storage {
/**
* 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]
Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines +49 to +58
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())
Copy link
Member

Choose a reason for hiding this comment

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

cannot it all be in a single UploadStatus factory?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Comment on lines +61 to +66
forge.addFactory(RemovalReasonFlushedForgeryFactory())
forge.addFactory(RemovalReasonPurgedForgeryFactory())
forge.addFactory(RemovalReasonInvalidForgeryFactory())
forge.addFactory(RemovalReasonObsoleteForgeryFactory())
forge.addFactory(RemovalReasonIntakeCodeForgeryFactory())
forge.addFactory(RemovalReasonForgeryFactory())
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above

@mariusc83 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
internal var storage: Storage = NoOpStorage()
internal var uploader: DataUploader = NoOpDataUploader()
internal var uploadScheduler: UploadScheduler = NoOpUploadScheduler()
internal var fileOrchestrator: FileOrchestrator = NoOpFileOrchestrator()

// region SDK Feature
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't delete this

Copy link
Member Author

Choose a reason for hiding this comment

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

yup...missed that

storage.confirmBatchRead(
batchId,
RemovalReason.IntakeCode(uploadStatus.code)
) { confirmation ->
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@mariusc83 mariusc83 force-pushed the mconstantin/rumm-3506/collect-batch-deleted-telemetry branch from 9104519 to d307f67 Compare August 23, 2023 09:29
@mariusc83 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
@mariusc83 mariusc83 force-pushed the mconstantin/rumm-3506/collect-batch-deleted-telemetry branch from d307f67 to e3ef1be Compare August 23, 2023 11:09
@mariusc83 mariusc83 requested review from xgouchet and 0xnm August 23, 2023 11:15
Copy link
Member

@0xnm 0xnm 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!

@mariusc83 mariusc83 force-pushed the mconstantin/rumm-3506/collect-batch-deleted-telemetry branch from e3ef1be to 979430f Compare August 23, 2023 14:37
@mariusc83 mariusc83 merged commit edf843e into develop Aug 23, 2023
@mariusc83 mariusc83 deleted the mconstantin/rumm-3506/collect-batch-deleted-telemetry branch August 23, 2023 15:08
@xgouchet xgouchet added this to the 2.1.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.

5 participants