RUM-8042 Replace Upload Quality with Batch Blocked telemetry #2230
Conversation
maxep
force-pushed
the
maxep/RUM-8042/batch-blocked-telemetry
branch
from
229b5b9 to
09a608a
Compare
Datadog ReportBranch report: ✅ 0 Failed, 1033 Passed, 2853 Skipped, 1m 16.08s Total duration (1m 31.33s time saved) |
maxep
force-pushed
the
maxep/RUM-8042/batch-blocked-telemetry
branch
from
09a608a to
f51cb4c
Compare
| /// Each track reports its own upload quality metrics. | ||
| let uploadQuality: [String: UploadQuality] | ||
| /// Information about the upload cycles during the session. | ||
| /// The upload cycles is splitting between upload track name. |
There was a problem hiding this comment.
\nit /// The upload cycles are splitting between upload track name.
maxep
dismissed stale reviews from barboraplasovska, simaoseica-dd, and mariedm
via
5204914
maxep
force-pushed
the
maxep/RUM-8042/batch-blocked-telemetry
branch
3 times, most recently
from
aa1b8d9 to
31ae520
Compare
| static let sampleRate: Float = 1.5 // 1.5% | ||
| /// The key for uploader's current delay. | ||
| static let uploaderDelayKey = "uploader_delay.current" | ||
| /// The key for count of bacthes being blocked. |
There was a problem hiding this comment.
nit: The key for the number of batches being blocked.
| DD.logger.debug("⏳ (\(self.featureName)) Uploading batches...") | ||
| self.backgroundTaskCoordinator?.beginBackgroundTask() | ||
| self.uploadFile(from: files.reversed(), context: context) | ||
| sendUploadCycleMetric() | ||
| } else { | ||
| let batchLabel = files?.isEmpty == false ? "YES" : (isSystemReady ? "NO" : "NOT CHECKED") | ||
| let batchLabel = files.isEmpty ? "NO" : "YES" |
There was a problem hiding this comment.
I imagine the answer is no, but does not sending "NOT CHECKED" anymore have any consequences on our metrics?
There was a problem hiding this comment.
No it doesn't, it was just for console printing. The drawback of this change is that we will check for batches even if the upload is blocked, but the impact is minimal as we only list the files.
There was a problem hiding this comment.
If I read this correctly, we use to send sendUploadQualityMetric both in cases of success and failure. Now we send sendUploadCycleMetric before every upload, and track failures with sendBatchBlockedMetric. So we don't explicitly send a telemetry in case of success. I imagine we can still graph the success rate, but is this intentional?
| let files = fileReader.readFiles(limit: maxBatchesPerUpload) | ||
|
|
||
| if !files.isEmpty && isSystemReady { |
There was a problem hiding this comment.
Can we try to asses if this change might have a negative performance impact ?
I don't think it would because contentsOfDirectory (urderlying Filesystem method used) only get URLs, and not the content of each file, but maybe it's worth checking using maxBatchesPerUpload files everytime ?
Since we only use files to get their count for the telemetry, it would be bad if this also had a performance impact.
There was a problem hiding this comment.
The files with their InputStream will be needed for the self.uploadFile(from: files.reversed(), context: context)
maxep
force-pushed
the
maxep/RUM-8042/batch-blocked-telemetry
branch
from
31ae520 to
564152b
Compare
What and why?
Specifications of
Upload Qualityattributes inSession Endedmetric telemetry are not suitable for graphing.Instead, I propose adding a new Batch metric event to report batches blocked during upload. Currently, we have the
Batch Deletedtelemetry metric, from which we can compute the count of batches deleted due tointake-code-202(successful upload). We can add aBatch Blockedmetric to report when a batch is blocked due to local blockers (low power, offline, etc.) or network failures. This will provide visibility into why batches are not uploaded and allow us to compute the ratio of successful uploads versus failures. This new telemetry event should be sampled at the same rate asBatch Deleted: 1.5%.The complex
rse.upload_qualityattribute can then be replaced by a simplerse.upload_cycle.<logs|rum|*>that track upload cycle per track. In this new definition, an Upload Cycle will include multiple batch upload.How?
Similar to
BatchDeletedMetric, theBatchBlockedMetricwill report reasons for blocking batch upload, or failures to upload (network or http)Review checklist
make api-surface)