Lower log level for S3CrtClient metareq response err from warn to debug

[dannycjones]

We currently log at warn for any request returning an Err from CRT, which includes 4XX errors. This causes logs to be spammed with warnings for objects that do not exist.

Instead, let's log these at debug. Note that this will drop others like redirects (301) from warn also.

2023-04-11T09:20:25.877698Z  WARN mountpoint_s3_client::s3_crt_client: request failed request_id="J589XM24EK4HZ8TB" duration_us=584035 request_result=MetaRequestResult { response_status: 301, crt_error: Error(14343, "aws-c-s3: AWS_ERROR_S3_INVALID_RESPONSE_STATUS, Invalid response status from request"), error_response_headers: Some(Headers { inner: 0x7fc682407fa0 }), error_response_body: None }
2023-04-11T09:20:27.902899Z  WARN mountpoint_s3_client::s3_crt_client: request failed request_id="2F77Q3SG65HMCB5E" duration_us=244622 request_result=MetaRequestResult { response_status: 404, crt_error: Error(14343, "aws-c-s3: AWS_ERROR_S3_INVALID_RESPONSE_STATUS, Invalid response status from request"), error_response_headers: Some(Headers { inner: 0x7fc682304080 }), error_response_body: None }
2023-04-11T09:20:28.407394Z  WARN mountpoint_s3_client::s3_crt_client: request failed request_id="RWGJK0XXC80X9R3X" duration_us=318581 request_result=MetaRequestResult { response_status: 404, crt_error: Error(14343, "aws-c-s3: AWS_ERROR_S3_INVALID_RESPONSE_STATUS, Invalid response status from request"), error_response_headers: Some(Headers { inner: 0x7fc68230e610 }), error_response_body: None }

Here's the original warning for unauthenticated request as well, which would now be debug:

2023-04-11T09:20:10.327379Z  WARN mountpoint_s3_client::s3_crt_client: request failed request_id="22VSQ3XFZEX7K9QY" duration_us=389885 request_result=MetaRequestResult { response_status: 400, crt_error: Error(14343, "aws-c-s3: AWS_ERROR_S3_INVALID_RESPONSE_STATUS, Invalid response status from request"), error_response_headers: Some(Headers { inner: 0x7f86bb6119c0 }), error_response_body: None }

Note that when the connector fails to start, it will return the error reasons as usual:

Error: Failed to create S3 client

Caused by:
    0: HeadBucket failed for bucket djonesoa-dub in region eu-west-1
    1: Service error
    2: Permission denied

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

[jamesbornholt]

I'm not totally clear on whether this is the right thing to do. Probably instead we should log warnings (or even errors) for 5xx responses, and debug for all other outcomes (2xx, 3xx, 4xx, except 429 which is an error if it ever gets returned because it means retries have failed)? Or maybe debug for 2xx, 3xx, and 404, and error for everything else, since 400, 403 etc are failures too.

[dannycjones]

I'm not totally clear on whether this is the right thing to do. Probably instead we should log warnings (or even errors) for 5xx responses, and debug for all other outcomes (2xx, 3xx, 4xx, except 429 which is an error if it ever gets returned because it means retries have failed)? Or maybe debug for 2xx, 3xx, and 404, and error for everything else, since 400, 403 etc are failures too.

I think that makes sense. I also considered this, but was a bit wary of adding complexity. However, if we just know we probably don't care about 2xx, 3xx, 404, then that works.

Will push an update tomorrow.

[dannycjones]

Actually, still unsure. I'll think about it a bit more tomorrow.

The fact that we don't care is both Mountpoint not caring but also a question of should the S3 CRT Client (which is where this log lives) care about the status code when logging this?

[jamesbornholt]

I think "different HTTP status codes have different severity" is probably generic enough that it's fine being in the CRT client. Consumers of the client can obviously still match on the errors themselves if they want to treat them differently.

[dannycjones]

This PR was already merged, so I've implemented what we discussed in #205.