Skip to content

diagnostics_channel: capture console messages#56292

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Qard:console-diagnostics-channels
Jan 13, 2025
Merged

diagnostics_channel: capture console messages#56292
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Qard:console-diagnostics-channels

Conversation

@Qard

@Qard Qard commented Dec 17, 2024

Copy link
Copy Markdown
Member

I've added diagnostics_channel support to capture and modify inputs to the main console methods. This enables some useful things like APMs injecting tracing data into log messages to correlate log messages to the requests from which they originated. You can also replace the args list entirely, so you could apply your own custom formatting to produce JSON logs instead or things like that.

cc @nodejs/diagnostics @nodejs/console

@Qard Qard added console Issues and PRs related to the console subsystem. diagnostics_channel Issues and PRs related to diagnostics channel labels Dec 17, 2024
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Dec 17, 2024
@Qard Qard force-pushed the console-diagnostics-channels branch from 6ba11e5 to 07fbcd4 Compare December 17, 2024 16:11
@Qard Qard marked this pull request as ready for review December 17, 2024 16:12
@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Qard Qard force-pushed the console-diagnostics-channels branch 2 times, most recently from 8094626 to b3830bf Compare December 17, 2024 16:56
@eugeneo

eugeneo commented Dec 17, 2024

Copy link
Copy Markdown
Contributor

I lack context on this, so I might be missing some point.

Can this use the Inspector protocol instead of adding hooks directly into console object. In-process inspector API can be configured to watch for the messages? This way there will be less coupling.

@Qard

Qard commented Dec 17, 2024

Copy link
Copy Markdown
Member Author

The Inspector API is substantially more expensive. This is meant for being able to rewrite potentially all log messages in production, so it needs to be fast.

@eugeneo

eugeneo commented Dec 17, 2024

Copy link
Copy Markdown
Contributor

Thanks for the reply.

@Flarna

Flarna commented Dec 17, 2024

Copy link
Copy Markdown
Member

Maybe worth to extend test to verify adding/removing/replacing elements,

Main problem I see with mutating diagnostics channels is the undefined sequence if there are more subscribers.


* `args` {any\[]}

Emitted when `console.log()` is called. Receives and array of the arguments

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe describe that this is the arguments array and therefore mutation is possible.

Comment thread lib/internal/console/constructor.js
@legendecas

Copy link
Copy Markdown
Member

Would you mind rebasing on top of the main branch? The build failure should be fixed once rebased.

@Qard Qard force-pushed the console-diagnostics-channels branch from b3830bf to 3548683 Compare January 13, 2025 04:58
@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2025
@nodejs-github-bot

This comment was marked as outdated.

@codecov

codecov Bot commented Jan 13, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.20%. Comparing base (f4fcf0e) to head (82b3ae5).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #56292   +/-   ##
=======================================
  Coverage   89.19%   89.20%           
=======================================
  Files         662      662           
  Lines      191762   191791   +29     
  Branches    36907    36916    +9     
=======================================
+ Hits       171041   171079   +38     
+ Misses      13572    13558   -14     
- Partials     7149     7154    +5     
Files with missing lines Coverage Δ
lib/internal/console/constructor.js 98.54% <100.00%> (+0.51%) ⬆️

... and 39 files with indirect coverage changes

@Qard Qard force-pushed the console-diagnostics-channels branch from 3548683 to 82b3ae5 Compare January 13, 2025 08:33
@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Qard

Qard commented Jan 13, 2025

Copy link
Copy Markdown
Member Author

Test are passing now. I'd appreciate some fresh approvals! 🙂

@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 13, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 13, 2025
@nodejs-github-bot nodejs-github-bot merged commit db7a31e into nodejs:main Jan 13, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in db7a31e

@Qard Qard deleted the console-diagnostics-channels branch January 13, 2025 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

console Issues and PRs related to the console subsystem. diagnostics_channel Issues and PRs related to diagnostics channel needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants