-
Notifications
You must be signed in to change notification settings - Fork 4.7k
grpclog: add GRPC_GO_COMPONENT_LOG_LEVEL for controling component logs #8885
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
ef6044e
324a394
c0d5868
5eda131
1b7e50f
76662e9
f566320
d723006
1058fd9
619a19e
5492a39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,33 +20,78 @@ package grpclog | |||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||||||
| "strings" | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| type severityLevel int | ||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this whole logic of parsing the newly added env var should be in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated: f566320 |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const ( | ||||||||||||||||||||||||||||||||||||
| severityInfo severityLevel = iota | ||||||||||||||||||||||||||||||||||||
| severityWarning | ||||||||||||||||||||||||||||||||||||
| severityError | ||||||||||||||||||||||||||||||||||||
| severityFatal | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| func parseComponentLogLevel(logLevel string) map[string]severityLevel { | ||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated: c0d5868 |
||||||||||||||||||||||||||||||||||||
| if logLevel == "" { | ||||||||||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| logLevels := make(map[string]severityLevel) | ||||||||||||||||||||||||||||||||||||
| for part := range strings.SplitSeq(logLevel, ",") { | ||||||||||||||||||||||||||||||||||||
|
eshitachandwani marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||||
| kv := strings.Split(part, ":") | ||||||||||||||||||||||||||||||||||||
| if len(kv) < 2 { | ||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| component := kv[0] | ||||||||||||||||||||||||||||||||||||
| if component == "" { | ||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| level := kv[1] | ||||||||||||||||||||||||||||||||||||
| switch level { | ||||||||||||||||||||||||||||||||||||
| case "INFO", "info": | ||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here , instead of just checking for all uppercase and all lower case , can we instead convert the input to all lowercase and then just check for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As you mentioned, and as I mentioned in the issue regarding component logging: #3937 (comment), @easwars What do you think about this? 👀
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me notify in case of you missed this comment 🙏 |
||||||||||||||||||||||||||||||||||||
| logLevels[component] = severityInfo | ||||||||||||||||||||||||||||||||||||
| case "WARNING", "warning": | ||||||||||||||||||||||||||||||||||||
| logLevels[component] = severityWarning | ||||||||||||||||||||||||||||||||||||
| case "ERROR", "error": | ||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, we consider an empty string to mean log at
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @easwars |
||||||||||||||||||||||||||||||||||||
| logLevels[component] = severityError | ||||||||||||||||||||||||||||||||||||
| default: | ||||||||||||||||||||||||||||||||||||
| logLevels[component] = severityFatal | ||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the log level does not match any, we might want to let the global default take over for the specific component instead of level being
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also thought it would be better to let the global default logger take over if the log level for a component does not match any know level. Lines 109 to 124 in ef6044e
So, I made the new I think it would be fine to make the new @easwars What do you think about this? 👀
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you say that it implicitly behaves like fatal if an unknown string is passed? My reading of it is that passing an unknown string should just lead to nothing being logged because all three writers would be set to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, sorry I missed that internal.NewLoggerV2 uses the error writer also for the fatal writer: grpc-go/grpclog/internal/loggerv2.go Line 258 in d723006
I made it to use global default logger when it gets an unrecognized string for a component by this commit: d723006 |
||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return logLevels | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // componentData records the settings for a component. | ||||||||||||||||||||||||||||||||||||
| type componentData struct { | ||||||||||||||||||||||||||||||||||||
| name string | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| infoDepth func(depth int, args ...any) | ||||||||||||||||||||||||||||||||||||
| warningDepth func(depth int, args ...any) | ||||||||||||||||||||||||||||||||||||
| errorDepth func(depth int, args ...any) | ||||||||||||||||||||||||||||||||||||
| fatalDepth func(depth int, args ...any) | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+34
to
+37
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need these function pointers? Can't we instead store the severityLevel for this component, and handle it directly in each of its methods? func (c *componentData) InfoDepth(depth int, args ...any) {
if c.level > severityInfo { return } // Fast-path exit
// ... rest of logic
}As an improvement, we could even store the computed prefix in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @easwars
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the meantime, I updated the prefix computation logic: 619a19e |
||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| var cache = map[string]*componentData{} | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| func (c *componentData) InfoDepth(depth int, args ...any) { | ||||||||||||||||||||||||||||||||||||
| args = append([]any{"[" + string(c.name) + "]"}, args...) | ||||||||||||||||||||||||||||||||||||
| InfoDepth(depth+1, args...) | ||||||||||||||||||||||||||||||||||||
| c.infoDepth(depth+1, args...) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| func (c *componentData) WarningDepth(depth int, args ...any) { | ||||||||||||||||||||||||||||||||||||
| args = append([]any{"[" + string(c.name) + "]"}, args...) | ||||||||||||||||||||||||||||||||||||
| WarningDepth(depth+1, args...) | ||||||||||||||||||||||||||||||||||||
| c.warningDepth(depth+1, args...) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| func (c *componentData) ErrorDepth(depth int, args ...any) { | ||||||||||||||||||||||||||||||||||||
| args = append([]any{"[" + string(c.name) + "]"}, args...) | ||||||||||||||||||||||||||||||||||||
| ErrorDepth(depth+1, args...) | ||||||||||||||||||||||||||||||||||||
| c.errorDepth(depth+1, args...) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| func (c *componentData) FatalDepth(depth int, args ...any) { | ||||||||||||||||||||||||||||||||||||
| args = append([]any{"[" + string(c.name) + "]"}, args...) | ||||||||||||||||||||||||||||||||||||
| FatalDepth(depth+1, args...) | ||||||||||||||||||||||||||||||||||||
| c.fatalDepth(depth+1, args...) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| func (c *componentData) Info(args ...any) { | ||||||||||||||||||||||||||||||||||||
|
|
@@ -101,6 +146,9 @@ func (c *componentData) V(l int) bool { | |||||||||||||||||||||||||||||||||||
| return V(l) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| func noopDepth(_ int, _ ...any) { | ||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: You could ignore the blank identifiers in the parameter list because all of them are blank.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated: 1b7e50f |
||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Component creates a new component and returns it for logging. If a component | ||||||||||||||||||||||||||||||||||||
| // with the name already exists, nothing will be created and it will be | ||||||||||||||||||||||||||||||||||||
| // returned. SetLoggerV2 will panic if it is called with a logger created by | ||||||||||||||||||||||||||||||||||||
|
|
@@ -109,7 +157,50 @@ func Component(componentName string) DepthLoggerV2 { | |||||||||||||||||||||||||||||||||||
| if cData, ok := cache[componentName]; ok { | ||||||||||||||||||||||||||||||||||||
| return cData | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| c := &componentData{componentName} | ||||||||||||||||||||||||||||||||||||
| c := &componentData{ | ||||||||||||||||||||||||||||||||||||
| name: componentName, | ||||||||||||||||||||||||||||||||||||
| infoDepth: InfoDepth, | ||||||||||||||||||||||||||||||||||||
| warningDepth: WarningDepth, | ||||||||||||||||||||||||||||||||||||
| errorDepth: ErrorDepth, | ||||||||||||||||||||||||||||||||||||
| fatalDepth: FatalDepth, | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if level, ok := componentLogLevels[componentName]; ok { | ||||||||||||||||||||||||||||||||||||
| c.fatalDepth = func(depth int, args ...any) { | ||||||||||||||||||||||||||||||||||||
| componentFatalDepth(depth+1, args...) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| switch level { | ||||||||||||||||||||||||||||||||||||
| case severityInfo: | ||||||||||||||||||||||||||||||||||||
| c.infoDepth = func(depth int, args ...any) { | ||||||||||||||||||||||||||||||||||||
| componentInfoDepth(depth+1, args...) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| c.warningDepth = func(depth int, args ...any) { | ||||||||||||||||||||||||||||||||||||
| componentWarningDepth(depth+1, args...) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| c.errorDepth = func(depth int, args ...any) { | ||||||||||||||||||||||||||||||||||||
| componentErrorDepth(depth+1, args...) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| case severityWarning: | ||||||||||||||||||||||||||||||||||||
| c.infoDepth = noopDepth | ||||||||||||||||||||||||||||||||||||
| c.warningDepth = func(depth int, args ...any) { | ||||||||||||||||||||||||||||||||||||
| componentWarningDepth(depth+1, args...) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| c.errorDepth = func(depth int, args ...any) { | ||||||||||||||||||||||||||||||||||||
| componentErrorDepth(depth+1, args...) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| case severityError: | ||||||||||||||||||||||||||||||||||||
| c.infoDepth = noopDepth | ||||||||||||||||||||||||||||||||||||
| c.warningDepth = noopDepth | ||||||||||||||||||||||||||||||||||||
| c.errorDepth = func(depth int, args ...any) { | ||||||||||||||||||||||||||||||||||||
| componentErrorDepth(depth+1, args...) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| case severityFatal: | ||||||||||||||||||||||||||||||||||||
| c.infoDepth = noopDepth | ||||||||||||||||||||||||||||||||||||
| c.warningDepth = noopDepth | ||||||||||||||||||||||||||||||||||||
| c.errorDepth = noopDepth | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| cache[componentName] = c | ||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Access to the global
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As gemini mentioned, my implementation does not use a mutex because the original implementation does not use a mutex to protect @eshitachandwani @easwars What do you think about this? 👀
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using a mutex would mean every component trying to initialize the logger would have to acquire the mutex and it will significantly increase the latency, so I dont think using mutex is a good idea.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The mutex is only going to be used when the component is created and not everytime the component is used to log a message. So, I don't think it is going to significantly increase the latency. I think we can go ahead with having the mutex.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated: 5eda131 |
||||||||||||||||||||||||||||||||||||
| return c | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
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.
I think we should consolidate all this information in the godoc for the
grpclogpackage (including the existing paragraph in this section) and simply point to the godoc from here.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.
Updated: 76662e9