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.
| } | ||
| } | ||
| // start the download reporter | ||
| detailsReporter := newDownloadReporter(downloadReporterDefaultInterval, packageLength) // TODO set interval |
There was a problem hiding this comment.
I need some guidance on where the user of a client library can be expected to set a value for the update interval, or if we even need to expose it
There was a problem hiding this comment.
I think this should be made configurable by the client
There was a problem hiding this comment.
Yes, my comment is more about my confusion as to where we should try to pass the config option from; Should we add PackageReporterInterval/SetPackageReporterInterval funcs to the types.PackagesStateProvider interface or does it belong somewhere else?
There was a problem hiding this comment.
I think it should be part of StartSettings
There was a problem hiding this comment.
StartSettings is not currently being passed to the NewPackagesSyncer function; do we want to add it as another parameter?
There was a problem hiding this comment.
NewPackagesSyncer is internal, so passing the interval param should be fine. This is similar to how we enabled heartbeat interval.
michel-laterman
changed the title
| case <-timer.C: | ||
| downloadTime := time.Now().Sub(p.start) | ||
| downloaded := p.downloaded.Load() | ||
| bps := downloaded / uint64(downloadTime/time.Second) |
There was a problem hiding this comment.
This could cause panic when the downloadTime is less than a second. Also, would make sense to report float instead uint64 for bps precision?
There was a problem hiding this comment.
I can update the spec for changing the bps to a float: open-telemetry/opamp-spec#210
| } | ||
| } | ||
| // start the download reporter | ||
| detailsReporter := newDownloadReporter(downloadReporterDefaultInterval, packageLength) // TODO set interval |
There was a problem hiding this comment.
I think this should be made configurable by the client
Add feedback from review, not expected to pass until spec changes are made
| // report periodically updates the package status details and calls the passed upateFn to send the new status. | ||
| func (p *downloadReporter) report(ctx context.Context, status *protobufs.PackageStatus, updateFn func(context.Context, bool) error) { | ||
| go func() { | ||
| // Make sure we wait at least 1s before reporting the download rate in bps to avoid a panic |
There was a problem hiding this comment.
I think having a lower bound of 1s on reporting interval makes sense, but I am not sure I understand the "avoid a panic" comment. What would be panic reason?
Also, downloadReporter.interval is essentially inaccurate by 1 second. Perhaps to make it accurate set the lower bound in newDownloadReporter instead?
| } | ||
| // 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.