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

All CDK-Assets logging needs to be pluggable #196

Open
mrgrain opened this issue Nov 21, 2024 · 3 comments
Open

All CDK-Assets logging needs to be pluggable #196

mrgrain opened this issue Nov 21, 2024 · 3 comments
Assignees

Comments

@mrgrain
Copy link
Contributor

mrgrain commented Nov 21, 2024

Currently cdk-assets writes logs directly to stdout and stderr. It also uses a boolean quiet flag to decide if output is printed or not.

  • Instead we need the logging to be pluggable from the outside world.
  • The quiet options needs to be deprecated in favor of a log level. Existing quiet should be mapped to a log level
  • Anything written to process.stdout or process.stderr needs also be pluggable
@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 26, 2024

Are you sure we need this? The cdk-assets library already exposes a pluggable logger interface:

    /**
     * Listener for progress events
     *
     * @default No listener
     */
    readonly progressListener?: IPublishProgressListener;

That is used by the CDK CLI. CDK CLI funnels log events to the regular CDK CLI logger.

@HBobertz HBobertz self-assigned this Nov 26, 2024
@mrgrain
Copy link
Contributor Author

mrgrain commented Nov 26, 2024

Are you sure we need this? The cdk-assets library already exposes a pluggable logger interface:
That is used by the CDK CLI. CDK CLI funnels log events to the regular CDK CLI logger.

Great find! In that case we just need to make sure this is used everywhere. I came across the Docker asset building implementation which is not using the logger at the moment:

process.stdout.write(chunk);

@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 26, 2024

I was discussing this with @HBobertz today, and we're not quite sure what the best solution is here. This is for build output that is intended primarily for a human's eyeballs, not for a program. Of course, in an IDE context or whatever, we would want to capture it so we can send it somewhere else. Buffering stdout to the last moment and then emitting it all at once is probably a bad experience, so we'll need to do something on-the-fly. Also, the chunks can unpredictably range anywhere from 1 to 8092 bytes.

What do you think of the following approach:

  • Introduce a new cdk-assets EventType, something like EventType.BUILD_OUTPUT or whatever. Do we want a separation between stdout and stderr?
  • Split stdout/stderr on newlines (take into account that chunk boundaries can straddle line boundaries)
  • Emit every line over the regular IPublishProgressListener interface.

@HBobertz HBobertz changed the title Logging needs to be pluggable All CDK-Assets logging needs to be pluggable Dec 8, 2024
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

No branches or pull requests

3 participants