Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,28 @@ $ export GRPC_GO_LOG_VERBOSITY_LEVEL=99
$ export GRPC_GO_LOG_SEVERITY_LEVEL=info
```

Additionally, you can configure log severity levels for individual components
using the `GRPC_GO_COMPONENT_LOG_LEVEL` environment variable. The format is a
comma-separated list of `component:LEVEL` pairs like this:

```console
$ export GRPC_GO_COMPONENT_LOG_LEVEL=dns:ERROR,transport:WARNING
```

When both `GRPC_GO_LOG_SEVERITY_LEVEL` and `GRPC_GO_COMPONENT_LOG_LEVEL` are
set, component-specific settings take precedence over `GRPC_GO_LOG_SEVERITY_LEVEL`
for the specified components. Components not listed in
`GRPC_GO_COMPONENT_LOG_LEVEL` will use the `GRPC_GO_LOG_SEVERITY_LEVEL` setting.
For example:

```console
$ export GRPC_GO_LOG_SEVERITY_LEVEL=ERROR
$ export GRPC_GO_COMPONENT_LOG_LEVEL=dns:INFO
```

In this case, the `dns` component will log at `INFO` level, while all other
Copy link
Copy Markdown
Contributor

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 grpclog package (including the existing paragraph in this section) and simply point to the godoc from here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated: 76662e9

components will log at `ERROR` level.

### The RPC failed with error `"code = Unavailable desc = transport is closing"`

This error means the connection the RPC is using was closed, and there are many
Expand Down
101 changes: 96 additions & 5 deletions grpclog/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,33 +20,78 @@ package grpclog

import (
"fmt"
"strings"
)

type severityLevel int
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 grpclog/grpclog.go and not here. This file should only contain the implementation of the component logger.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated: f566320


const (
severityInfo severityLevel = iota
severityWarning
severityError
severityFatal
)

func parseComponentLogLevels(logLevel string) map[string]severityLevel {
if logLevel == "" {
return nil
}

logLevels := make(map[string]severityLevel)
for part := range strings.SplitSeq(logLevel, ",") {
Comment thread
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":
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 info. This is similar to what we do for balancer and resolver registries. But I also see that this is how log levels are checked earlier so maybe @easwars knows if there is a particular reason for that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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),
this code follows what the existing GRPC_GO_LOG_SEVERITY_LEVEL does for case-insensitive level matching.
I also thought it might be better to use strings.ToLower for matching, but I would say we should make two environment variables have the same logic for case-insensitivity.

@easwars What do you think about this? 👀

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@easwars

Let me notify in case of you missed this comment 🙏

logLevels[component] = severityInfo
case "WARNING", "warning":
logLevels[component] = severityWarning
case "ERROR", "error":
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, we consider an empty string to mean log at ERROR as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@easwars
So, do you men if we have GRPC_GO_COMPONENT_LOG_LEVEL as dns:INFO,xds:,authz:ERROR, the log level for xds component should be ERROR instead of make it use the global default logger, right?

logLevels[component] = severityError
default:
logLevels[component] = severityFatal
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 Fatal.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.
However, I found that the existing GRPC_GO_LOG_SEVERITY_LEVEL implicitly behaves like the "FATAL" level if an unknown level string is passed:

grpc-go/grpclog/loggerv2.go

Lines 109 to 124 in ef6044e

func newLoggerV2(w io.Writer, config internal.LoggerV2Config, level string) LoggerV2 {
errorW := io.Discard
warningW := io.Discard
infoW := io.Discard
switch level {
case "", "ERROR", "error": // If env is unset, set level to ERROR.
errorW = w
case "WARNING", "warning":
warningW = w
case "INFO", "info":
infoW = w
}
return internal.NewLoggerV2(infoW, warningW, errorW, config)
}

So, I made the new GRPC_GO_COMPONENT_LOG_LEVEL behave similarly.

I think it would be fine to make the new GRPC_GO_COMPONENT_LOG_LEVEL behave differently from the existing GRPC_GO_LOG_SEVERITY_LEVEL and let the global default logger take over if the log level for a component does not match any as @eshitachandwani mentioned.

@easwars What do you think about this? 👀

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 io.Discard.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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:

fatalW := errorW

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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 componentData struct. I'm talking about [" + string(c.name) + "] and not have to compute that for every log message.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@easwars
The reason why I use function pointers is to avoid checking severity every time a log function is called, by replacing loggers with the noopDepth function according to their levels.
I also think it's OK to handle it directly in each methods.
What do you think about this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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) {
Expand Down Expand Up @@ -101,6 +146,9 @@ func (c *componentData) V(l int) bool {
return V(l)
}

func noopDepth(_ int, _ ...any) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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
Expand All @@ -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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Access to the global cache map is not synchronized. Since Component is a public function that can be called concurrently from multiple goroutines (e.g., during lazy initialization of different gRPC components), this can lead to a concurrent map write panic. Although the original code had this issue, the PR significantly increases the logic within the critical section, making the race more likely. A sync.Mutex or sync.RWMutex should be used to protect the cache.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 cache.

@eshitachandwani @easwars What do you think about this? 👀

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

@110y 110y Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated: 5eda131

return c
}
Loading
Loading