Report S3 operation name of specific request that failed. #377
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #377 +/- ##
==========================================
+ Coverage 88.55% 88.64% +0.08%
==========================================
Files 20 20
Lines 5348 5380 +32
==========================================
+ Hits 4736 4769 +33
+ Misses 612 611 -1
|
also, whoopsie fix memory leak
| 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. */ | ||
| AWS_S3_API | ||
| void aws_s3_request_metrics_get_request_type( |
There was a problem hiding this comment.
IIRC we added this function and all the aws_s3_request_type stuff specifically for Mountpoint for use in metrics. If Mountpoint is still the only customer for it and you feel like you want to break it, it would be really easy for us to switch to this new interface instead, since we really only needed it to get operation names anyway.
There was a problem hiding this comment.
I'll keep the enums. I'm finding uses for both enum and string
…them via vtable calls. In my opinion, the previous approach was complicated, hard to use, and easy to mess up. I didn't realize we weren't handling the case where singlepart operations use DEFAULT request type under the hood. Now our code MUST declare request-type and operation-name up front. It's obvious. It works. Yay.
Instead of weird inconsistency storing NULL for unknown aws_string, but "" for unknown c-string.
source/s3_auto_ranged_put.c
Outdated
| @@ -510,7 +496,9 @@ 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, | |||
| "CreateMultipartUpload", | |||
There was a problem hiding this comment.
debatable: I would prefer to have a global static string for the operation names we set (I know you are not a fan of those).
- In case the name changed from the model, it's easier to us to update
- multiple places sharing the same string.
There was a problem hiding this comment.
I eliminated pretty much all the repeatedly typed strings
Still not using global static variables 😛
In the 1 case where the name differed from the type, we'll just set that afterwards.
Co-authored-by: Waqar Ahmed Khan <waahm7@gmail.com>
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_OBJECTfails, they need to know whether it was a "PutObject", "CreateMultipartUpload", "UploadPart", or "CompleteMultipartUpload" HTTP request that failed.Description of changes:
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.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.