Skip to content

refactor(cli): localize SDK tracing#33273

Merged
mergify[bot] merged 3 commits intomainfrom
mrgrain/refactor/localiz-sdk-tracing
Feb 4, 2025
Merged

refactor(cli): localize SDK tracing#33273
mergify[bot] merged 3 commits intomainfrom
mrgrain/refactor/localiz-sdk-tracing

Conversation

@mrgrain
Copy link
Copy Markdown
Contributor

@mrgrain mrgrain commented Feb 3, 2025

Reason for this change

The CLI has a global method call tracing feature that is currently only used for SDK calls.
Eventually this feature needs to support logging through the IoHost. While this PR doesn't achieve this, it co-locates the tracing feature with SDK logs and explicitly calls out its limitations.

We will need a better, different approach once we are looking at the SDK Provider in more detail.

Description of changes

Move the feature to aws-auth module.
Limit exports and renames for clarity.
Limit tracing to member methods as these can add a class logger.
Add a manual trace to the one static method this was currently tracig

Describe any new or updated permissions being added

n/a

Description of how you validated changes

Existing unit & integ tests, manual verification

Checklist


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@mrgrain mrgrain requested a review from a team as a code owner February 3, 2025 14:04
@github-actions github-actions bot added the p2 label Feb 3, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team February 3, 2025 14:05
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Feb 3, 2025
Copy link
Copy Markdown
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

(This review is outdated)

@mrgrain mrgrain force-pushed the mrgrain/refactor/localiz-sdk-tracing branch from 75b4bb4 to 2e0aee9 Compare February 3, 2025 16:16
@mrgrain
Copy link
Copy Markdown
Contributor Author

mrgrain commented Feb 3, 2025

@mergify update

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Feb 3, 2025

update

✅ Branch has been successfully updated

@mergify mergify bot temporarily deployed to test-pipeline February 3, 2025 19:47 Inactive
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 3, 2025

Codecov Report

Attention: Patch coverage is 68.29268% with 13 lines in your changes missing coverage. Please review.

Project coverage is 80.80%. Comparing base (6a77e4f) to head (ded442c).
Report is 1 commits behind head on main.

❌ Your patch status has failed because the patch coverage (68.29%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #33273      +/-   ##
==========================================
- Coverage   80.84%   80.80%   -0.04%     
==========================================
  Files         236      236              
  Lines       14230    14235       +5     
  Branches     2487     2487              
==========================================
- Hits        11504    11503       -1     
- Misses       2442     2448       +6     
  Partials      284      284              
Flag Coverage Δ
suite.unit 80.80% <68.29%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
packages/aws-cdk 79.57% <68.29%> (-0.07%) ⬇️
packages/aws-cdk-lib/core 82.14% <ø> (ø)

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

Copy link
Copy Markdown
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

lgtm

* role doesn't have `sts:AssumeRole` and will fail for no real good reason.
*/
@traceMethods
@traceMemberMethods
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i did not know we had these decorators...

logger[meth]('test');
expect(trace).not.toHaveBeenCalled();
test.each(['trace', 'debug'] as Array<keyof SdkToCliLogger>)('%s method does not call notify', (method) => {
logger[method]('test');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hahaha nice

@mrgrain mrgrain added pr-linter/exempt-codecov The PR linter will not require codecov checks to pass pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested labels Feb 3, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review February 4, 2025 12:04

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Feb 4, 2025

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Feb 4, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: ded442c
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Feb 4, 2025

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit d82ca88 into main Feb 4, 2025
14 of 19 checks passed
@mergify mergify bot deleted the mrgrain/refactor/localiz-sdk-tracing branch February 4, 2025 12:44
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 4, 2025

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

contribution/core This is a PR that came from AWS. p2 pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested pr-linter/exempt-codecov The PR linter will not require codecov checks to pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants