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 winston for opentelemetry logging #9234

Closed
ludamad opened this issue Oct 14, 2024 · 6 comments · Fixed by #9486
Closed

Use winston for opentelemetry logging #9234

ludamad opened this issue Oct 14, 2024 · 6 comments · Fixed by #9486
Assignees

Comments

@ludamad
Copy link
Collaborator

ludamad commented Oct 14, 2024

Winston is very featureful, we should combine the LOG_JSON mode wth our stdout mode which it both supports and get rid of our custom logger, doing any sort of transforms we currently do before hitting winston

@ludamad ludamad mentioned this issue Oct 14, 2024
1 task
@ludamad ludamad changed the title Refactor logging to not have both winston and our custom logging infra Refactor logging to use a single logger that alpha team decides Oct 20, 2024
@ludamad
Copy link
Collaborator Author

ludamad commented Oct 20, 2024

Note @alexghr had thoughts, can you write them here?

@alexghr
Copy link
Contributor

alexghr commented Oct 21, 2024

I quite like pino for logging. It logs JSON all the time and is a drop-in replacement for debug (see pino-debug) so we wouldn't have to change any of our logging code (at least initially).

For local logs pino has pino-pretty to pretty print the JSON logs (we'd just have to redefine our start scripts to pipe stdout through pino-pretty).

Pino is supposedly faster than both winston and debug but I haven't tested this myself (pino benchmarks);

Both pino and winston have official OpenTelemetry instrumentation plugins (to capture context and push logs to the collector):

CC @spalladino as he had some thoughts on logging too!

@ludamad
Copy link
Collaborator Author

ludamad commented Oct 21, 2024

Ah gotcha, well it's pretty trivial to move to Winston too, curious on @spalladino's thoughts too

@ludamad
Copy link
Collaborator Author

ludamad commented Oct 21, 2024

Hmm what was the rationale to keep node debugger module?

@ludamad
Copy link
Collaborator Author

ludamad commented Oct 23, 2024

@spypsy I'm probably leaning towards redoing this issue as just attaching an opentelemetry plugin for winston so we can point it to the k8s collector URL from the local network tests

@ludamad ludamad changed the title Refactor logging to use a single logger that alpha team decides Use winston for opentelemetry logging Oct 28, 2024
@ludamad ludamad assigned ludamad and unassigned spypsy Oct 28, 2024
@spalladino
Copy link
Collaborator

I'm fine with either winston or pino. A feature I wanted from winston when I first introduced it was easy logrotate, used in the sandbox for keeping local logs that users could send to us, but this probably won't be needed in proper node setups, and there seems to be a pino equivalent.

I haven't used pino in the past, but if it checks the boxes in the wishlist I had put together (assuming folks agree with those wishes!), let's go with it.

Hmm what was the rationale to keep node debugger module?

Personally, I'd remove debug altogether, and just have something where we can tweak logging per-namespace easily.

@github-project-automation github-project-automation bot moved this to Todo in A3 Oct 28, 2024
ludamad added a commit that referenced this issue Oct 30, 2024
After this PR:
- switched to deployment of metrics helm chart, meaning we are just a
normal endpoint and don't try to do k8s-aware log scraping
- Now code that passes LOG_JSON and OTEL_EXPORTER_OTLP_LOGS_ENDPOINT
will use winston otel transport to log to that otel endpoint (making its
way into grafana eventually)
- Adjustments in helm chart and winston transport side enabling logging,
see working here:
<img width="1228" alt="Screenshot 2024-10-29 at 8 38 01 PM"
src="https://github.com/user-attachments/assets/c141eac7-8ea6-4ed6-9ba9-5b181e89775c">

This was from the scripts/run_native_testnet_with_metrics.sh script that
got the publicly available metrics deployment and pointed the local
testnet scripts at it.
- minor fix for earthly s3 caching
- new post_deploy_spartan.sh script that runs 'network-bootstrap' that
initializes the network with some key test contracts
- refactor logging a bit to make it the negative patterns useful for all
logging pathways + extract logic added for offsite demo into its own
functional wrapper for adding 'fixed data'
- Closes #9234
@github-project-automation github-project-automation bot moved this from Todo to Done in A3 Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants