chore(cli): use CliIoHost as logger everywhere #32708

HBobertz · 2025-01-01T20:52:44Z

Related to #32346

Reason for this change

Capture all logs across the CLI as IoMessages so that when we allow customers to use the CDK Core functionality programmatically, these logs are delivered to the customers as messages and now just swallowed across the CLI.

Description of changes

Prevented access to log() and formatLogMessage(). Now the only ways to log something properly are either through the exported log functions (which is how everyone was doing it anyway), or through CliIoHost.notify() directly.

CliIoHost is now exposed as a global singleton which is currently only directly exclusively used in logging.ts but is effectively used everywhere as all logging functions inevitably call CliIoHost.notify()

All logging functions now optionally support the following input types

error(`operation failed: ${e}`)                                 // infers default error code `TOOLKIT_0000`
error('operation failed: %s', e)                                // infers default error code `TOOLKIT_0000`
error({ message: 'operation failed', code: 'SDK_0001' })        // specifies error code `SDK_0001`
error({ message: 'operation failed: %s', code: 'SDK_0001' }, e) // specifies error code `SDK_0001`

and everything is now translated into an IoMessage and calls CliIoHost.notify() from these logging functions. Nothing currently specifies any message code so it's all the generic _0000, _1000, _2000

Description of how you validated changes

added and updated unit tests

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

packages/aws-cdk/lib/logging.ts

codecov · 2025-01-01T21:07:08Z

Codecov Report

Attention: Patch coverage is 91.91176% with 11 lines in your changes missing coverage. Please review.

Project coverage is 67.02%. Comparing base (caf2415) to head (b9a8c60).
Report is 27 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #32708      +/-   ##
==========================================
+ Coverage   66.96%   67.02%   +0.05%     
==========================================
  Files         329      330       +1     
  Lines       18667    18683      +16     
  Branches     3260     3255       -5     
==========================================
+ Hits        12501    12522      +21     
+ Misses       5839     5833       -6     
- Partials      327      328       +1

Flag	Coverage Δ
suite.unit	`67.02% <91.91%> (+0.05%)`	⬆️

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

Components	Coverage Δ
packages/aws-cdk	`80.98% <91.91%> (+0.27%)`	⬆️
packages/aws-cdk-lib/core	`82.09% <ø> (ø)`

aws-cdk-automation · 2025-01-06T16:04:52Z

➡️ 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.

kaizencc

i haven't looked at everything, mainly the tests, but providing some initial feedback and will review fully tmr

packages/aws-cdk/lib/cdk-toolkit.ts

kaizencc · 2025-01-06T22:24:50Z

packages/aws-cdk/lib/cdk-toolkit.ts

@@ -601,7 +601,7 @@ export class CdkToolkit {
          anyRollbackable = true;
        }
        const elapsedRollbackTime = new Date().getTime() - startRollbackTime;
-        print('\n✨  Rollback time: %ss\n', formatTime(elapsedRollbackTime));
+        print('\n✨  Rollback time: %ss\n', formatTime(elapsedRollbackTime).toString());


If you're going away from the %ss, which i support, we should do that here too

Actually people can still do %s. This is actually just a good catch that I don't need to do .toString anymore. It was previously more important.

packages/aws-cdk/lib/cli.ts

packages/aws-cdk/lib/logging.ts

kaizencc · 2025-01-06T22:29:09Z

packages/aws-cdk/lib/logging.ts

      logBuffer.splice(0);
    }
  }
 }

+interface LogOptions {


all these should be readonly

bumping this unless you disagree

kaizencc · 2025-01-06T22:31:35Z

packages/aws-cdk/lib/logging.ts

+      throw new Error(`Invalid message code: ${options.code}`);
+    }
+  } else {
+    options.code = `TOOLKIT_${Math.min(levelPriority[options.level] - 1, 2)}000`;


did we decide on this way of leveling here? and i agree with codecov on this one, should be tested

feels odd that we would have one system of 1-4, and another system of 0-2.

bumping this

kaizencc · 2025-01-06T22:33:43Z

packages/aws-cdk/lib/toolkit/cli-io-host.ts

+   * All 'error' level message codes begin with 0.
+   * All warning level message codes begin with 1.
+   * All info level message codes begin with 2.


Suggested change

* All 'error' level message codes begin with 0.

* All warning level message codes begin with 1.

* All info level message codes begin with 2.

* All 'error' level message codes begin with 0.

* All 'warning' level message codes begin with 1.

* All 'info' level message codes begin with 2.

kaizencc · 2025-01-06T22:34:52Z

packages/aws-cdk/lib/toolkit/cli-io-host.ts

+  /**
+   * Whether the CliIoHost is running in CI mode. In CI mode, all non-error output goes to stdout instead of stderr.
+   */
+  private ci: boolean = false;


i'm not against this style, but i would like how you add defaults to ci and isTTY to be the same...

kaizencc · 2025-01-06T22:35:58Z

packages/aws-cdk/lib/toolkit/cli-io-host.ts

+    case 'debug':
+    case 'trace':
+      return levelNum === 2;
+    default:


should be no need for default if you get all the levels

You never know what someone is gonna add and forget to modify in the validation function 😅

kaizencc · 2025-01-06T22:37:36Z

packages/aws-cdk/lib/toolkit/cli-io-host.ts

+ * @param level The IoMessageLevel of the message
+ * @returns boolean indicating if the code is valid for the given level
+ */
+export function validateMessageCode(code: string, level?: IoMessageLevel): boolean {


i would prefer this function return not just a true/false but information on why/what the errors are. usually achieved by returning a list of errors, with [] indicating "no error"

Eh maybe, either way I'll refactor this into something after we confirm how exactly we want message codes to look. Might just throw errors tbh

rix0rrr · 2025-01-07T09:31:52Z

packages/aws-cdk/lib/toolkit/cli-io-host.ts

+   * All 'error' level message codes begin with 0.
+   * All warning level message codes begin with 1.
+   * All info level message codes begin with 2.


Personally I think it'd be nicer to use letters for this: E, W, I. But that's a bit of bikeshedding.

rix0rrr · 2025-01-07T09:34:23Z

packages/aws-cdk/lib/toolkit/cli-io-host.ts

+   * 'ASSETS_2000'            // valid cdk-assets info message with generic info code _2000
+   * 'TOOLKIT_0002'           // valid toolkit error message with specific error code _0002
+   * 'SDK_1023'               // valid sdk warning message with specific warning code _1023
+   * 'sdk_0001'               // invalid: lowercase
+   * 'TOOLKIT-0001'           // invalid: invalid separator
+   * 'SDK_3000'               // invalid: all message codes must be between 0000 and 2999


Can we work CDK into some of the categories in some way? CDKTOOLKIT_0002 or CT0002 for short or something?

By the way, I would say that if you clearly define the format, you don't need to show counterexamples of all the things it cannot be.

A code is for the format <XX>_<L><yyy>, where:

<XX> is a category code, and must be one of CA, CT, SDK.

<L> indicates a severity level, one of E, W, I, D, T.

<yyy> is a 3 digit code indicating the source of the message. The code is either 000 if this is a nonspecific log message, or a specific code unique within its category to indicate a specific type of message.

If you specify the "grammar" of error codes this way, there's no need to say "it can't be sdk-11" because that will be implicitly disallowed by the grammar already.

Define constants for the categories and codes somewhere.

iliapolo · 2025-01-07T13:25:41Z

packages/aws-cdk/lib/toolkit/cli-io-host.ts

@@ -21,7 +21,21 @@ interface IoMessage {
  readonly action: IoAction;

  /**
-   * A short code uniquely identifying message type.
+   * A short message code uniquely identifying a message type of the format [CATEGORY]_[NUMBER_CODE].


I think this is fine but I like what @rix0rrr suggested with using letters to denote level.

So it would be: [CATEGORY]_[LEVEL_CHAR][NUMBER_CODE]

ASSETS_E0001

TOOLKIT_I0001

Did you have a look at how JSII emits messages? Not saying we should do exactly the same - I think this proposal actually adds some enhancements over it, but its a good resource to look at.

I didn't really check it out and just started fresh. But if we want to incorporate that into the number system that's fine with me

iliapolo · 2025-01-07T13:31:18Z

packages/aws-cdk/lib/toolkit/cli-io-host.ts

+   * All warning level message codes begin with 1.
+   * All info level message codes begin with 2.
+   *
+   * Codes ending in 000 are generic messages, while codes ending in 001-999 are specific to a particular message.


I don't fully understand the need for this. Isn't every code attached to a particular message?

Can you show an example of both authoring and consuming different codes here?

I feel like we can just say that we have 3 digits to denote the message and thats it, without the special handling of 000. Also having a generic code might eventually result in a graveyard (though not having it will bloat up the numbers).

We can also start without 000. That is, start with 001...999 and add 000 as a "generic" thing if we need it.

Every message will have a code but they do not need to be a unique code. I fully suspect that especially for info level codes that people really won't care to differentiate as much as they would with warnings or errors. I suspect any arbitrary log message today that is like "doing , please wait" probably doesn't warrant a code so it'll be a generic info code. The alternative is that code becomes an optional value (which is probably fine), and is really only relevant for errors and warnings

Co-authored-by: Kaizen Conroy <[email protected]>

aws-cdk-automation · 2025-01-08T05:07:41Z

AWS CodeBuild CI Report

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

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

kaizencc

Read through all the files. I'm a bit concerned as to whether or not we're ok with changing the printed messages in this PR. If we are not aiming to do that, then we really need to scrutinize the tests and the diff makes that very difficult to do :). So I haven't done that yet unless it's really necessary.

kaizencc · 2025-01-08T17:29:08Z

packages/aws-cdk/lib/logging.ts

      logBuffer.splice(0);
    }
  }
 }

+interface LogOptions {


bumping this unless you disagree

kaizencc · 2025-01-08T17:29:41Z

packages/aws-cdk/lib/logging.ts

+      throw new Error(`Invalid message code: ${options.code}`);
+    }
+  } else {
+    options.code = `TOOLKIT_${Math.min(levelPriority[options.level] - 1, 2)}000`;


bumping this

kaizencc · 2025-01-08T17:31:35Z

packages/aws-cdk/lib/logging.ts

+  ...args: unknown[]
+): void {
+  // Extract message and code from input
+  const { message, code = `TOOLKIT_${getCodeLevel(level)}000` } = typeof input === 'object'


i believe you're going to update to include CDK prefix. but also, this and line 94 both hardcode the syntax. it should live in one place so that if for whatever reason we need to change it we don't have to do it twice

kaizencc · 2025-01-08T17:31:52Z

packages/aws-cdk/lib/logging.ts

+}
+
+// Type for the object parameter style
+interface LogParams {


kaizencc · 2025-01-08T17:33:59Z

packages/aws-cdk/lib/logging.ts

+};
+
+/** Alias for the {@link info} logging function */
+export const print = info;


why can't we just use info everywhere that print is being used?

kaizencc · 2025-01-08T17:39:31Z

packages/aws-cdk/lib/api/aws-auth/credential-plugins.ts

@@ -151,7 +151,7 @@ function v3ProviderFromV2Credentials(x: SDKv2CompatibleCredentials): AwsCredenti
 function refreshFromPluginProvider(current: AwsCredentialIdentity, producer: () => Promise<PluginProviderResult>): AwsCredentialIdentityProvider {
  return async () => {
    // eslint-disable-next-line no-console
-    console.error(current, Date.now());
+    info(format(current), Date.now());


you should remove the // eslint-disable-next-line no-console on this any any other instances where you no longer call the console.

kaizencc · 2025-01-08T17:49:59Z

packages/aws-cdk/lib/api/util/cloudformation/stack-activity-monitor.ts

@@ -514,7 +515,7 @@ export class HistoryActivityPrinter extends ActivityPrinterBase {
  public stop() {
    // Print failures at the end
    if (this.failures.length > 0) {
-      this.stream.write('\nFailed resources:\n');
+      info('\nFailed resources:');


in this file, multiple classes hold a this.stream property to determine if it writes to stdout or stderr. with your change, this property seems obsolete -- we are no longer storing information on what stream to write to on these printers. if that is correct, we should remove that property altogether so as not to leave unused properties behind

kaizencc · 2025-01-08T17:51:04Z

packages/aws-cdk/lib/api/util/cloudformation/stack-activity-monitor.ts

      util.format(
-        '%s | %s%s | %s | %s | %s %s%s%s\n',
+        '%s | %s%s | %s | %s | %s %s%s%s',


why remove newline? and specifically, is the goal of this PR to not change printing output at all? how are you verifying that

kaizencc · 2025-01-08T17:51:45Z

packages/aws-cdk/lib/cdk-toolkit.ts


    if (stackCollection.stackCount === 0) {
      // eslint-disable-next-line no-console
-      console.error('No stacks selected');
+      error('No stacks selected');


remove eslint comment above. please find all the other instances, i'm not going to comment on them all.

kaizencc · 2025-01-08T17:52:44Z

packages/aws-cdk/lib/commands/context.ts

@@ -56,7 +56,7 @@ export async function contextHandler(options: ContextOptions): Promise<number> {
    if (options.json) {
      /* istanbul ignore next */
      const contextValues = options.context.all;
-      process.stdout.write(JSON.stringify(contextValues, undefined, 2));
+      info(JSON.stringify(contextValues, undefined, 2));


wont info write to stderr?

aws-cdk-automation · 2025-01-08T18:00:39Z

The pull request linter fails with the following errors:

❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/32708/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

HBobertz added 2 commits January 1, 2025 15:48

initial functional IoHost logging

d304251

Merge branch 'main' into bobertzh/CliIoHost

12396ca

aws-cdk-automation requested a review from a team January 1, 2025 20:52

github-actions bot added the p2 label Jan 1, 2025

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 1, 2025

HBobertz requested a deployment to test-pipeline January 1, 2025 20:53 — with GitHub Actions Waiting

aws-cdk-automation requested changes Jan 1, 2025

View reviewed changes

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Jan 1, 2025

HBobertz commented Jan 1, 2025

View reviewed changes

packages/aws-cdk/lib/logging.ts Show resolved Hide resolved

HBobertz and others added 2 commits January 3, 2025 13:32

add testing

72c93da

Merge branch 'main' into bobertzh/CliIoHost

498dde1

HBobertz requested a deployment to test-pipeline January 3, 2025 18:33 — with GitHub Actions Waiting

fix lint problems

fa3513d

HBobertz requested a deployment to test-pipeline January 3, 2025 18:51 — with GitHub Actions Waiting

better comments

ea2f842

HBobertz requested a deployment to test-pipeline January 3, 2025 20:15 — with GitHub Actions Waiting

lint fixes

a5b59a6

HBobertz requested a deployment to test-pipeline January 3, 2025 20:50 — with GitHub Actions Waiting

update logic and improve testing

6f9add3

HBobertz requested a deployment to test-pipeline January 5, 2025 18:40 — with GitHub Actions Waiting

update non-functionals

306f08b

HBobertz requested a deployment to test-pipeline January 5, 2025 18:45 — with GitHub Actions Waiting

update log function name

879fa3c

HBobertz requested a deployment to test-pipeline January 5, 2025 18:56 — with GitHub Actions Waiting

change comment

255dc60

HBobertz requested a deployment to test-pipeline January 5, 2025 18:58 — with GitHub Actions Waiting

add action logic

cc2f7a9

HBobertz marked this pull request as ready for review January 5, 2025 19:11

HBobertz requested a review from a team as a code owner January 5, 2025 19:11

Merge branch 'main' into bobertzh/CliIoHost

ae48a18

HBobertz requested a deployment to test-pipeline January 5, 2025 19:13 — with GitHub Actions Waiting

add format

3c6bb9b

HBobertz temporarily deployed to test-pipeline January 6, 2025 15:45 — with GitHub Actions Inactive

kaizencc requested changes Jan 6, 2025

View reviewed changes

kaizencc changed the title ~~chore: use CliIoHost as logger everywhere~~ chore(cli): use CliIoHost as logger everywhere Jan 6, 2025

rix0rrr reviewed Jan 7, 2025

View reviewed changes

iliapolo reviewed Jan 7, 2025

View reviewed changes

add cli-io-host logging

65751a4

HBobertz requested a deployment to test-pipeline January 8, 2025 01:59 — with GitHub Actions Waiting

Update packages/aws-cdk/lib/logging.ts

7e45655

Co-authored-by: Kaizen Conroy <[email protected]>

HBobertz requested a deployment to test-pipeline January 8, 2025 03:37 — with GitHub Actions Waiting

chalk support the smart way

b9a8c60

HBobertz requested a deployment to test-pipeline January 8, 2025 04:37 — with GitHub Actions Waiting

kaizencc requested changes Jan 8, 2025

View reviewed changes

chore(cli): use CliIoHost as logger everywhere #32708

Are you sure you want to change the base?

chore(cli): use CliIoHost as logger everywhere #32708

Conversation

HBobertz commented Jan 1, 2025 • edited Loading

Related to #32346

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 1, 2025 • edited Loading

Codecov Report

aws-cdk-automation commented Jan 6, 2025

kaizencc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 8, 2025

AWS CodeBuild CI Report

kaizencc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 8, 2025

HBobertz commented Jan 1, 2025 •

edited

Loading

codecov bot commented Jan 1, 2025 •

edited

Loading

rix0rrr Jan 7, 2025 •

edited

Loading