-
Notifications
You must be signed in to change notification settings - Fork 63
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
RUMM-3506 Collect the batch deleted telemetry #1577
Conversation
ba9b604
to
ecf976a
Compare
return when (featureName) { | ||
Feature.RUM_FEATURE_NAME -> "rum" | ||
Feature.LOGS_FEATURE_NAME -> "logs" | ||
Feature.TRACING_FEATURE_NAME -> "traces" |
There was a problem hiding this comment.
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)
Feature.TRACING_FEATURE_NAME -> "traces" | |
Feature.TRACING_FEATURE_NAME -> "trace" |
} | ||
|
||
private fun resolveFileAge(file: File): Long { | ||
return dateTimeProvider.getServerTimestamp() - file.asTimestamp() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 🚀💪🙏.
There was a problem hiding this comment.
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.
ecf976a
to
19fa0a0
Compare
19fa0a0
to
52a9fa1
Compare
Codecov Report
@@ 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
|
cfde8e0
to
35e8143
Compare
52a9fa1
to
9104519
Compare
b2af158
to
c2f62d2
Compare
There was a problem hiding this 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.
val uploadFrequency = resolveUploadFrequency() | ||
val uploadConfiguration = DataUploadConfiguration(uploadFrequency) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param removalReason the reason why the batch is being removed [RemovalReason] | |
* @param removalReason the reason why the batch is being removed |
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()) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
forge.addFactory(RemovalReasonFlushedForgeryFactory()) | ||
forge.addFactory(RemovalReasonPurgedForgeryFactory()) | ||
forge.addFactory(RemovalReasonInvalidForgeryFactory()) | ||
forge.addFactory(RemovalReasonObsoleteForgeryFactory()) | ||
forge.addFactory(RemovalReasonIntakeCodeForgeryFactory()) | ||
forge.addFactory(RemovalReasonForgeryFactory()) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
ac0ffa6
to
71a10da
Compare
internal var storage: Storage = NoOpStorage() | ||
internal var uploader: DataUploader = NoOpDataUploader() | ||
internal var uploadScheduler: UploadScheduler = NoOpUploadScheduler() | ||
internal var fileOrchestrator: FileOrchestrator = NoOpFileOrchestrator() | ||
|
||
// region SDK Feature |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 -> |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
9104519
to
d307f67
Compare
71a10da
to
409b4e0
Compare
d307f67
to
e3ef1be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work!
e3ef1be
to
979430f
Compare
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 specialbatch_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)