-
Notifications
You must be signed in to change notification settings - Fork 9
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
Change logger to singleton #564
Conversation
|
packages/shared/src/logger/logger.ts
Outdated
@@ -71,6 +72,7 @@ class Logger { | |||
return; | |||
} | |||
|
|||
this.grafana.logger.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miriambudayr I had to add, otherwise, the process wasn't exiting packages/puppeteer/src/first-run.ts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, interesting. So basically the CLI process would hang forever?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed that you're calling close
before flush
here though. Is that intentional?
If it's important to call both, it seems like flushing before closing would be the expected order. (That's the order @Andarist chose too.)
I'll resolve the conflicts to accept his ordering for now. If that's a mistake, tag me and I'll follow up! 🙇🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed that you're calling close before flush here though. Is that intentional?
I specifically chose to call flush before close so we could await the flush. Close also flushes things but it's using a "detached" promise to do that (it's not returned):
https://github.com/JaniAnttonen/winston-loki/blob/2ed0954e97d85e4be1b81f8d853f1864fa37cf6c/src/batcher.js#L304
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Good clarification. (Maybe we should add an inline comment with a link to the docs?)
Since Miriam approved this PR, I'm going to merge it. Hope that's okay! (Doing this will unblock me to make a small change to the |
Closes PRO-722.
This PR moves the logger to singleton, we can keep all tags centrally. Will be needed for the sessionId change I'm doing.