-
-
Notifications
You must be signed in to change notification settings - Fork 331
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
feat(logger): remove unused trace log level #5861
Conversation
What happens if you pass |
Performance Report✔️ no performance regression detected Full benchmark results
|
It will error as it's unsupported per our logging policy. Should I throw an error on startup stating that its not supported? Or perhaps should I leave the functions but throw a better error and kill the process if we use it so we know not to? |
As per issue
Right now we break this use case, I would suggest we go with what I suggested in #5809 (comment).
|
5ee0d47
to
4b6a3cc
Compare
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.
A runtime error like this is not the best way to handle it
I agree. Was actually just messaging @nflaig about this on discord. Was tossing around the idea of just having trace log to the debug LogLevel but that isnt great either because the function can still be used... Another idea would be to have two "log levels" one for reporting with all of them and one for the "supported levels" that is used for interface generation so the classes do not export the symbol. That way passing --trace as the reporting level just outputs debug level but the intellisense will not have trace as a method. What do you prefer @dapplion and @nflaig? |
We can simply update the logger interface lodestar/packages/utils/src/logger.ts Line 4 in e690ac6
to something like this export type Logger = Record<Exclude<LogLevel, LogLevel.trace>, LogHandler>; This will allow to set |
Yeah, would like to see the solution @nflaig proposed |
…evel" This reverts commit 4b6a3cc.
9fd6f86
to
92cca53
Compare
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.
shouldn't this method be removed here as well?
lodestar/packages/logger/src/winston.ts
Line 80 in 92cca53
trace(message: string, context?: LogData, error?: Error): void { |
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.
Lgtm, can you update the PR description with the latest changes / motivation before you merge?
On that note, does this close #5809 as well? The expected behavior in the issue is
but after discussion, the expected behavior should rather be what I mentioend in #5861 (comment)
|
@wemeetagain the PR body is updated but cannot merge because of e2e test not closing cleanly. Ready to merge though I believe. |
🎉 This PR is included in v1.12.0 🎉 |
Motivation
trace
does not follow our logging policy and it is broken as well. Found an error and instead of fixing we decided to remove it.Closes #5809
Description
log.trace()
Logger
interface to removetrace
methodLogLevel.trace
so that it can be passed into the CLI without throwing an error but will be handled similar todebug
logging leveltrace
method fromWinstonLogger
,getLoggerVc
andgetEmptyLogger