Skip to content

Commit

Permalink
Report S3 operation name of specific request that failed. (#377)
Browse files Browse the repository at this point in the history
**Issue:**
It was difficult to map errors from aws-c-s3 into errors in other AWS SDKs without knowing exactly which HTTP request failed.

For example, if `AWS_S3_META_REQUEST_TYPE_PUT_OBJECT` fails, they need to know whether it was a "PutObject", "CreateMultipartUpload", "UploadPart", or "CompleteMultipartUpload" HTTP request that failed.

**Description of changes:**
- Report the S3 operation name of the specific HTTP request that failed (e.g. "CreateMultipartUpload").
- Likewise, report that operation name in metrics.
- For `AWS_S3_META_REQUEST_TYPE_DEFAULT`, where aws-c-s3 isn't sure what operation is being performed, allow the user to pass in the operation name.
- Internally, pass request_type and operation_name around directly. Instead of querying them via vtable gymnastics, which I found confusing and error-prone.

Co-authored-by: Waqar Ahmed Khan <[email protected]>
  • Loading branch information
graebm and waahm7 authored Nov 22, 2023
1 parent 7192ab5 commit cc6ba34
Show file tree
Hide file tree
Showing 18 changed files with 358 additions and 117 deletions.
7 changes: 7 additions & 0 deletions include/aws/s3/private/s3_default_meta_request.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ struct aws_s3_meta_request_default {

size_t content_length;

/* Actual type for the single request (may be AWS_S3_REQUEST_TYPE_UNKNOWN) */
enum aws_s3_request_type request_type;

/* S3 operation name for the single request (NULL if unknown) */
struct aws_string *operation_name;

/* Members to only be used when the mutex in the base type is locked. */
struct {
int cached_response_status;
Expand All @@ -30,6 +36,7 @@ struct aws_s3_meta_request_default {
struct aws_s3_meta_request *aws_s3_meta_request_default_new(
struct aws_allocator *allocator,
struct aws_s3_client *client,
enum aws_s3_request_type request_type,
uint64_t content_length,
bool should_compute_content_md5,
const struct aws_s3_meta_request_options *options);
Expand Down
3 changes: 0 additions & 3 deletions include/aws/s3/private/s3_meta_request_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,6 @@ 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)(const struct aws_s3_request *request);
};

/**
Expand Down
17 changes: 15 additions & 2 deletions include/aws/s3/private/s3_request.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ struct aws_s3_request_metrics {
struct aws_string *host_address;
/* The the request ID header value. */
struct aws_string *request_id;
/* S3 operation name for the request (NULL if unknown) */
struct aws_string *operation_name;
/* The type of request made */
enum aws_s3_request_type request_type;
} req_resp_info_metrics;
Expand Down Expand Up @@ -155,9 +157,18 @@ struct aws_s3_request {
* by the derived type. Request tags do not necessarily map 1:1 with actual S3 API requests. (For example, they can
* be more contextual, like "first part" instead of just "part".) */

/* TODO: this should be a union type to make it clear that this could be one of two enums for puts, and gets. */
/* TODO: Eliminate the concept of "request tag" and just use request_type.
* It's confusing having 2 concepts that are so similar.
* There's only 1 case where 2 tags used the same type,
* we can use some other bool/flag to differentiate this 1 case. */
int request_tag;

/* Actual S3 type for the single request (may be AWS_S3_REQUEST_TYPE_UNKNOWN) */
enum aws_s3_request_type request_type;

/* S3 operation name for the single request (e.g. "CompleteMultipartUpload") (NULL if unknown) */
struct aws_string *operation_name;

/* Members of this structure will be repopulated each time the request is sent. If the request fails, and needs to
* be retried, then the members of this structure will be cleaned up and re-populated on the next send.
*/
Expand Down Expand Up @@ -224,6 +235,7 @@ AWS_S3_API
struct aws_s3_request *aws_s3_request_new(
struct aws_s3_meta_request *meta_request,
int request_tag,
enum aws_s3_request_type request_type,
uint32_t part_number,
uint32_t flags);

Expand All @@ -245,7 +257,8 @@ struct aws_s3_request *aws_s3_request_release(struct aws_s3_request *request);
AWS_S3_API
struct aws_s3_request_metrics *aws_s3_request_metrics_new(
struct aws_allocator *allocator,
struct aws_http_message *message);
const struct aws_s3_request *request,
const struct aws_http_message *message);

AWS_EXTERN_C_END

Expand Down
71 changes: 65 additions & 6 deletions include/aws/s3/s3_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,21 @@ enum aws_s3_meta_request_type {
};

/**
* The type of S3 request made. Used by metrics.
* The type of a single S3 HTTP request. Used by metrics.
* A meta-request can make multiple S3 HTTP requests under the hood.
*
* For example, AWS_S3_META_REQUEST_TYPE_PUT_OBJECT for a large file will
* do multipart upload, resulting in 3+ HTTP requests:
* AWS_S3_REQUEST_TYPE_CREATE_MULTIPART_UPLOAD, one or more AWS_S3_REQUEST_TYPE_UPLOAD_PART,
* and finally AWS_S3_REQUEST_TYPE_COMPLETE_MULTIPART_UPLOAD.
*
* aws_s3_request_type_operation_name() returns the S3 operation name
* for types that map (e.g. AWS_S3_REQUEST_TYPE_HEAD_OBJECT -> "HeadObject"),
* or empty string for types that don't map (e.g. AWS_S3_REQUEST_TYPE_UNKNOWN -> "").
*/
enum aws_s3_request_type {
/* Same as the original HTTP request passed to aws_s3_meta_request_options */
AWS_S3_REQUEST_TYPE_DEFAULT,
/* The actual type of the single S3 HTTP request is unknown */
AWS_S3_REQUEST_TYPE_UNKNOWN,

/* S3 APIs */
AWS_S3_REQUEST_TYPE_HEAD_OBJECT,
Expand All @@ -100,8 +110,14 @@ enum aws_s3_request_type {
AWS_S3_REQUEST_TYPE_ABORT_MULTIPART_UPLOAD,
AWS_S3_REQUEST_TYPE_COMPLETE_MULTIPART_UPLOAD,
AWS_S3_REQUEST_TYPE_UPLOAD_PART_COPY,
AWS_S3_REQUEST_TYPE_COPY_OBJECT,
AWS_S3_REQUEST_TYPE_PUT_OBJECT,

/* Max enum value */
AWS_S3_REQUEST_TYPE_MAX,

/** @deprecated Use AWS_S3_REQUEST_TYPE_UNKNOWN if the actual S3 HTTP request type is unknown */
AWS_S3_REQUEST_TYPE_DEFAULT = AWS_S3_REQUEST_TYPE_UNKNOWN,
};

/**
Expand Down Expand Up @@ -481,6 +497,15 @@ struct aws_s3_meta_request_options {
/* The type of meta request we will be trying to accelerate. */
enum aws_s3_meta_request_type type;

/**
* Optional.
* The S3 operation name (e.g. "CreateBucket").
* This will only be used when type is AWS_S3_META_REQUEST_TYPE_DEFAULT;
* it is automatically populated for other meta-request types.
* This name is used to fill out details in metrics and error reports.
*/
struct aws_byte_cursor operation_name;

/* Signing options to be used for each request created for this meta request. If NULL, options in the client will
* be used. If not NULL, these options will override the client options. */
const struct aws_signing_config_aws *signing_config;
Expand Down Expand Up @@ -602,12 +627,23 @@ struct aws_s3_meta_request_options {
*/
struct aws_s3_meta_request_result {

/* HTTP Headers for the failed request that triggered finish of the meta request. NULL if no request failed. */
/* If meta request failed due to an HTTP error response from S3, these are the headers.
* NULL if meta request failed for another reason. */
struct aws_http_headers *error_response_headers;

/* Response body for the failed request that triggered finishing of the meta request. NULL if no request failed.*/
/* If meta request failed due to an HTTP error response from S3, this the body.
* NULL if meta request failed for another reason, or if the response had no body (such as a HEAD response). */
struct aws_byte_buf *error_response_body;

/* If meta request failed due to an HTTP error response from S3,
* this is the name of the S3 operation it was responding to.
* For example, if a AWS_S3_META_REQUEST_TYPE_PUT_OBJECT fails this could be
* "PutObject, "CreateMultipartUpload", "UploadPart", "CompleteMultipartUpload", or others.
* For AWS_S3_META_REQUEST_TYPE_DEFAULT, this is the same value passed to
* aws_s3_meta_request_options.operation_name.
* NULL if the meta request failed for another reason, or the operation name is not known. */
struct aws_string *error_response_operation_name;

/* Response status of the failed request or of the entire meta request. */
int response_status;

Expand Down Expand Up @@ -823,6 +859,17 @@ void aws_s3_init_default_signing_config(
const struct aws_byte_cursor region,
struct aws_credentials_provider *credentials_provider);

/**
* Return operation name for aws_s3_request_type,
* or empty string if the type doesn't map to an actual operation.
* For example:
* AWS_S3_REQUEST_TYPE_HEAD_OBJECT -> "HeadObject"
* AWS_S3_REQUEST_TYPE_UNKNOWN -> ""
* AWS_S3_REQUEST_TYPE_MAX -> ""
*/
AWS_S3_API
const char *aws_s3_request_type_operation_name(enum aws_s3_request_type type);

/**
* Add a reference, keeping this object alive.
* The reference must be released when you are done with it, or it's memory will never be cleaned up.
Expand Down Expand Up @@ -974,7 +1021,19 @@ 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. */
/**
* Get the S3 operation name of the request (e.g. "HeadObject").
* If unavailable, AWS_ERROR_S3_METRIC_DATA_NOT_AVAILABLE will be raised.
* If available, out_operation_name 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_operation_name(
const struct aws_s3_request_metrics *metrics,
const struct aws_string **out_operation_name);

/* Get the request type from request metrics.
* If you just need a string, aws_s3_request_metrics_get_operation_name() is more reliable. */
AWS_S3_API
void aws_s3_request_metrics_get_request_type(
const struct aws_s3_request_metrics *metrics,
Expand Down
27 changes: 8 additions & 19 deletions source/s3_auto_ranged_get.c
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ 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(const 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 @@ -39,21 +37,8 @@ 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(const struct aws_s3_request *request) {
switch (request->request_tag) {
case AWS_S3_AUTO_RANGE_GET_REQUEST_TYPE_HEAD_OBJECT:
return AWS_S3_REQUEST_TYPE_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_GET_OBJECT;
}
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 Expand Up @@ -176,7 +161,8 @@ static bool s_s3_auto_ranged_get_update(
request = aws_s3_request_new(
meta_request,
AWS_S3_AUTO_RANGE_GET_REQUEST_TYPE_HEAD_OBJECT,
0,
AWS_S3_REQUEST_TYPE_HEAD_OBJECT,
0 /*part_number*/,
AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS);

request->discovers_object_size = true;
Expand All @@ -197,7 +183,8 @@ static bool s_s3_auto_ranged_get_update(
request = aws_s3_request_new(
meta_request,
AWS_S3_AUTO_RANGE_GET_REQUEST_TYPE_PART,
1,
AWS_S3_REQUEST_TYPE_GET_OBJECT,
1 /*part_number*/,
AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS | AWS_S3_REQUEST_FLAG_PART_SIZE_RESPONSE_BODY);

request->ticket = ticket;
Expand All @@ -223,7 +210,8 @@ static bool s_s3_auto_ranged_get_update(
request = aws_s3_request_new(
meta_request,
AWS_S3_AUTO_RANGE_GET_REQUEST_TYPE_INITIAL_MESSAGE,
0,
AWS_S3_REQUEST_TYPE_GET_OBJECT,
0 /*part_number*/,
AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS);

auto_ranged_get->synced_data.get_without_range_sent = true;
Expand Down Expand Up @@ -272,7 +260,8 @@ static bool s_s3_auto_ranged_get_update(
request = aws_s3_request_new(
meta_request,
AWS_S3_AUTO_RANGE_GET_REQUEST_TYPE_PART,
auto_ranged_get->synced_data.num_parts_requested + 1,
AWS_S3_REQUEST_TYPE_GET_OBJECT,
auto_ranged_get->synced_data.num_parts_requested + 1 /*part_number*/,
AWS_S3_REQUEST_FLAG_PART_SIZE_RESPONSE_BODY);

request->ticket = ticket;
Expand Down
36 changes: 12 additions & 24 deletions source/s3_auto_ranged_put.c
Original file line number Diff line number Diff line change
Expand Up @@ -294,23 +294,6 @@ static int s_try_init_resume_state_from_persisted_data(
return AWS_OP_SUCCESS;
}

static int s_s3_auto_ranged_put_request_type(const struct aws_s3_request *request) {
switch (request->request_tag) {
case AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_LIST_PARTS:
return AWS_S3_REQUEST_TYPE_LIST_PARTS;
case AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_CREATE_MULTIPART_UPLOAD:
return AWS_S3_REQUEST_TYPE_CREATE_MULTIPART_UPLOAD;
case AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_PART:
return AWS_S3_REQUEST_TYPE_UPLOAD_PART;
case AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_ABORT_MULTIPART_UPLOAD:
return AWS_S3_REQUEST_TYPE_ABORT_MULTIPART_UPLOAD;
case AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_COMPLETE_MULTIPART_UPLOAD:
return AWS_S3_REQUEST_TYPE_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 @@ -321,7 +304,6 @@ 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 Expand Up @@ -480,7 +462,8 @@ static bool s_s3_auto_ranged_put_update(
request = aws_s3_request_new(
meta_request,
AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_LIST_PARTS,
0,
AWS_S3_REQUEST_TYPE_LIST_PARTS,
0 /*part_number*/,
AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS);

auto_ranged_put->synced_data.list_parts_state.started = true;
Expand All @@ -494,7 +477,8 @@ static bool s_s3_auto_ranged_put_update(
request = aws_s3_request_new(
meta_request,
AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_LIST_PARTS,
0,
AWS_S3_REQUEST_TYPE_LIST_PARTS,
0 /*part_number*/,
AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS);
auto_ranged_put->synced_data.list_parts_state.continues = false;
goto has_work_remaining;
Expand All @@ -510,7 +494,8 @@ static bool s_s3_auto_ranged_put_update(
request = aws_s3_request_new(
meta_request,
AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_CREATE_MULTIPART_UPLOAD,
0,
AWS_S3_REQUEST_TYPE_CREATE_MULTIPART_UPLOAD,
0 /*part_number*/,
AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS);

auto_ranged_put->synced_data.create_multipart_upload_sent = true;
Expand Down Expand Up @@ -564,7 +549,8 @@ static bool s_s3_auto_ranged_put_update(
request = aws_s3_request_new(
meta_request,
AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_PART,
0,
AWS_S3_REQUEST_TYPE_UPLOAD_PART,
0 /*part_number*/,
AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS | AWS_S3_REQUEST_FLAG_PART_SIZE_REQUEST_BODY);

request->part_number = auto_ranged_put->threaded_update_data.next_part_number;
Expand Down Expand Up @@ -610,7 +596,8 @@ static bool s_s3_auto_ranged_put_update(
request = aws_s3_request_new(
meta_request,
AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_COMPLETE_MULTIPART_UPLOAD,
0,
AWS_S3_REQUEST_TYPE_COMPLETE_MULTIPART_UPLOAD,
0 /*part_number*/,
AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS);

auto_ranged_put->synced_data.complete_multipart_upload_sent = true;
Expand Down Expand Up @@ -675,7 +662,8 @@ static bool s_s3_auto_ranged_put_update(
request = aws_s3_request_new(
meta_request,
AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_ABORT_MULTIPART_UPLOAD,
0,
AWS_S3_REQUEST_TYPE_ABORT_MULTIPART_UPLOAD,
0 /*part_number*/,
AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS | AWS_S3_REQUEST_FLAG_ALWAYS_SEND);

auto_ranged_put->synced_data.abort_multipart_upload_sent = true;
Expand Down
17 changes: 15 additions & 2 deletions source/s3_client.c
Original file line number Diff line number Diff line change
Expand Up @@ -1043,7 +1043,13 @@ static struct aws_s3_meta_request *s_s3_client_meta_request_factory_default(
* TODO: Still need tests to verify that the request of a part is
* splittable or not */
if (aws_http_headers_has(initial_message_headers, aws_byte_cursor_from_c_str("partNumber"))) {
return aws_s3_meta_request_default_new(client->allocator, client, content_length, false, options);
return aws_s3_meta_request_default_new(
client->allocator,
client,
AWS_S3_REQUEST_TYPE_GET_OBJECT,
content_length,
false /*should_compute_content_md5*/,
options);
}

return aws_s3_meta_request_auto_ranged_get_new(client->allocator, client, client->part_size, options);
Expand Down Expand Up @@ -1092,6 +1098,7 @@ static struct aws_s3_meta_request *s_s3_client_meta_request_factory_default(
return aws_s3_meta_request_default_new(
client->allocator,
client,
AWS_S3_REQUEST_TYPE_PUT_OBJECT,
content_length,
client->compute_content_md5 == AWS_MR_CONTENT_MD5_ENABLED &&
!aws_http_headers_has(initial_message_headers, g_content_md5_header_name),
Expand Down Expand Up @@ -1138,7 +1145,13 @@ static struct aws_s3_meta_request *s_s3_client_meta_request_factory_default(
return aws_s3_meta_request_copy_object_new(client->allocator, client, options);
}
case AWS_S3_META_REQUEST_TYPE_DEFAULT:
return aws_s3_meta_request_default_new(client->allocator, client, content_length, false, options);
return aws_s3_meta_request_default_new(
client->allocator,
client,
AWS_S3_REQUEST_TYPE_UNKNOWN,
content_length,
false /*should_compute_content_md5*/,
options);
default:
AWS_FATAL_ASSERT(false);
}
Expand Down
Loading

0 comments on commit cc6ba34

Please sign in to comment.