Use pino instead of console.log #2263
Conversation
There was a problem hiding this comment.
@DracFiendMG, thanks so much for starting this!
It looks like currencyservice is failing to build because the logger object hasn't been initialized inside server.js.
> [currencyservice-6f5d5cf7b6-vwlmb server] /usr/src/app/server.js:18
> [currencyservice-6f5d5cf7b6-vwlmb server] logger.info("Profiler disabled.")
> [currencyservice-6f5d5cf7b6-vwlmb server] ^
> [currencyservice-6f5d5cf7b6-vwlmb server] ReferenceError: Cannot access 'logger' before initialization
> [currencyservice-6f5d5cf7b6-vwlmb server] at Object.<anonymous> (/usr/src/app/server.js:18:3)
You may want to test your changes locally by deploying Online Boutique to a kind (or minikube) Kubernetes cluster on your machine.
You can set this up by following /docs/development-guide.md
|
Sure @NimJay, I'll do that and get back to you |
|
Hi @NimJay, I've deployed and tested this locally and also committed the relevant changes, can you check it now? And I have one question: When I did skaffold run i noticed that almost 15-20 GB was occupied on my disk, do you know where these files are getting stored? I was not able to find them |
|
@DracFiendMG, you asked:
That's likely a result of the Docker container images that are stored locally on your machine — when you run You can list Docker images and containers on your machine using: You can delete all images and containers using: |
| } | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
Suggestion: (feel free to ignore)
Feel free to create a separate logger.js file (in each Node.js microservice) where we initialize the logger = pino(...) object (just once for each microservice) and re-use that object in other files (by importing the object from the logger.js file) but that's a bonus (out-of-scope) for this pull-request.
There was a problem hiding this comment.
Yeah, in the beginning I was going to do it this way, but creating multiple logger files for different services seemed a bit too much so I only made changes in the required services.
If you have a suggestion where we only use one logger.js and that can be used across all the services then do tell me about it. Because the only change in the logger is the name of the service, if we know how to set the name of the logger's service dynamically then it should be good.
If that's not possible then maybe you can raise another issue for this and I'll create separate logger.js files for all the services.
There was a problem hiding this comment.
For now, I've created logger.js for paymentservice only.
Please check and validate that
There was a problem hiding this comment.
Thanks for going the extra mile (but know that you don't have do)!
Issue: The logger.js file of the paymentservice should live at /src/paymentservice/logger.js.
|
Thanks for your continual progress on this change, @DracFiendMG! :) |
|
@NimJay, can you look into this |
src/shared/logger.js
Outdated
| @@ -0,0 +1,27 @@ | |||
| /* | |||
| * Copyright 2018 Google LLC | |||
There was a problem hiding this comment.
First off, thank you for creating a separate logger.js file!
The header-check is failing before the year should be 2023 on new files introduced by this pull-request.
| * Copyright 2018 Google LLC | |
| * Copyright 2023 Google LLC |
There was a problem hiding this comment.
I've resolved this one
|
@NimJay, I've performed the suggested changes. |
| return { severity: logLevelString } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Suggestion: We should use this in src/paymentservice/charge.js as well.
There was a problem hiding this comment.
Nice, it works! 😄 👏
@DracFiendMG, thanks so much for your patience with this pull-request.
Approved!
Remaining work:
- Introduce a
logger.jsfile in currencyservice. - Update
src/paymentservice/charge.jsto also usesrc/paymentservice/logger.js.
But someone can do those things outside of the pull-request — I don't want this pull-request to exhaust you.
|
@NimJay Thank you, I loved doing this~ |
This fixes #674.