addressing review comments

Krish · Krish · commit 6d357b4bbaa0 · 2025-10-14T17:24:29.000-07:00
diff --git a/include/aws/s3/private/s3_request.h b/include/aws/s3/private/s3_request.h
@@ -34,19 +34,19 @@ struct aws_s3_request_metrics {
         /* The time stamp when the request was first initiated by the client, including the very first attempt.
          * This timestamp is set once at the beginning and never updated during retries. Timestamps are from
          * `aws_high_res_clock_get_ticks`. This will always be available. */
-        int64_t request_start_timestamp_ns;
+        int64_t s3_request_first_attempt_start_timestamp_ns;
 
         /* The time stamp when the request completely finished (success or final failure), including all retry
          * attempts. This is set when the request reaches its final state - either successful completion or
          * exhaustion of all retry attempts. Timestamps are from `aws_high_res_clock_get_ticks`. This will
          * be available with the last attempt. */
-        int64_t request_end_timestamp_ns;
+        int64_t s3_request_last_attempt_end_timestamp_ns;
 
         /* The total time duration for the complete request lifecycle from initial start to final completion,
          * including all retry attempts, backoff delays, and connection establishment time
-         * (request_end_timestamp_ns - request_start_timestamp_ns). This represents the end-to-end user
+         * (s3_request_last_attempt_end_timestamp_ns - s3_request_first_attempt_start_timestamp_ns). This represents the end-to-end user
          * experience time. This will be available with the last attempt. */
-        int64_t request_duration_ns;
+        int64_t s3_request_total_duration_ns;
 
         /* The time stamp when the request started by S3 client, which is prepared time by the client. Timestamps
          * are from `aws_high_res_clock_get_ticks`. This will always be available. */
@@ -164,8 +164,6 @@ struct aws_s3_request_metrics {
         int error_code;
         /* Retry attempt. */
         uint32_t retry_attempt;
-        /* The part number if it is a multipart request */
-        uint32_t part_number;
     } crt_info_metrics;
 
     struct aws_ref_count ref_count;
@@ -177,6 +175,17 @@ struct aws_s3_request {
     /* Linked list node used for queuing. */
     struct aws_linked_list_node node;
 
+    /* Timestamp when retry attempt started. Overwritten on each retry and copied to new attempt's setup data.
+     * -1 means data not available. Timestamp from `aws_high_res_clock_get_ticks` */
+    int64_t retry_start_timestamp_ns;
+    /* Timestamp when retry attempt ended. Overwritten on each retry and copied to new attempt's setup data.
+     * -1 means data not available. Timestamp from `aws_high_res_clock_get_ticks` */
+    int64_t retry_end_timestamp_ns;
+    /* Time duration for retry attempt (retry_end_timestamp_ns - retry_start_timestamp_ns).
+     * Overwritten on each retry and copied to new attempt's setup data.
+     * When retry_end_timestamp_ns is -1, means data not available. */
+    int64_t retry_duration_ns;
+
     /* Linked list node used for tracking the request is active from HTTP level. */
     struct aws_linked_list_node cancellable_http_streams_list_node;
 
diff --git a/include/aws/s3/s3_client.h b/include/aws/s3/s3_client.h
@@ -1606,21 +1606,21 @@ uint32_t aws_s3_request_metrics_get_retry_attempt(const struct aws_s3_request_me
 
 /* Get the request start timestamp from aws_s3_request_metrics. Always available. */
 AWS_S3_API
-void aws_s3_request_metrics_get_request_start_timestamp_ns(
+void aws_s3_request_metrics_get_s3_request_first_attempt_start_timestamp_ns(
     const struct aws_s3_request_metrics *metrics,
-    uint64_t *out_request_start_time);
+    uint64_t *out_s3_request_first_attempt_start_time);
 
 /* Get the request end timestamp. AWS_ERROR_S3_METRIC_DATA_NOT_AVAILABLE will be raised if data not
  * available. */
 AWS_S3_API
-int aws_s3_request_metrics_get_request_end_timestamp_ns(
+int aws_s3_request_metrics_get_s3_request_last_attempt_end_timestamp_ns(
     const struct aws_s3_request_metrics *metrics,
-    uint64_t *out_request_end_time);
+    uint64_t *out_s3_request_last_attempt_end_time);
 
 /* Get the request duration. AWS_ERROR_S3_METRIC_DATA_NOT_AVAILABLE will be raised if data not
  * available. */
 AWS_S3_API
-int aws_s3_request_metrics_get_request_duration_ns(
+int aws_s3_request_metrics_get_s3_request_total_duration_ns(
     const struct aws_s3_request_metrics *metrics,
     uint64_t *out_request_duration);
 
diff --git a/source/s3_client.c b/source/s3_client.c
@@ -2463,7 +2463,7 @@ void aws_s3_client_notify_connection_finished(
 
         /* Ask the retry strategy to schedule a retry of the request. */
         aws_high_res_clock_get_ticks(
-            (uint64_t *)&request->send_data.metrics->time_metrics.retry_delay_start_timestamp_ns);
+            (uint64_t *)&request->retry_start_timestamp_ns);
         if (aws_retry_strategy_schedule_retry(
                 connection->retry_token, error_type, s_s3_client_retry_ready, connection)) {
 
@@ -2485,6 +2485,9 @@ void aws_s3_client_notify_connection_finished(
 
 reset_connection:
 
+    aws_high_res_clock_get_ticks((uint64_t *)&request->send_data.metrics->time_metrics.s3_request_last_attempt_end_timestamp_ns);
+    request->send_data.metrics->time_metrics.s3_request_total_duration_ns = request->send_data.metrics->time_metrics.s3_request_last_attempt_end_timestamp_ns - request->send_data.metrics->time_metrics.s3_request_first_attempt_start_timestamp_ns;
+
     if (connection->retry_token != NULL) {
         /* If we have a retry token and successfully finished, record that success. */
         if (finish_code == AWS_S3_CONNECTION_FINISH_CODE_SUCCESS) {
@@ -2553,6 +2556,9 @@ static void s_s3_client_retry_ready(struct aws_retry_token *token, int error_cod
     struct aws_s3_request *request = connection->request;
     AWS_PRECONDITION(request);
 
+    aws_high_res_clock_get_ticks((uint64_t *)&request->retry_end_timestamp_ns);
+    request->retry_duration_ns = request->retry_end_timestamp_ns - request->retry_start_timestamp_ns;
+
     struct aws_s3_meta_request *meta_request = request->meta_request;
     AWS_PRECONDITION(meta_request);
 
diff --git a/source/s3_meta_request.c b/source/s3_meta_request.c
@@ -1518,6 +1518,12 @@ static void s_s3_meta_request_stream_metrics(
     s3_metrics->time_metrics.send_end_timestamp_ns = http_metrics->send_end_timestamp_ns;
     s3_metrics->time_metrics.sending_duration_ns = http_metrics->sending_duration_ns;
     s3_metrics->time_metrics.receive_start_timestamp_ns = http_metrics->receive_start_timestamp_ns;
+    
+    if (s3_metrics->time_metrics.receive_start_timestamp_ns != -1) {
+        s3_metrics->time_metrics.service_call_duration_ns =
+            s3_metrics->time_metrics.receive_start_timestamp_ns - s3_metrics->time_metrics.conn_acquire_start_timestamp_ns;
+    }
+
     s3_metrics->time_metrics.receive_end_timestamp_ns = http_metrics->receive_end_timestamp_ns;
     s3_metrics->time_metrics.receiving_duration_ns = http_metrics->receiving_duration_ns;
 
@@ -1902,7 +1908,7 @@ static struct aws_s3_request_metrics *s_s3_request_finish_up_and_release_metrics
             aws_high_res_clock_get_ticks((uint64_t *)&metrics->time_metrics.end_timestamp_ns);
             metrics->time_metrics.total_duration_ns =
                 metrics->time_metrics.end_timestamp_ns - metrics->time_metrics.start_timestamp_ns;
-            aws_high_res_clock_get_ticks((uint64_t *)&metrics->time_metrics.request_end_timestamp_ns);
+            aws_high_res_clock_get_ticks((uint64_t *)&metrics->time_metrics.s3_request_last_attempt_end_timestamp_ns);
         }
 
         if (meta_request->telemetry_callback != NULL) {
diff --git a/source/s3_request.c b/source/s3_request.c
@@ -92,23 +92,11 @@ void aws_s3_request_setup_send_data(struct aws_s3_request *request, struct aws_h
     if (request->num_times_prepared > 0 && request->send_data.metrics != NULL) {
         /* If there is a metrics from previous attempt, complete it now. */
 
-        /* checkpoint retry_delay_end and calculate duration */
-        aws_high_res_clock_get_ticks(
-            (uint64_t *)&request->send_data.metrics->time_metrics.retry_delay_end_timestamp_ns);
-        request->send_data.metrics->time_metrics.retry_delay_duration_ns =
-            request->send_data.metrics->time_metrics.retry_delay_end_timestamp_ns -
-            request->send_data.metrics->time_metrics.retry_delay_start_timestamp_ns;
-
         struct aws_s3_request_metrics *metric = request->send_data.metrics;
         aws_high_res_clock_get_ticks((uint64_t *)&metric->time_metrics.end_timestamp_ns);
         metric->time_metrics.total_duration_ns =
             metric->time_metrics.end_timestamp_ns - metric->time_metrics.start_timestamp_ns;
 
-        if (metric->time_metrics.receive_start_timestamp_ns != -1) {
-            metric->time_metrics.service_call_duration_ns =
-                metric->time_metrics.receive_start_timestamp_ns - metric->time_metrics.conn_acquire_start_timestamp_ns;
-        }
-
         struct aws_s3_meta_request *meta_request = request->meta_request;
         if (meta_request != NULL && meta_request->telemetry_callback != NULL) {
 
@@ -120,7 +108,7 @@ void aws_s3_request_setup_send_data(struct aws_s3_request *request, struct aws_h
         }
 
         /* retain the first attempt timestamp since we should not re-initialize it. */
-        first_attempt_start_timestamp_ns = metric->time_metrics.request_start_timestamp_ns;
+        first_attempt_start_timestamp_ns = metric->time_metrics.s3_request_first_attempt_start_timestamp_ns;
 
         request->send_data.metrics = aws_s3_request_metrics_release(metric);
         aws_s3_request_clean_up_send_data(request);
@@ -129,20 +117,24 @@ void aws_s3_request_setup_send_data(struct aws_s3_request *request, struct aws_h
 
         // metrics persisted to the next request
         request->send_data.metrics->crt_info_metrics.retry_attempt = request->num_times_prepared;
-        request->send_data.metrics->time_metrics.request_start_timestamp_ns = first_attempt_start_timestamp_ns;
-    }
-
-    /* Set request start timestamp */
-    if (first_attempt_start_timestamp_ns == -1) {
-        aws_high_res_clock_get_ticks((uint64_t *)&request->send_data.metrics->time_metrics.request_start_timestamp_ns);
+        request->send_data.metrics->time_metrics.s3_request_first_attempt_start_timestamp_ns = first_attempt_start_timestamp_ns;
     }
 
     request->send_data.message = message;
     s_populate_metrics_from_message(request, message);
     /* Start the timestamp */
     aws_high_res_clock_get_ticks((uint64_t *)&request->send_data.metrics->time_metrics.start_timestamp_ns);
 
-    request->send_data.metrics->crt_info_metrics.part_number = request->part_number;
+    /* Set s3 request start timestamp if first attempt*/
+    if (first_attempt_start_timestamp_ns == -1) {
+        request->send_data.metrics->time_metrics.s3_request_first_attempt_start_timestamp_ns =
+            request->send_data.metrics->time_metrics.start_timestamp_ns;
+    }
+
+    /* copy delay duration since previous attempt */
+    request->send_data.metrics->time_metrics.retry_delay_start_timestamp_ns = request->retry_start_timestamp_ns;
+    request->send_data.metrics->time_metrics.retry_delay_end_timestamp_ns = request->retry_end_timestamp_ns;
+    request->send_data.metrics->time_metrics.retry_delay_duration_ns = request->retry_duration_ns;
 
     aws_http_message_acquire(message);
 }
@@ -233,9 +225,9 @@ struct aws_s3_request_metrics *aws_s3_request_metrics_new(struct aws_allocator *
     struct aws_s3_request_metrics *metrics = aws_mem_calloc(allocator, 1, sizeof(struct aws_s3_request_metrics));
     metrics->allocator = allocator;
 
-    metrics->time_metrics.request_start_timestamp_ns = -1;
-    metrics->time_metrics.request_end_timestamp_ns = -1;
-    metrics->time_metrics.request_duration_ns = -1;
+    metrics->time_metrics.s3_request_first_attempt_start_timestamp_ns = -1;
+    metrics->time_metrics.s3_request_last_attempt_end_timestamp_ns = -1;
+    metrics->time_metrics.s3_request_total_duration_ns = -1;
 
     metrics->time_metrics.start_timestamp_ns = -1;
     metrics->time_metrics.end_timestamp_ns = -1;
@@ -617,35 +609,35 @@ uint32_t aws_s3_request_metrics_get_retry_attempt(const struct aws_s3_request_me
     return metrics->crt_info_metrics.retry_attempt;
 }
 
-void aws_s3_request_metrics_get_request_start_timestamp_ns(
+void aws_s3_request_metrics_get_s3_request_first_attempt_start_timestamp_ns(
     const struct aws_s3_request_metrics *metrics,
-    uint64_t *out_request_start_time) {
+    uint64_t *out_s3_request_first_attempt_start_time) {
     AWS_PRECONDITION(metrics);
-    AWS_PRECONDITION(out_request_start_time);
-    *out_request_start_time = metrics->time_metrics.request_start_timestamp_ns;
+    AWS_PRECONDITION(out_s3_request_first_attempt_start_time);
+    *out_s3_request_first_attempt_start_time = metrics->time_metrics.s3_request_first_attempt_start_timestamp_ns;
 }
 
-int aws_s3_request_metrics_get_request_end_timestamp_ns(
+int aws_s3_request_metrics_get_s3_request_last_attempt_end_timestamp_ns(
     const struct aws_s3_request_metrics *metrics,
-    uint64_t *out_request_end_time) {
+    uint64_t *out_s3_request_last_attempt_end_time) {
     AWS_PRECONDITION(metrics);
-    AWS_PRECONDITION(out_request_end_time);
-    if (metrics->time_metrics.request_end_timestamp_ns < 0) {
+    AWS_PRECONDITION(out_s3_request_last_attempt_end_time);
+    if (metrics->time_metrics.s3_request_last_attempt_end_timestamp_ns < 0) {
         return aws_raise_error(AWS_ERROR_S3_METRIC_DATA_NOT_AVAILABLE);
     }
-    *out_request_end_time = metrics->time_metrics.request_end_timestamp_ns;
+    *out_s3_request_last_attempt_end_time = metrics->time_metrics.s3_request_last_attempt_end_timestamp_ns;
     return AWS_OP_SUCCESS;
 }
 
-int aws_s3_request_metrics_get_request_duration_ns(
+int aws_s3_request_metrics_get_s3_request_total_duration_ns(
     const struct aws_s3_request_metrics *metrics,
     uint64_t *out_request_duration) {
     AWS_PRECONDITION(metrics);
     AWS_PRECONDITION(out_request_duration);
-    if (metrics->time_metrics.request_duration_ns < 0) {
+    if (metrics->time_metrics.s3_request_total_duration_ns < 0) {
         return aws_raise_error(AWS_ERROR_S3_METRIC_DATA_NOT_AVAILABLE);
     }
-    *out_request_duration = metrics->time_metrics.request_duration_ns;
+    *out_request_duration = metrics->time_metrics.s3_request_total_duration_ns;
     return AWS_OP_SUCCESS;
 }
 
diff --git a/source/s3_util.c b/source/s3_util.c
@@ -723,17 +723,6 @@ void aws_s3_request_finish_up_metrics_synced(struct aws_s3_request *request, str
         metrics->time_metrics.total_duration_ns =
             metrics->time_metrics.end_timestamp_ns - metrics->time_metrics.start_timestamp_ns;
 
-        if (metrics->time_metrics.receive_start_timestamp_ns != -1) {
-            metrics->time_metrics.service_call_duration_ns = metrics->time_metrics.receive_start_timestamp_ns -
-                                                             metrics->time_metrics.conn_acquire_start_timestamp_ns;
-        }
-
-        if (metrics->crt_info_metrics.error_code != AWS_S3_CONNECTION_FINISH_CODE_RETRY) {
-            aws_high_res_clock_get_ticks((uint64_t *)&metrics->time_metrics.request_end_timestamp_ns);
-            metrics->time_metrics.request_duration_ns =
-                metrics->time_metrics.request_end_timestamp_ns - metrics->time_metrics.request_start_timestamp_ns;
-        }
-
         if (meta_request->telemetry_callback != NULL) {
             struct aws_s3_meta_request_event event = {.type = AWS_S3_META_REQUEST_EVENT_TELEMETRY};
             event.u.telemetry.metrics = aws_s3_request_metrics_acquire(metrics);
