Client reports download status and download details #341
Conversation
When the client is downloading a package it will report the download status, and report download details (bytes per second and percent download) periodically.
michel-laterman
changed the title
Add feedback from review, not expected to pass until spec changes are made
client/internal/packagessyncer.go
Outdated
| } | ||
| // start the download reporter | ||
| detailsReporter := newDownloadReporter(downloadReporterDefaultInterval, packageLength) // TODO set interval | ||
| detailsReporter.report(ctx, status, s.reportStatuses) |
There was a problem hiding this comment.
Is reportStatuses safe to be called concurrently? The docs don't say it, so I am not sure. Note that reportStatuses calls user-supplied SetLastReportedStatuses which also is not documented to be safe for concurrent call. If you look at the only implementation in InMemPackagesStore I think it is indeed not safe to call concurrently.
| if p.packageLength > 0 { | ||
| downloadPercent = downloaded / p.packageLength * 100 | ||
| } | ||
| status.DownloadDetails = &protobufs.PackageDownloadDetails{ |
There was a problem hiding this comment.
I am a bit worried that status is a struct owned by someone else and we modify it from our goroutine here, while status's other fields are also simultaneously being modified by the owner. If the code is accidentally modified in the future to modify the same fields we will have a problem.
Can we instead change updateFn to accept the DownloadPercent and DownloadBytesPerSecond as parameters and eliminate the status parameter from downloadReporter? This way the responsibility for getting concurrency correctly will be sole responsibility of the caller of downloadReporter.report instead of being a shared responsibility.
Add implementation of downloading status along with download details details as described in open-telemetry/opamp-spec#206
When the client is downloading a package it will report the download status, and report download details (bytes per second and percent download) periodically.