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

Broken trace logging #5809

Closed
matthewkeil opened this issue Jul 27, 2023 · 7 comments · Fixed by #5861
Closed

Broken trace logging #5809

matthewkeil opened this issue Jul 27, 2023 · 7 comments · Fixed by #5861
Labels
meta-bug Issues that identify a bug and require a fix.

Comments

@matthewkeil
Copy link
Member

Describe the bug

It appears that trace logging is broken. Attempted to add a logger.trace() call and e2e tests and the node broke when deployed.

I removed the call and the error went away. I changed the error to debug and the error also went away. It looks like the name was changed from silly to trace in #4539 and winston expects the string silly to be passed.

We are setting the logLevelNum which the docs say should work but it may not be set correctly.

TypeError: colors[Colorizer.allColors[lookup]] is not a function
    at Colorizer.colorize (/usr/src/lodestar/node_modules/logform/colorize.js:73:49)
    at Colorizer.transform (/usr/src/lodestar/node_modules/logform/colorize.js:98:25)
    at Format.transform (/usr/src/lodestar/node_modules/logform/combine.js:20:24)
    at DerivedLogger._transform (/usr/src/lodestar/node_modules/winston/lib/winston/logger.js:313:29)
    at DerivedLogger.Transform._read (/usr/src/lodestar/node_modules/readable-stream/lib/_stream_transform.js:177:10)
    at DerivedLogger.Transform._write (/usr/src/lodestar/node_modules/readable-stream/lib/_stream_transform.js:164:83)
    at doWrite (/usr/src/lodestar/node_modules/readable-stream/lib/_stream_writable.js:409:139)
    at writeOrBuffer (/usr/src/lodestar/node_modules/readable-stream/lib/_stream_writable.js:398:5)
    at DerivedLogger.Writable.write (/usr/src/lodestar/node_modules/readable-stream/lib/_stream_writable.js:307:11)
    at DerivedLogger.log (/usr/src/lodestar/node_modules/winston/lib/winston/logger.js:225:14) colors[Colorizer.allColors[lookup]] is not a function

Expected behavior

A trace log should work and not break the other log levels

Steps to reproduce

  1. Create a logger.trace
  2. Run e2e tests and should work correctly

Additional context

No response

Operating system

Linux

Lodestar version or commit hash

unstable

@matthewkeil matthewkeil added the meta-bug Issues that identify a bug and require a fix. label Jul 27, 2023
@matthewkeil
Copy link
Member Author

It looks like the ColorizeOptions expect a map with matching levels for color lookup

@nflaig
Copy link
Member

nflaig commented Jul 27, 2023

Last time we had discussion on this in #5299 there was no push towards adding trace, it is not part of the Logging policy. It is not supposed to work right now and not used anywhere.

As far as I understand the renaming in #4539 was just done so you could set --logLevel trace on CLI, see issue #4536.

Before looking into fixing the issue here, I would suggest we first discuss if trace is a log level we should support and then add it to logging policy first.

@matthewkeil
Copy link
Member Author

We should remove the option if we are not going to use it. Broke the node when i deployed trying to trace the network worker message latency to figure out why the grafana dashboard wasnt working. Or fix it. I figured out how to and was pretty easy. Just needed to pass a map to the color options. Already fixed on mkeil/fix-log-trace branch. Just need to update the unit tests.

@dapplion
Copy link
Contributor

From the agreed logging policy we do not support trace right?

@philknows
Copy link
Member

Currently we do not support trace. It’s not in our contribution md.

@nflaig
Copy link
Member

nflaig commented Aug 1, 2023

We should remove it from types to prevent accidental usage but still allow it to be passed through CLI to not break the use case mentioned in #4536.

@matthewkeil
Copy link
Member Author

Closing. Removed trace log level in #5861

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta-bug Issues that identify a bug and require a fix.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants