Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add request type to telemetry #287

Merged
merged 10 commits into from
May 23, 2023
Merged

Add request type to telemetry #287

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1819036
add request type
TingDaoK May 12, 2023
b9a22ec
bad copy/past
TingDaoK May 12, 2023
62f9fa9
add copy as well, even though not used.
TingDaoK May 12, 2023
f6859c7
use the public api name
TingDaoK May 16, 2023
364d633
Merge branch 'main' into add-request-type
TingDaoK May 16, 2023
1112b26
fix error
TingDaoK May 16, 2023
d906b72
use s3 api name completely
TingDaoK May 18, 2023
0a42947
it's `uploadPartCopy` instead
TingDaoK May 22, 2023
58fd4db
Merge branch 'main' into add-request-type
TingDaoK May 22, 2023
a2a0159
Merge branch 'main' into add-request-type
TingDaoK May 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions include/aws/s3/private/s3_meta_request_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ struct aws_s3_meta_request_vtable {

/* Pause the given request */
int (*pause)(struct aws_s3_meta_request *meta_request, struct aws_s3_meta_request_resume_token **resume_token);

/* Get the type of the aws_s3_request */
int (*get_request_type)(struct aws_s3_request *request);
};

/**
Expand Down
2 changes: 2 additions & 0 deletions include/aws/s3/private/s3_request.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ struct aws_s3_request_metrics {
struct aws_string *host_address;
/* The the request ID header value. */
struct aws_string *request_id;
/* The type of request made */
enum aws_s3_request_type request_type;
} req_resp_info_metrics;

struct {
Expand Down
38 changes: 37 additions & 1 deletion include/aws/s3/s3_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,36 @@ enum aws_s3_meta_request_type {
AWS_S3_META_REQUEST_TYPE_MAX,
};

/**
* The type of S3 request made. Used by metrics.
*/
enum aws_s3_request_type {

/* Auto Ranged PUT request types */
AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_LIST_PARTS,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe simply use the S3 API name.

I'm not sure it's a great idea to use our private subclass names as part of the public API. Frees us up to change the names of things, or move to some other design scheme (break up subclasses differently, change to some other design pattern, etc)

Suggested change
AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_LIST_PARTS,
AWS_S3_REQUEST_TYPE_LIST_PARTS,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure how this data is going to be used.
Would it be better to just report a string in the metrics, instead of an enum?
Would it be OK if the string changed in some future version?
Is it better that we indicate what's going on under the hood, which is liable to change. Or is it better to be stable in what we report, and keep out details that might change in the future?

I don't know. maybe talk with someone who's thinking about using this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure as well. I did this mainly because:

  • I think we already expose the metrics as some inspect of the internal implementation. So, those private objects are as well part of the inspect of how we actually doing the job under the hood
  • We are not matching the S3 api one by one. Like get the object size from copy, get object part, which is a ranged get, etc?

BUT, I think it makes more sense to keep public stuff align with the public doc from S3, and user knows the meta request were made. So, it's okay if copy/put shared the same API calls. And, keep us free to change in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The S3 API name is all we really want to know. We already know what type of meta request it is from the aws_s3_meta_request pointer the telemetry callback gets; we just don't have a way to distinguish e.g. HeadObject and GetObject calls in auto_ranged_get (so we can report on their latency separately).

AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_CREATE_MULTIPART_UPLOAD,
AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_PART,
AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_ABORT_MULTIPART_UPLOAD,
AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_COMPLETE_MULTIPART_UPLOAD,

/* Auto Ranged GET request types */
AWS_S3_REQUEST_TYPE_AUTO_RANGE_GET_HEAD_OBJECT,
TingDaoK marked this conversation as resolved.
Show resolved Hide resolved
AWS_S3_REQUEST_TYPE_AUTO_RANGE_GET_PART,
AWS_S3_REQUEST_TYPE_AUTO_RANGE_GET_INITIAL_MESSAGE,
TingDaoK marked this conversation as resolved.
Show resolved Hide resolved

/* The default type. Same as default meta request */
AWS_S3_REQUEST_TYPE_DEFAULT,
TingDaoK marked this conversation as resolved.
Show resolved Hide resolved

/* Copy Object types */
AWS_S3_REQUEST_TYPE_COPY_OBJECT_GET_OBJECT_SIZE,
AWS_S3_REQUEST_TYPE_COPY_OBJECT_CREATE_MULTIPART_UPLOAD,
AWS_S3_REQUEST_TYPE_COPY_OBJECT_MULTIPART_COPY,
AWS_S3_REQUEST_TYPE_COPY_OBJECT_ABORT_MULTIPART_UPLOAD,
AWS_S3_REQUEST_TYPE_COPY_OBJECT_COMPLETE_MULTIPART_UPLOAD,

AWS_S3_REQUEST_TYPE_MAX,
};

/**
* Invoked to provide response headers received during execution of the meta request, both for
* success and error HTTP status codes.
Expand Down Expand Up @@ -846,9 +876,15 @@ int aws_s3_request_metrics_get_thread_id(const struct aws_s3_request_metrics *me
AWS_S3_API
int aws_s3_request_metrics_get_request_stream_id(const struct aws_s3_request_metrics *metrics, uint32_t *out_stream_id);

/* Get the request type from request metrics. */
AWS_S3_API
void aws_s3_request_metrics_get_request_type(
const struct aws_s3_request_metrics *metrics,
enum aws_s3_request_type *out_request_type);

/* Get the AWS CRT error code from request metrics. */
AWS_S3_API
int aws_s3_request_metrics_get_error_code(const struct aws_s3_request_metrics *out_metrics);
int aws_s3_request_metrics_get_error_code(const struct aws_s3_request_metrics *metrics);

AWS_EXTERN_C_END

Expand Down
15 changes: 15 additions & 0 deletions source/s3_auto_ranged_get.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ static void s_s3_auto_ranged_get_request_finished(
struct aws_s3_request *request,
int error_code);

static int s_s3_auto_ranged_get_request_type(struct aws_s3_request *request);

static struct aws_s3_meta_request_vtable s_s3_auto_ranged_get_vtable = {
.update = s_s3_auto_ranged_get_update,
.send_request_finish = aws_s3_meta_request_send_request_finish_default,
Expand All @@ -45,8 +47,21 @@ static struct aws_s3_meta_request_vtable s_s3_auto_ranged_get_vtable = {
.finished_request = s_s3_auto_ranged_get_request_finished,
.destroy = s_s3_meta_request_auto_ranged_get_destroy,
.finish = aws_s3_meta_request_finish_default,
.get_request_type = s_s3_auto_ranged_get_request_type,
};

static int s_s3_auto_ranged_get_request_type(struct aws_s3_request *request) {
switch (request->request_tag) {
case AWS_S3_AUTO_RANGE_GET_REQUEST_TYPE_HEAD_OBJECT:
return AWS_S3_REQUEST_TYPE_AUTO_RANGE_GET_HEAD_OBJECT;
case AWS_S3_AUTO_RANGE_GET_REQUEST_TYPE_PART:
case AWS_S3_AUTO_RANGE_GET_REQUEST_TYPE_INITIAL_MESSAGE:
return AWS_S3_REQUEST_TYPE_AUTO_RANGE_GET_PART;
}
AWS_ASSERT(false);
return AWS_S3_REQUEST_TYPE_MAX;
}

static int s_s3_auto_ranged_get_success_status(struct aws_s3_meta_request *meta_request) {
AWS_PRECONDITION(meta_request);

Expand Down
18 changes: 18 additions & 0 deletions source/s3_auto_ranged_put.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,23 @@ static int s_try_init_resume_state_from_persisted_data(
return AWS_OP_SUCCESS;
}

static int s_s3_auto_ranged_put_request_type(struct aws_s3_request *request) {
switch (request->request_tag) {
case AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_LIST_PARTS:
return AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_LIST_PARTS;
case AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_CREATE_MULTIPART_UPLOAD:
return AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_CREATE_MULTIPART_UPLOAD;
case AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_PART:
return AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_PART;
case AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_ABORT_MULTIPART_UPLOAD:
return AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_ABORT_MULTIPART_UPLOAD;
case AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_COMPLETE_MULTIPART_UPLOAD:
return AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_COMPLETE_MULTIPART_UPLOAD;
}
AWS_ASSERT(false);
return AWS_S3_REQUEST_TYPE_MAX;
}

static struct aws_s3_meta_request_vtable s_s3_auto_ranged_put_vtable = {
.update = s_s3_auto_ranged_put_update,
.send_request_finish = s_s3_auto_ranged_put_send_request_finish,
Expand All @@ -221,6 +238,7 @@ static struct aws_s3_meta_request_vtable s_s3_auto_ranged_put_vtable = {
.destroy = s_s3_meta_request_auto_ranged_put_destroy,
.finish = aws_s3_meta_request_finish_default,
.pause = s_s3_auto_ranged_put_pause,
.get_request_type = s_s3_auto_ranged_put_request_type,
};

/* Allocate a new auto-ranged put meta request */
Expand Down
21 changes: 21 additions & 0 deletions source/s3_copy_object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,26 @@ static void s_s3_copy_object_request_finished(
struct aws_s3_request *request,
int error_code);

static int s_s3_copy_object_request_type(struct aws_s3_request *request) {
switch (request->request_tag) {
case AWS_S3_COPY_OBJECT_REQUEST_TAG_GET_OBJECT_SIZE:
return AWS_S3_REQUEST_TYPE_COPY_OBJECT_GET_OBJECT_SIZE;
case AWS_S3_COPY_OBJECT_REQUEST_TAG_BYPASS:
/* A single copy object request, same as default */
return AWS_S3_REQUEST_TYPE_DEFAULT;
case AWS_S3_COPY_OBJECT_REQUEST_TAG_CREATE_MULTIPART_UPLOAD:
return AWS_S3_REQUEST_TYPE_COPY_OBJECT_CREATE_MULTIPART_UPLOAD;
case AWS_S3_COPY_OBJECT_REQUEST_TAG_MULTIPART_COPY:
return AWS_S3_REQUEST_TYPE_COPY_OBJECT_MULTIPART_COPY;
case AWS_S3_COPY_OBJECT_REQUEST_TAG_ABORT_MULTIPART_UPLOAD:
return AWS_S3_REQUEST_TYPE_COPY_OBJECT_ABORT_MULTIPART_UPLOAD;
case AWS_S3_COPY_OBJECT_REQUEST_TAG_COMPLETE_MULTIPART_UPLOAD:
return AWS_S3_REQUEST_TYPE_COPY_OBJECT_COMPLETE_MULTIPART_UPLOAD;
}
AWS_ASSERT(false);
return AWS_S3_REQUEST_TYPE_MAX;
}

static struct aws_s3_meta_request_vtable s_s3_copy_object_vtable = {
.update = s_s3_copy_object_update,
.send_request_finish = aws_s3_meta_request_send_request_finish_handle_async_error,
Expand All @@ -46,6 +66,7 @@ static struct aws_s3_meta_request_vtable s_s3_copy_object_vtable = {
.finished_request = s_s3_copy_object_request_finished,
.destroy = s_s3_meta_request_copy_object_destroy,
.finish = aws_s3_meta_request_finish_default,
.get_request_type = s_s3_copy_object_request_type,
};

/* Allocate a new copy object meta request */
Expand Down
14 changes: 14 additions & 0 deletions source/s3_request.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ void aws_s3_request_setup_send_data(struct aws_s3_request *request, struct aws_h
if (meta_request->telemetry_callback) {
/* start the telemetry for the request to be sent */
request->send_data.metrics = aws_s3_request_metrics_new(request->allocator, message);
if (!meta_request->vtable->get_request_type) {
request->send_data.metrics->req_resp_info_metrics.request_type = AWS_S3_REQUEST_TYPE_DEFAULT;
} else {
request->send_data.metrics->req_resp_info_metrics.request_type =
meta_request->vtable->get_request_type(request);
}
/* Start the timestamp */
aws_high_res_clock_get_ticks((uint64_t *)&request->send_data.metrics->time_metrics.start_timestamp_ns);
}
Expand Down Expand Up @@ -379,6 +385,14 @@ int aws_s3_request_metrics_get_request_stream_id(const struct aws_s3_request_met
return AWS_OP_SUCCESS;
}

void aws_s3_request_metrics_get_request_type(
const struct aws_s3_request_metrics *metrics,
enum aws_s3_request_type *out_request_type) {
AWS_PRECONDITION(metrics);
AWS_PRECONDITION(out_request_type);
*out_request_type = metrics->req_resp_info_metrics.request_type;
}

int aws_s3_request_metrics_get_error_code(const struct aws_s3_request_metrics *metrics) {
AWS_PRECONDITION(metrics);
return metrics->crt_info_metrics.error_code;
Expand Down
18 changes: 18 additions & 0 deletions tests/s3_mock_server_tests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ static int s_validate_mpu_mock_server_metrics(struct aws_array_list *metrics_lis
ASSERT_SUCCESS(aws_s3_request_metrics_get_receiving_duration_ns(metrics, &time_stamp));
ASSERT_FALSE(time_stamp == 0);
time_stamp = 0;
enum aws_s3_request_type request_type = 0;
aws_s3_request_metrics_get_request_type(metrics, &request_type);
ASSERT_UINT_EQUALS(AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_CREATE_MULTIPART_UPLOAD, request_type);

/* Second metrics should be the Upload Part */
aws_array_list_get_at(metrics_list, (void **)&metrics, 1);
Expand All @@ -92,6 +95,21 @@ static int s_validate_mpu_mock_server_metrics(struct aws_array_list *metrics_lis
ASSERT_TRUE(aws_byte_cursor_eq_c_str(&header_value, "b54357faf0632cce46e942fa68356b38"));
ASSERT_SUCCESS(aws_http_headers_get(response_headers, aws_byte_cursor_from_c_str("Connection"), &header_value));
ASSERT_TRUE(aws_byte_cursor_eq_c_str(&header_value, "keep-alive"));
request_type = 0;
aws_s3_request_metrics_get_request_type(metrics, &request_type);
ASSERT_UINT_EQUALS(AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_PART, request_type);

/* Third metrics still be Upload Part */
aws_array_list_get_at(metrics_list, (void **)&metrics, 2);
request_type = 0;
aws_s3_request_metrics_get_request_type(metrics, &request_type);
ASSERT_UINT_EQUALS(AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_PART, request_type);

/* Fourth should be complete MPU */
aws_array_list_get_at(metrics_list, (void **)&metrics, 3);
request_type = 0;
aws_s3_request_metrics_get_request_type(metrics, &request_type);
ASSERT_UINT_EQUALS(AWS_S3_REQUEST_TYPE_AUTO_RANGED_PUT_COMPLETE_MULTIPART_UPLOAD, request_type);
/* All the rest should be similar */

return AWS_OP_SUCCESS;
Expand Down