RUM-8042 Add pending batches to batch deleted metric #2191
Conversation
maxep
force-pushed
the
maxep/RUM-8042/pending-batch-telemetry
branch
from
8d0f383 to
a6cd347
Compare
Datadog ReportBranch report: ✅ 0 Failed, 790 Passed, 3020 Skipped, 59.93s Total Time |
| @ReadWriteLock | ||
| private var pendingBatches: Int = 0 |
There was a problem hiding this comment.
I'm using a lock here because getting and deleting a file can happen on different threads, but this can add penalty to the call thread.
The other option will be to read the directory each time we send the telemetry:
BatchDeletedMetric.pendingBatches: try? directory.files()But this also has the drawback of reading the disk each time.
| @@ -258,6 +268,9 @@ internal class FilesOrchestrator: FilesOrchestratorType { | |||
| /// | |||
| /// Note: The `batchFile` doesn't exist at this point. | |||
| private func sendBatchDeletedMetric(batchFile: ReadableFile, deletionReason: BatchDeletedMetric.RemovalReason) { | |||
| // Decrement pending batches at each batch deletion | |||
| pendingBatches -= 1 | |||
There was a problem hiding this comment.
Shouldn't the decrementation happen where the batch is deleted rather than when sending the telemetry?
| let files = try directory.files() | ||
|
|
||
| // Reset pending batches for telemetry | ||
| pendingBatches = files.count |
There was a problem hiding this comment.
Could this lead to discrepancies? Right now this variable can be updated from different places.
There was a problem hiding this comment.
That's a fair point! Makes me leaning toward the solution proposed here. This was, we would be sure that data is accurate.
There was a problem hiding this comment.
Or could we update pendingBatches either only here, or when a file gets created & removed? This way we keep the lock.
There was a problem hiding this comment.
Getting the files.count while reading is to know the pending batches when the SDK starts. At start, no file has been created or removed but there could be files on disk. So, we need to get the count at read/create/delete operations. I will push the other solution, LMKWYT!
There was a problem hiding this comment.
Got it!
How often would this telemetry be sent, apart from app launch? I wonder what would be the performance impact of reading from disk.
Initially I wanted to suggest initializing the counter when creating the FileOrchestrator, but I see getReadableFiles is called from other places so that won't work.
Given that potential inconsistencies only happen at app launch, when the reset happens, would the initial counter implementation be acceptable? It might be a good tradeoff between performance and accuracy.
There was a problem hiding this comment.
I think reading the disk each time we send the telemetry is acceptable. With average upload frequency, this will be call every 2s if a batch is present and ready. So I don't expect any perf impact.
maxep
force-pushed
the
maxep/RUM-8042/pending-batch-telemetry
branch
from
9932036 to
63e8fd6
Compare
maxep
force-pushed
the
maxep/RUM-8042/pending-batch-telemetry
branch
from
63e8fd6 to
737e7f4
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
The median merge time in
|
dd-devflow
bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
What and why?
Add pending batches attribute to Batch Deleted Metric.
This attribute will help drawing distribution of pending batches throughout sessions.
How?
FilesOrchestratorkeeps track of pending batches and send it as part ofBatchDeletedMetrictelemetry.Review checklist
make api-surface)