Metric callback improve #390
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #390 +/- ##
=======================================
Coverage 88.74% 88.74%
=======================================
Files 21 21
Lines 5986 6024 +38
=======================================
+ Hits 5312 5346 +34
- Misses 674 678 +4
|
source/s3_util.c
Outdated
| metric->crt_info_metrics.error_code = error_code; | ||
| if (record_end) { | ||
| aws_high_res_clock_get_ticks((uint64_t *)&metric->time_metrics.end_timestamp_ns); | ||
| metric->time_metrics.total_duration_ns = |
There was a problem hiding this comment.
what's the deal with record_end? The docs on end_timestamp and total_duration say they're always available. If record_end=false we're going to end up queuing the telemetry event for delivery without these values being set yet?
If we're trying to delay until the body is delivered, should we just wait until its delivered before we call aws_s3_request_finish_up_metrics_synced() ?
There was a problem hiding this comment.
yeah ... I think you're double-delivering this telemetry event for downloads.
You probably want to just NOT call aws_s3_request_finish_up_metrics_synced() if you're going to call it instead after the body is delivered
There was a problem hiding this comment.
The idea is when we have body to be delivered, we delay the end_timestamp until the callback is scheduled to be invoked, from here
source/s3_util.c
Outdated
| metric->crt_info_metrics.error_code = error_code; | ||
| if (record_end) { | ||
| aws_high_res_clock_get_ticks((uint64_t *)&metric->time_metrics.end_timestamp_ns); | ||
| metric->time_metrics.total_duration_ns = |
There was a problem hiding this comment.
yeah ... I think you're double-delivering this telemetry event for downloads.
You probably want to just NOT call aws_s3_request_finish_up_metrics_synced() if you're going to call it instead after the body is delivered
| if (request->send_data.metrics != NULL) { | ||
| /* Request is done streaming the body, complete the metrics for the request now. */ | ||
| struct aws_s3_request_metrics *metrics = request->send_data.metrics; | ||
| metrics->crt_info_metrics.error_code = error_code; |
There was a problem hiding this comment.
why are you altering the metric's error_code? the error_code on the right-hand-side is whether the meta-request has failed. I would think the metrics callback would want to report whether that individual HTTP request failed.
Move the telemetry callback to be invoked from the meta request event loop
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.