telemetry API #271

TingDaoK · 2023-03-29T16:05:43Z

Support get the telemetry after each single request

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

include/aws/s3/s3_client.h

graebm · 2023-03-29T17:38:39Z

include/aws/s3/s3_client.h

+ */
+struct aws_s3_request_telemetry_metrics {
+    /* X-AMZ-REQUEST-ID Header value. */
+    struct aws_byte_cursor request_id;


we're already passing all the response headers, we don't need this

just want to have a simple way to fetch the request id instead of fetching it from the headers.

include/aws/s3/s3_client.h

codecov-commenter · 2023-03-31T22:42:29Z

Codecov Report

Merging #271 (a8ee7e4) into main (ea78cb6) will increase coverage by 0.36%.
The diff coverage is 94.88%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #271      +/-   ##
==========================================
+ Coverage   87.52%   87.88%   +0.36%     
==========================================
  Files          16       16              
  Lines        4103     4316     +213     
==========================================
+ Hits         3591     3793     +202     
- Misses        512      523      +11

Impacted Files	Coverage Δ
source/s3.c	`96.15% <ø> (ø)`
source/s3_util.c	`98.64% <ø> (ø)`
source/s3_request.c	`95.02% <93.67%> (-4.98%)`	⬇️
source/s3_client.c	`87.35% <100.00%> (+0.04%)`	⬆️
source/s3_meta_request.c	`94.51% <100.00%> (+0.27%)`	⬆️

…metry-api

…t_ticks

include/aws/s3/private/s3_request.h

…metry-api

include/aws/s3/private/s3_util.h

include/aws/s3/s3_client.h

include/aws/s3/private/s3_request.h

DmitriyMusatkin · 2023-04-28T23:24:32Z

include/aws/s3/private/s3_request.h

+/**
+ * Information sent in the meta_request telemetry callback.
+ */
+struct aws_s3_request_metrics {


should we expose extra things like num_parts, bytes_transferred?

num_parts is included in the request_path_query, bytes_transferred can be interesting, but most likely will be the part size.

maybe i dont understand what request_path_query is, but how would it include num parts?

For put parts, it will have the part number in it, but, for multipart get, it is not.

I'll add the part number to be easier for the user

But, for multipart get, we were actually doing ranged-get, which is different from get parts. I guess it can be confused to the user as well.

include/aws/s3/private/s3_request.h

include/aws/s3/s3_client.h

waahm7 · 2023-04-29T01:35:09Z

include/aws/s3/private/s3_request.h

+        uint64_t send_end_timestamp_ns;
+        /* The time duration for the request from start encoding to finish encoding (send_end_timestamp_ns -
+         * send_start_timestamp_ns). When send_end_timestamp_ns is 0, means data not available. */
+        uint64_t sending_duration_ns;


Did we decide on #271 (comment)? I think it would be nice to be consistent with http layer.

In my opinion, the reason we used int64_t to have the data unavailable easy to check, but we provide the getters with error, so I guess it will make more sense for those value to be unsigned, as they should never less than zero.

source/s3_request.c

Co-authored-by: Waqar Ahmed Khan <waahm7@gmail.com>

…metry-api

This reverts commit 70c568d.

include/aws/s3/s3_client.h

source/s3_meta_request.c

Co-authored-by: Waqar Ahmed Khan <waahm7@gmail.com>

include/aws/s3/s3_client.h

graebm · 2023-05-02T22:21:20Z

include/aws/s3/s3_client.h

+AWS_S3_API
+void aws_s3_request_metrics_get_request_path_query(
+    const struct aws_s3_request_metrics *metrics,
+    struct aws_byte_cursor *out_request_path_query);


trivial: I would use aws_string for these string properties, instead of aws_byte_cursor/aws_byte_buf

Why? I think we have the metrics as the object holding all the data, so this is the getter to fetch the data from the object, will it be better to use aws_byte_cursor?

aws_string is null-terminated, which makes is easier if the user wants to pass it along to things like printf() or other old-fashioned C calls that expect a null-terminated string

Additionally, it is easier to reason about the lifetime of aws_string. With a byte_cursor, users must also ensure that the underlying memory is not freed until the lifetime of the byte_cursor.

I was still proposing the same sketchy lifetime as we'd get with aws_byte_cursor, where we just give them a pointer to the internal data and warn them about lifetimes

source/s3_request.c

graebm · 2023-05-02T22:34:52Z

source/s3_request.c

+    if (metrics->time_metrics.receive_end_timestamp_ns == 0 || metrics->time_metrics.receive_start_timestamp_ns == 0) {
+        return aws_raise_error(AWS_ERROR_S3_METRIC_DATA_NOT_AVAILABLE);
+    }
+    *receiving_duration = metrics->time_metrics.receiving_duration_ns;


wouldn't it be less error_prone to continue storing times as int64_t internally, so we could simply use -1 to indicate date not available?

Suggested change

if (metrics->time_metrics.receive_end_timestamp_ns == 0 || metrics->time_metrics.receive_start_timestamp_ns == 0) {

return aws_raise_error(AWS_ERROR_S3_METRIC_DATA_NOT_AVAILABLE);

}

*receiving_duration = metrics->time_metrics.receiving_duration_ns;

if (metrics->time_metrics.receiving_duration_ns < 0) {

return aws_raise_error(AWS_ERROR_S3_METRIC_DATA_NOT_AVAILABLE);

}

*receiving_duration = (uint64_t)metrics->time_metrics.receiving_duration_ns;

vs doing this thing where 0 maaay be valid from some values, so we better consult two other values

like we were doing in that "Range: 0-0" bug we just had

…metry-api

graebm · 2023-05-03T22:10:16Z

include/aws/s3/s3_client.h

+/* Get the request ID from aws_s3_request_metrics, AWS_ERROR_S3_METRIC_DATA_NOT_AVAILABLE will be raised when data
+ * unavailable */
+AWS_S3_API
+int aws_s3_request_metrics_get_request_id(
+    struct aws_allocator *allocator,
+    const struct aws_s3_request_metrics *metrics,
+    struct aws_string **out_request_id);


Suggested change

/* Get the request ID from aws_s3_request_metrics, AWS_ERROR_S3_METRIC_DATA_NOT_AVAILABLE will be raised when data

* unavailable */

AWS_S3_API

int aws_s3_request_metrics_get_request_id(

struct aws_allocator *allocator,

const struct aws_s3_request_metrics *metrics,

struct aws_string **out_request_id);

/**

* Get the request ID from aws_s3_request_metrics.

* If unavailable, AWS_ERROR_S3_METRIC_DATA_NOT_AVAILABLE will be raised.

* If available, out_request_id will be set to a string. Be warned this string's lifetime is tied to the metrics object.

*/

AWS_S3_API

int aws_s3_request_metrics_get_request_id(

const struct aws_s3_request_metrics *metrics,

const struct aws_string **out_request_id);

// OR return NULL and raise error if unavailable

const struct aws_string *aws_s3_request_metrics_get_request_id(

const struct aws_s3_request_metrics *metrics);

I wasn't suggesting we allocate a new string every time someone queries, I was suggesting storing these internally as aws_string (instead of aws_byte_buf), and just handing out the pointer from the query

If all this rigamarole about using aws_string instead of aws_byte_cursor is more trouble than it's worth, then just roll back to using aws_byte_cursor. this was a debatable suggestion to begin with

source/s3_request.c

graebm

fix & ship

include/aws/s3/s3_client.h

source/s3.c

source/s3_meta_request.c

include/aws/s3/s3_client.h

jamesbornholt · 2023-05-10T14:37:15Z

include/aws/s3/s3_client.h

+     * Notes:
+     * - The callback will be invoked multiple times from different threads.


including concurrently?

jamesbornholt · 2023-05-10T14:37:41Z

include/aws/s3/s3_client.h


+    /**
+     * Optional.
+     * To get telemetry metrics when a single request finishes.


is this guaranteed to only be invoked before the finish_callback?

jamesbornholt · 2023-05-10T14:46:20Z

include/aws/s3/s3_client.h

+/* Get the part number of the request, if the request is not associated with a part, error code will be raised. */
+AWS_S3_API
+int aws_s3_request_metrics_get_part_number(const struct aws_s3_request_metrics *metrics, uint32_t *out_part_number);


Is it possible to get the range of the part for auto-ranged gets?

yes, but the response header should have content-range in it, and covers the range of the part for the request.

I think we can just remove it for now, the part number is confusing for auto-ranged get. And for auto-put we can get the information from the aws_s3_request_metrics_get_request_path_query

jamesbornholt · 2023-05-10T14:47:50Z

include/aws/s3/s3_client.h

+void aws_s3_request_metrics_get_request_path_query(
+    const struct aws_s3_request_metrics *metrics,
+    const struct aws_string **out_request_path_query);


Can we get the type of request this way (GetObject/HeadObject/UploadPart/etc)?

I'll add the type of request in a follow up PR to be more clear

TingDaoK added 2 commits March 29, 2023 09:03

telemetry API

f2e749c

ops, wrong structure

7bc0be0

graebm reviewed Mar 29, 2023

View reviewed changes

TingDaoK added 2 commits March 31, 2023 14:59

change the structure to private and init

7e2bc99

unused variable

28cc168

TingDaoK added 5 commits April 11, 2023 10:48

WIP

40514cd

impl the telemetry stuff

2ab446c

fix compile warning

67a2121

add get thread id

aee2407

Merge branch 'main' into telemetry-api

ead9746

TingDaoK marked this pull request as ready for review April 13, 2023 17:34

TingDaoK added 4 commits April 13, 2023 11:42

add more tests

e5c27e1

Merge branch 'telemetry-api' of github.com:awslabs/aws-c-s3 into tele…

c8191e4

…metry-api

add part of the doc and change timestamp to use aws_high_res_clock_ge…

ba89d2d

…t_ticks

Merge branch 'main' into telemetry-api

9ab302b

JonathanHenson reviewed Apr 21, 2023

View reviewed changes

include/aws/s3/private/s3_request.h Outdated Show resolved Hide resolved

TingDaoK added 5 commits April 24, 2023 10:26

update document

03fe52e

Merge branch 'main' into telemetry-api

8b226e8

use the new aws_http_connection_get_remote_endpoint

09fc4cf

Merge branch 'telemetry-api' of github.com:awslabs/aws-c-s3 into tele…

3eb0a8a

…metry-api

adapt change from http and more test

a13d45b

DmitriyMusatkin reviewed Apr 28, 2023

View reviewed changes

address comments

6a75795

waahm7 reviewed Apr 29, 2023

View reviewed changes

TingDaoK and others added 5 commits May 1, 2023 09:16

address comments

8f4fdf5

Apply suggestions from code review

d3abc40

Co-authored-by: Waqar Ahmed Khan <waahm7@gmail.com>

forget to update the test

4d9702f

Merge branch 'telemetry-api' of github.com:awslabs/aws-c-s3 into tele…

616172b

…metry-api

update the public interface of resume token as well

70c568d

Revert "update the public interface of resume token as well"

af68680

This reverts commit 70c568d.

waahm7 reviewed May 1, 2023

View reviewed changes

include/aws/s3/s3_client.h Outdated Show resolved Hide resolved

include/aws/s3/s3_client.h Outdated Show resolved Hide resolved

source/s3_meta_request.c Show resolved Hide resolved

TingDaoK and others added 4 commits May 1, 2023 15:53

Apply suggestions from code review

3a083b0

Co-authored-by: Waqar Ahmed Khan <waahm7@gmail.com>

add host address for the metric

14c986e

Merge branch 'main' into telemetry-api

bcb109c

Merge branch 'main' into telemetry-api

69b77f0

waahm7 approved these changes May 2, 2023

View reviewed changes

graebm reviewed May 2, 2023

View reviewed changes

TingDaoK added 6 commits May 3, 2023 12:48

address comments

665d597

Merge branch 'telemetry-api' of github.com:awslabs/aws-c-s3 into tele…

490ad0e

…metry-api

pointer of pointer

7c0f17b

Merge branch 'main' into telemetry-api

f764da2

add readme for account settings

862c670

Merge branch 'telemetry-api' of github.com:awslabs/aws-c-s3 into tele…

56a71c1

…metry-api

graebm reviewed May 3, 2023

View reviewed changes

TingDaoK added 3 commits May 3, 2023 15:31

Merge branch 'main' into telemetry-api

4569bbf

address comments

883abdf

unused variable

0d16418

graebm approved these changes May 9, 2023

View reviewed changes

include/aws/s3/s3_client.h Show resolved Hide resolved

source/s3.c Outdated Show resolved Hide resolved

source/s3_meta_request.c Outdated Show resolved Hide resolved

include/aws/s3/s3_client.h Outdated Show resolved Hide resolved

TingDaoK added 2 commits May 9, 2023 14:29

address comment

d772368

memory leak

5f79af5

jamesbornholt reviewed May 10, 2023

View reviewed changes

remove part number from metrics, which is mostly just confusing

a8ee7e4

TingDaoK merged commit 634d5b0 into main May 11, 2023

TingDaoK deleted the telemetry-api branch May 11, 2023 16:45

TingDaoK mentioned this pull request May 15, 2023

Add request type to telemetry #287

Merged

		* Notes:
		* - The callback will be invoked multiple times from different threads.

telemetry API #271

telemetry API #271

Uh oh!

Conversation

TingDaoK commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

graebm left a comment

Choose a reason for hiding this comment

TingDaoK commented Mar 29, 2023 •

edited

Loading

codecov-commenter commented Mar 31, 2023 •

edited

Loading

TingDaoK May 10, 2023 •

edited

Loading