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

Use threadId of worker_thread when logging inside worker thread #9281

Closed
1 task done
gioragutt opened this issue Mar 2, 2022 · 5 comments
Closed
1 task done

Use threadId of worker_thread when logging inside worker thread #9281

gioragutt opened this issue Mar 2, 2022 · 5 comments
Labels
needs triage This issue has not been looked into type: enhancement 🐺

Comments

@gioragutt
Copy link

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe it

This isn't critical but could help out with working out logs.
I have an app that delegates jobs via pull to workers that run with worker_threads.

When logging, there's no indication that the log is emitted from a worker thread (compared to the main thread).

Example log:

[Nest] 15054  - 03/02/2022, 9:59:30 AM     LOG [NestApplication] Nest application successfully started +0ms

Describe the solution you'd like

I'd be happy if, optionally, the logger would append the thread id to (f.e) the PID, like so:

[Nest] 15054 (1)  - 03/02/2022, 9:59:30 AM     LOG [NestApplication] Nest application successfully started +0ms

Where 1 is the thread id.

Teachability, documentation, adoption, migration strategy

If this won't be turned on by default, perhaps this can be activated via a method as follows:

import {Logger} from '@nestjs/common';
import {isMainThread, threadId} from 'worker_threads';

if (!isMainThread) {
  Logger.setThreadId(threadId);
}

What is the motivation / use case for changing the behavior?

Ability to better understand and differentiate logs coming from worker threads.
I do believe the same might apply to cluster f.e, I didn't use it but I assume a similar problem might arise there.

@gioragutt gioragutt added needs triage This issue has not been looked into type: enhancement 🐺 labels Mar 2, 2022
@stanimirovv
Copy link

@kamilmysliwiec @micalevisk

is anyone working on this ? If not I would gladly give it a shot.

@micalevisk
Copy link
Member

micalevisk commented Mar 8, 2022

@stanimirovv I guess it's better to wait Kamil's thoughts on this feat. I'd prefer this to be always enable, therefore you would just do a minor change below:

const pidMessage = color(`[Nest] ${process.pid} - `);

Also, to me, the thread id should be displayed only if !isMainThread


thanks :)

@kamilmysliwiec
Copy link
Member

kamilmysliwiec commented Mar 9, 2022

I'd say we should just make it somehow simpler to extend the current format of the messages so people who want to add certain details (such as a thread id) could extend the built-in logger and tweak 1 method. I suppose extracting the logic responsible for formatting messages (that's is now hardcoded in the printMessages) to a separate protected method should solve this issue.

PRs are more than welcome!

@stanimirovv
Copy link

On it

@kamilmysliwiec
Copy link
Member

Let's track this here #9316

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into type: enhancement 🐺
Projects
None yet
Development

No branches or pull requests

4 participants