Enhancement for logs #30000
Enhancement for logs #30000
Conversation
siteCache to prevent nil map access comments enhancements
enhance comments
| @@ -173,8 +173,8 @@ func (l *logger) Write(level slog.Level, msg string, attrs ...any) { | |||
| l.inner.Handler().Handle(context.Background(), r) | |||
| } | |||
|
|
|||
| func (l *logger) Log(level slog.Level, msg string, attrs ...any) { | |||
| l.Write(level, msg, attrs...) | |||
| func (l *logger) Log(level slog.Level, msg string, ctx ...interface{}) { | |||
There was a problem hiding this comment.
Why do you prefer interface to any here?
|
|
||
| fs := runtime.CallersFrames([]uintptr{r.PC}) | ||
| frame, _ := fs.Next() | ||
|
|
||
| for _, rule := range h.patterns { | ||
| if rule.pattern.MatchString(fmt.Sprintf("+%s", frame.File)) { | ||
| h.siteCache[r.PC], lvl, ok = rule.level, rule.level, true | ||
| break |
There was a problem hiding this comment.
Doesn't this change the logic by only matching on the first occurrence instead of every occurrence?
| @@ -184,20 +186,21 @@ func (h *GlogHandler) Handle(_ context.Context, r slog.Record) error { | |||
| // If we didn't cache the callsite yet, calculate it | |||
| if !ok { | |||
| h.lock.Lock() | |||
| defer h.lock.Unlock() | |||
There was a problem hiding this comment.
Afaict this will lock the handler even for the h.origin.Handle call in line 206 (203 in original) which means we hold the lock longer than we need to
| } | ||
| buf.WriteString(color) | ||
| buf.Write(appendEscapeString(buf.AvailableBuffer(), attr.Key)) | ||
| buf.WriteString("\x1b[0m=") |
There was a problem hiding this comment.
Pretty sure that this is wrong, previously we wrote = when color was "", now its "\x1b[0m="
| } | ||
| b.WriteString(color) | ||
| b.WriteString(LevelAlignedString(r.Level)) | ||
| b.WriteString("\x1b[0m") |
There was a problem hiding this comment.
This should only be written if we're using terminal colors
| fieldPadding map[string]int | ||
|
|
||
| buf []byte | ||
| mu sync.Mutex // Mutex for thread safety |
There was a problem hiding this comment.
"Mutex for thread safety" is a moot description.
The fact that it's a mutex is already given, and mutexes are used for "thread safety", or rather, to handle mutually exclusive accesses in a multi-threaded context.
If you want to put a meaningful description, you should instead say something like "mu is used to prevent multu-threaded access on the buf field" or something like that, which describes specifically what fields the mu guards.
| // compilePattern converts a vmodule pattern to a regular expression string. | ||
| func compilePattern(pattern string) string { |
There was a problem hiding this comment.
what's the benefit of moving this out?
| @@ -168,7 +170,7 @@ func (h *GlogHandler) WithGroup(name string) slog.Handler { | |||
| panic("not implemented") | |||
| } | |||
|
|
|||
| // Log implements Handler.Log, filtering a log record through the global, local | |||
There was a problem hiding this comment.
This has been fixed in #29907 please revert this changes
|
In the past many months we have had a lot of issues with people opening PRs that are - for the most part - not useful. These include typo fixes, performance optimisations in non-hot code paths, variable renames, error message tweaks, etc. Whilst these PRs aren't unilaterally bad or worthless, most people are not opening them to make our codebase better, rather they are opening to farm commits for airdrops. From our perspective, these commits often take a non-negligible effort to review and merge, because they keep touching sensitive code, that we need to be extra careful to understand. Even trivial fixes require attention and keep distracting us from focusing on developing and maintaining Geth. We've decided to be more agressive in rejecting outside contributions that do not make a meaningful improvement:
We appreciate the enthusiasm and effort of opening a PR against Geth, but due to the significant abuse against our time, we'll be closing PRs of the above type for the foreseeable future. We apologise if someone's genuine effort gets caught up in this agressive stance. |
This pr offers: