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.

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

feat(sdkv3): centralize and improve client logging and error handling. #6780

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Mar 12, 2025

Problem

  • This PR introduced a logging regression by removing headers and performance based logging. (Additional Context: https://github.com/aws/aws-toolkit-vscode/pull/6734/files#r1989725353)
  • logging middleware is implemented in two places, once in the generic client builder, and once specifically for SSO.
  • CodeCatalyst call functionality should be available to other clients. call logged performance and header information on failures, and allowed for silent failure with fallback values.
  • Error codes are inconsistent from SDKv3. <Error>.code is no longer reliably defined.

Solution

  • Unify middleware definitions between generic and SSO client. Ideally, SSO client would use the generic client builder. However, much of this code will likely be migrated to LSP soon.
  • Generalize old call implementation in CodeCatalyst to live within makeRequest, such that failures in the generic ClientWrapper log performance and allow fallback values.
  • Attempt to extract error codes from different fields in SDK error. We try code, then Code, then fallback to name. Noticed some errors included Code, but not all.
  • Add headers logging in middleware at finalize step (so that any headers added along the way are included).
  • On error, log warning with the error from SDK, then log an error if no fallback value was provided.

Examples

On successful api request logs:

025-03-14 08:24:57.178 [debug] API Request (ec2.us-west-2.amazonaws.com /):
 headers: { Connection: 'keep-alive', host: 'ec2.us-west-2.amazonaws.com' }
 input: { NextToken: undefined, MaxResults: undefined }
2025-03-14 08:24:57.639 [debug] API Response (ec2.us-west-2.amazonaws.com /): {
  '$metadata': {
    httpStatusCode: 200,
    requestId: '[manually-omitted]',
    extendedRequestId: undefined,
    cfId: undefined
  },
  Reservations: []
}

On failing API request logs:

2025-03-14 08:21:16.992 [debug] CreateBucket called with request: { bucketName: 'aaa' }
2025-03-14 08:21:16.992 [debug] API Request (aaa.s3.us-east-1.amazonaws.com /):
 headers: { Connection: 'keep-alive', host: 'aaa.s3.us-east-1.amazonaws.com' }
 input: { Bucket: 'aaa', CreateBucketConfiguration: undefined }
2025-03-14 08:21:17.173 [warning] API Request (aaa.s3.us-east-1.amazonaws.com /) resulted in error: {
  name: 'BucketAlreadyExists',
  '$fault': 'client',
  '$metadata': {
    httpStatusCode: 409,
    requestId: '[manually-omitted]',
    extendedRequestId: '[manually-omitted]',
    cfId: undefined
  },
  Code: 'BucketAlreadyExists',
  BucketName: 'aaa',
  RequestId: '[manually-omitted]',
  HostId: '[manually-omitted]',
  message: 'The requested bucket name is not available. The bucket namespace is shared by all users of the system. Please select a different name and try again.',
  mesage: 'The requested bucket name is not available. The bucket namespace is shared by all users of the system. Please select a different name and try again.'
}
2025-03-14 08:21:17.174 [error] API Request failed without fallback (time: 185ms) 
params: { Bucket: 'aaa', CreateBucketConfiguration: undefined }

the [manually-omitted] field were removed by hand and exist in the original logs.

Future Work

  • verify middleware behavior via E2E tests with local endpoint.

  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Hweinstock
Copy link
Contributor Author

Hweinstock commented Mar 13, 2025

/runIntegrationTests

@Hweinstock Hweinstock changed the title refactor(sdkv3): v3 clients log performance and header information by default. (WIP) refactor(sdkv3): v3 clients log performance and header information by default Mar 14, 2025
@Hweinstock Hweinstock changed the title refactor(sdkv3): v3 clients log performance and header information by default feat(sdkv3): centralize and improve client logging and error handling. Mar 14, 2025
@Hweinstock Hweinstock marked this pull request as ready for review March 14, 2025 13:17
@Hweinstock Hweinstock requested a review from a team as a code owner March 14, 2025 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant