feat(logs): toggle logging for subsystems #711
Conversation
cjcocokrisp
requested review from
chetan-rns,
jannfis,
jgwest and
mikeshng
as code owners
📝 WalkthroughWalkthroughAdds a CentralizedLogger instance API and per-subsystem loggers (resource-proxy, redis-proxy, grpc-event); converts package-level logging helpers into CentralizedLogger methods; adds CLI parsing for subsystem log levels; wires subsystem loggers through agent/principal options and replaces global log() calls with subsystem- or instance-specific loggers. Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
principal/redisproxy/redisproxy.go (2)
89-112: Inconsistent logging:Start()still uses globallog()instead ofrp.logger.The
Start()method uses the globallog()function (lines 92, 102, 109) rather than the instance loggerrp.logger.ModuleLogger("redisProxy"). This defeats the purpose of per-subsystem log level control for these log statements.🔧 Proposed fix
func (rp *RedisProxy) Start() error { + logCtx := rp.logger.ModuleLogger("redisProxy") l, err := net.Listen("tcp", rp.listenAddress) if err != nil { - log().WithError(err).Error("error occurred on listening to addr: " + rp.listenAddress) + logCtx.WithError(err).Error("error occurred on listening to addr: " + rp.listenAddress) return err } rp.listener = l // Start server and connection handler go func() { for { conn, err := l.Accept() if err != nil { - log().WithError(err).Error("error occurred on accepting connection") + logCtx.WithError(err).Error("error occurred on accepting connection") return } go rp.handleConnection(conn) } }() - log().Infof("Redis proxy started on %s", rp.listenAddress) + logCtx.Infof("Redis proxy started on %s", rp.listenAddress) return nil }
708-725: Inconsistent logging inwriteToArgoCDRedisSocket.Line 715 uses
log().WithError(err).Error(...)instead of the instance logger or the providedlogCtx. For consistency with the subsystem logging approach, this should use the logger context.🔧 Proposed fix
func (are *argoCDRedisWriterInternal) writeToArgoCDRedisSocket(logCtx *logrus.Entry, bytes []byte) error { are.argoCDWriteLock.Lock() defer are.argoCDWriteLock.Unlock() _, err := are.fromArgoCDWrite.Write(bytes) if err != nil { - log().WithError(err).Error("unable to write to Argo CD Redis socket") + logCtx.WithError(err).Error("unable to write to Argo CD Redis socket") return err } if err := are.fromArgoCDWrite.Flush(); err != nil { logCtx.WithError(err).Info("unable to flush to Argo CD Redis socket") return err } return nil }principal/apis/eventstream/eventstream.go (1)
102-114: Potential nil pointer dereference ifWithLoggeroption is not provided.
NewServerdoes not set a default logger, butPush()at line 425 directly accessess.options.logger.ModuleLogger(...). If the caller does not useWithLogger, this will panic.Consider setting a default logger:
func NewServer(queues queue.QueuePair, eventWriters *event.EventWritersMap, metrics *metrics.PrincipalMetrics, clusterMgr *cluster.Manager, opts ...ServerOption) *Server { options := &ServerOptions{} for _, o := range opts { o(options) } + if options.logger == nil { + options.logger = logging.GetDefaultLogger() + } return &Server{ queues: queues, options: options, eventWriters: eventWriters, metrics: metrics, clusterMgr: clusterMgr, } }
🤖 Fix all issues with AI agents
In `@agent/agent.go`:
- Around line 119-124: Typo in the comment above the logger fields: change
"contorl" to "control" to correct the comment describing subsystem loggers;
update the comment that sits above the fields resourceProxyLogger,
redisProxyLogger, and grpcEventLogger in the agent struct (in agent.go) to read
"control" so the documentation is accurate.
In `@cmd/argocd-agent/agent.go`:
- Around line 142-148: Rename the misspelled variable resorceProxyLogger to
resourceProxyLogger wherever it's declared and used so the call to
agent.WithSubsystemLoggers(resourceProxyLogger, redisProxyLogger,
grpcEventLogger) passes the correctly named logger; update the declaration that
calls cmdutil.CreateLogger(logLevelResourceProxy, logFormat) and any references
to resorceProxyLogger to the new name to keep identifiers consistent.
In `@cmd/argocd-agent/principal.go`:
- Around line 158-162: The WithSubsystemLoggers implementation incorrectly
assigns the fallback logger to o.options.resourceProxyLogger when grpcEvent is
nil; update the nil-check branch in WithSubsystemLoggers so that when grpcEvent
is nil you create the default logger (using the same creation pattern as the
other fallbacks) and assign it to o.options.grpcEventLogger (not
o.options.resourceProxyLogger), ensuring resourceProxyLogger is not overwritten
and grpcEventLogger is properly initialized.
In `@cmd/cmdutil/log.go`:
- Around line 95-104: The condition checking logLevel != "" is redundant because
logLevel is set to "info" when empty; remove that outer if and always parse the
logLevel using StringToLoglevel(logLevel). Replace the misleading Fatal message
string ("invalid resource proxy log level") with a generic message (e.g.,
"invalid log level") that includes the supplied logLevel and the
AvailableLogLevels() output, and keep calling logger.SetLevel(lvl) after
successful parsing; update references in this block involving the variables
logLevel, StringToLoglevel, AvailableLogLevels, Fatal, and logger.SetLevel.
- Around line 93-130: The CreateLogger function currently adds writer.Hook hooks
but doesn't discard the default logger output, causing duplicate log entries;
update CreateLogger (the function that constructs and returns *logrus.Logger) to
call logger.SetOutput(io.Discard) before adding the two writer.Hook hooks so all
output is routed only through the hooks, and ensure you import io if not already
imported.
In `@docs/configuration/agent/configuration.md`:
- Around line 204-222: The headings for the new subsystem log options "Redis
Proxy Log Level" and "GRPC Event Log Level" are using ### and break the document
hierarchy; change those two headings to #### to match "Resource Proxy Log Level"
and the rest of the file so section depth is consistent, leaving all other
content (flags, env vars, config entries, descriptions, types, defaults, valid
values, examples) unchanged.
In `@docs/configuration/principal/configuration.md`:
- Around line 367-396: The headings "Redis Proxy Log Level" and "GRPC Event Log
Level" are using ### but should be #### to match "Resource Proxy Log Level" and
maintain hierarchy under "Logging and Debugging"; update the markdown headings
for the sections titled "Redis Proxy Log Level" and "GRPC Event Log Level" to
use four hashes (####) instead of three so all three proxy log level subsections
("Resource Proxy Log Level", "Redis Proxy Log Level", "GRPC Event Log Level")
are at the same level.
In `@install/helm-repo/argocd-agent-agent/templates/agent-deployment.yaml`:
- Around line 46-63: The deployment manifest uses incorrect env var names that
don't match the agent code; update the environment variable keys so they match
the names used in cmd/argocd-agent/agent.go: replace
ARGOCD_AGENT_LOG_REDIS_PROXY_LEVEL with ARGOCD_AGENT_REDIS_PROXY_LOG_LEVEL and
replace ARGOCD_AGENT_LOG_GRPC_LOG_LEVEL with ARGOCD_AGENT_GRPC_LOG_LEVEL in the
container env block (the other name ARGOCD_AGENT_RESOURCE_PROXY_LOG_LEVEL is
already correct).
In `@install/helm-repo/argocd-agent-agent/templates/tests/test-configMap.yaml`:
- Around line 78-103: The test script contains copy-paste typos: correct the
three comment lines that currently read "Test agent.log.level" to describe each
specific file (resource-proxy, redis-proxy, grpc), fix the stray "level" suffix
in the cat invocation for agent.log.resource-proxy-level (remove the duplicated
"level"), and correct the misspelled error message "resrouce" to "resource" so
the existence checks and error outputs reference the right keys:
agent.log.resource-proxy-level, agent.log.redis-proxy-level, and
agent.log.grpc-level.
In `@install/kubernetes/agent/agent-deployment.yaml`:
- Around line 42-59: The environment variable names are inconsistent:
ARGOCD_AGENT_RESOURCE_PROXY_LOG_LEVEL, ARGOCD_AGENT_LOG_REDIS_PROXY_LEVEL, and
ARGOCD_AGENT_LOG_GRPC_LOG_LEVEL should follow a single pattern; rename
ARGOCD_AGENT_LOG_REDIS_PROXY_LEVEL → ARGOCD_AGENT_REDIS_PROXY_LOG_LEVEL and
ARGOCD_AGENT_LOG_GRPC_LOG_LEVEL → ARGOCD_AGENT_GRPC_EVENT_LOG_LEVEL (or another
consistent *_<component>_LOG_LEVEL form) and update their corresponding
configMapKeyRef keys (e.g., map agent.log.redis-proxy-level and
agent.log.grpc-level to the new variable names) and any code/templates that
consume these env vars (look for usages of the old symbols to update).
In `@install/kubernetes/principal/principal-deployment.yaml`:
- Around line 42-59: The environment variable names are inconsistent: change
ARGOCD_PRINCIPAL_RESOURCE_PROXY_LOG_LEVEL,
ARGOCD_PRINCIPAL_LOG_REDIS_PROXY_LEVEL, and ARGOCD_PRINCIPAL_LOG_GRPC_LOG_LEVEL
to follow the existing ARGOCD_PRINCIPAL_LOG_LEVEL naming pattern (e.g.,
ARGOCD_PRINCIPAL_LOG_RESOURCE_PROXY_LEVEL,
ARGOCD_PRINCIPAL_LOG_REDIS_PROXY_LEVEL, and ARGOCD_PRINCIPAL_LOG_GRPC_LEVEL) so
all subsystem log level vars use the common
ARGOCD_PRINCIPAL_LOG_<SUBSYSTEM>_LEVEL convention; update the Deployment env var
names accordingly wherever those keys are consumed or referenced.
In `@install/kubernetes/principal/principal-params-cm.yaml`:
- Around line 10-11: The comment for the principal.listen.port key is
corrupted/merged; update the inline comment above or next to
principal.listen.port to a clear, complete sentence such as
"principal.listen.port: The port the gRPC server should listen on; Default:
8443" so operators understand purpose and default; ensure the comment text is
properly separated from the key (e.g., using a leading # and whitespace) and
keep the existing value principal.listen.port: "8443" unchanged.
- Around line 16-26: The comments for the log level keys
(principal.log.resource-proxy-level, principal.log.redis-proxy-level,
principal.log.grpc-level) incorrectly refer to "the agent"; update each comment
to say "the principal" (or "principal" subsystem) instead so the description
matches the keys' namespace—keep the rest of the text and valid values
unchanged.
In `@internal/logging/logging.go`:
- Around line 233-238: The ErrorLogger method on CentralizedLogger calls
err.Error() unguarded which will panic if err is nil; update
CentralizedLogger.ErrorLogger to check whether the err parameter is nil and only
call err.Error() when non-nil (otherwise pass an empty string or a sentinel like
"<nil>") into the logrus.Fields value for logfields.Error so logging remains
safe when callers provide a nil error.
In `@internal/logging/README.md`:
- Around line 133-135: The example uses the removed package-level function
logging.ErrorLogger; update it to call the ErrorLogger method on the default
logger instead (e.g. replace logging.ErrorLogger("ModuleName",
"ConnectionError", err) with logging.Default().ErrorLogger("ModuleName",
"ConnectionError", err)), then use the returned logger instance's Error method
(logger.Error("Error occurred")) so the snippet compiles with the instance-based
API.
In `@principal/options.go`:
- Around line 525-529: The conditional assigns the wrong field when grpcEvent is
nil, leaving o.options.grpcEventLogger unset; change the else branch to set
o.options.grpcEventLogger = logging.GetDefaultLogger() instead of
o.options.resourceProxyLogger so that the grpc event logger is always
initialized (update the block that touches o.options.grpcEventLogger and
o.options.resourceProxyLogger accordingly).
In `@principal/redisproxy/redisproxy.go`:
- Line 245: The inline comment for the variable
forwardRawCommandToPrincipalRedis contains a typo ("falsGetDefaultLogger().e") —
update the comment to read "false" so it correctly explains that the flag is
false when the command has already been processed and true otherwise; locate the
declaration of forwardRawCommandToPrincipalRedis in redisproxy.go and correct
the comment text accordingly.
In `@principal/resourceproxy/options.go`:
- Around line 48-55: WithLogger is incorrectly declared as a method on
*ResourceProxy (receiver rp) but should be a standalone function like
WithTLSConfig and WithRequestMatcher so it can be used as a functional option
during New(); remove the receiver and change the signature to func
WithLogger(logger *logging.CentralizedLogger) ResourceProxyOption, leaving the
body to set p.logger and return nil, and ensure the return type uses the
existing ResourceProxyOption type so callers of New() can pass WithLogger(...)
just like the other option functions.
🧹 Nitpick comments (6)
internal/auth/header/header.go (1)
31-31: Consider lazy logger access to avoid stale entries.
With a replaceable default logger, caching a*logrus.Entryat init time can bypass later logger swaps. Prefer alog()helper that callsGetDefaultLogger()per use, matching other packages.install/helm-repo/argocd-agent-agent/values.schema.json (1)
133-144: Consider adding enum constraints for log level validation.The existing
logFormatproperty uses an enum constraint for validation. For consistency and to prevent invalid configuration values at deploy time, consider adding enum constraints to these new log level properties. Also, the description mentions "debug, info, warn, error" but the codebase also supports "trace" and "fatal" levels.Suggested improvement
"resourceProxyLogLevel": { "type": "string", - "description": "Log level (debug, info, warn, error)" + "description": "Log level for resource proxy subsystem", + "enum": ["trace", "debug", "info", "warn", "error", "fatal"] }, "redisProxyLogLevel": { "type": "string", - "description": "Log level (debug, info, warn, error)" + "description": "Log level for Redis proxy subsystem", + "enum": ["trace", "debug", "info", "warn", "error", "fatal"] }, "grpcEventLogLevel": { "type": "string", - "description": "Log level (debug, info, warn, error)" + "description": "Log level for gRPC events subsystem", + "enum": ["trace", "debug", "info", "warn", "error", "fatal"] },agent/outbound.go (1)
191-195: Consider usinglogfieldsconstants for consistency.The Application event handlers use
logfields.Eventandlogfields.Applicationconstants, but the AppProject and Repository handlers use inline strings like"event"and"appProject". Consider using or adding constants tologfieldsfor consistency across all event types.agent/inbound_redis.go (1)
186-186: Log context fields are lost when reassigning logCtx.The
logCtxparameter passed into this function (containing "method", "uuid", "connectionUUID" fields) is discarded when reassigning to a fresh logger with onlychannelName. Consider usinglogCtx.WithField(...)instead to preserve the existing context:- logCtx = a.redisProxyLogger.ModuleLogger("Agent").WithField(logfields.ChannelName, channelName) + logCtx = logCtx.WithField(logfields.ChannelName, channelName)This preserves debugging context (connection UUID, request UUID) in subscription-related logs.
principal/resourceproxy/resourceproxy.go (1)
189-189: Consider caching the module logger to reduce repeatedModuleLoggercalls.
rp.logger.ModuleLogger("proxy")is called on every log statement inproxyHandler. While not a correctness issue, you could cache the module logger once in the struct during initialization for slight efficiency:type ResourceProxy struct { // ... moduleLogger *logrus.Entry // cached module logger }Then in
New():p.moduleLogger = p.logger.ModuleLogger("proxy")This is optional since the overhead is minimal in practice.
Also applies to: 198-198, 210-210, 225-225, 232-232
internal/logging/logging_test.go (1)
416-416: Typo in test name (Seperate→Separate).Minor clarity tweak; keeps test naming consistent.
✍️ Suggested rename
-func TestSeperateLoggerLevels(t *testing.T) { +func TestSeparateLoggerLevels(t *testing.T) {
cmd/cmdutil/log.go
Outdated
| func CreateLogger(logLevel string, logFormat string) *logrus.Logger { | ||
| logger := logrus.New() | ||
| if logLevel == "" { | ||
| logLevel = "info" | ||
| } | ||
| if logLevel != "" { | ||
| lvl, err := StringToLoglevel(logLevel) | ||
| if err != nil { | ||
| Fatal("invalid resource proxy log level: %s. Available levels are %s", logLevel, AvailableLogLevels()) | ||
| } | ||
| logger.SetLevel(lvl) | ||
| } | ||
| if formatter, err := LogFormatter(logFormat); err != nil { | ||
| Fatal("%s", err.Error()) | ||
| } else { | ||
| logger.SetFormatter(formatter) | ||
| } | ||
|
|
||
| logger.AddHook(&writer.Hook{ | ||
| Writer: os.Stderr, | ||
| LogLevels: []logrus.Level{ | ||
| logrus.PanicLevel, | ||
| logrus.FatalLevel, | ||
| logrus.ErrorLevel, | ||
| logrus.WarnLevel, | ||
| }, | ||
| }) | ||
| logger.AddHook(&writer.Hook{ | ||
| Writer: os.Stdout, | ||
| LogLevels: []logrus.Level{ | ||
| logrus.InfoLevel, | ||
| logrus.DebugLevel, | ||
| logrus.TraceLevel, | ||
| }, | ||
| }) | ||
|
|
||
| return logger | ||
| } |
There was a problem hiding this comment.
Missing SetOutput(io.Discard) may cause duplicate log output.
The InitLogging function (line 57) calls logrus.SetOutput(io.Discard) before adding hooks to prevent logs from being written to both the default output and the hooks. This new function adds the same hooks but doesn't discard the default output, which may result in duplicate log entries.
Proposed fix
func CreateLogger(logLevel string, logFormat string) *logrus.Logger {
logger := logrus.New()
+ logger.SetOutput(io.Discard) // Send all logs to nowhere by default
if logLevel == "" {
logLevel = "info"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func CreateLogger(logLevel string, logFormat string) *logrus.Logger { | |
| logger := logrus.New() | |
| if logLevel == "" { | |
| logLevel = "info" | |
| } | |
| if logLevel != "" { | |
| lvl, err := StringToLoglevel(logLevel) | |
| if err != nil { | |
| Fatal("invalid resource proxy log level: %s. Available levels are %s", logLevel, AvailableLogLevels()) | |
| } | |
| logger.SetLevel(lvl) | |
| } | |
| if formatter, err := LogFormatter(logFormat); err != nil { | |
| Fatal("%s", err.Error()) | |
| } else { | |
| logger.SetFormatter(formatter) | |
| } | |
| logger.AddHook(&writer.Hook{ | |
| Writer: os.Stderr, | |
| LogLevels: []logrus.Level{ | |
| logrus.PanicLevel, | |
| logrus.FatalLevel, | |
| logrus.ErrorLevel, | |
| logrus.WarnLevel, | |
| }, | |
| }) | |
| logger.AddHook(&writer.Hook{ | |
| Writer: os.Stdout, | |
| LogLevels: []logrus.Level{ | |
| logrus.InfoLevel, | |
| logrus.DebugLevel, | |
| logrus.TraceLevel, | |
| }, | |
| }) | |
| return logger | |
| } | |
| func CreateLogger(logLevel string, logFormat string) *logrus.Logger { | |
| logger := logrus.New() | |
| logger.SetOutput(io.Discard) // Send all logs to nowhere by default | |
| if logLevel == "" { | |
| logLevel = "info" | |
| } | |
| if logLevel != "" { | |
| lvl, err := StringToLoglevel(logLevel) | |
| if err != nil { | |
| Fatal("invalid resource proxy log level: %s. Available levels are %s", logLevel, AvailableLogLevels()) | |
| } | |
| logger.SetLevel(lvl) | |
| } | |
| if formatter, err := LogFormatter(logFormat); err != nil { | |
| Fatal("%s", err.Error()) | |
| } else { | |
| logger.SetFormatter(formatter) | |
| } | |
| logger.AddHook(&writer.Hook{ | |
| Writer: os.Stderr, | |
| LogLevels: []logrus.Level{ | |
| logrus.PanicLevel, | |
| logrus.FatalLevel, | |
| logrus.ErrorLevel, | |
| logrus.WarnLevel, | |
| }, | |
| }) | |
| logger.AddHook(&writer.Hook{ | |
| Writer: os.Stdout, | |
| LogLevels: []logrus.Level{ | |
| logrus.InfoLevel, | |
| logrus.DebugLevel, | |
| logrus.TraceLevel, | |
| }, | |
| }) | |
| return logger | |
| } |
🤖 Prompt for AI Agents
In `@cmd/cmdutil/log.go` around lines 93 - 130, The CreateLogger function
currently adds writer.Hook hooks but doesn't discard the default logger output,
causing duplicate log entries; update CreateLogger (the function that constructs
and returns *logrus.Logger) to call logger.SetOutput(io.Discard) before adding
the two writer.Hook hooks so all output is routed only through the hooks, and
ensure you import io if not already imported.
| # principal.log.resource-proxy-level: The log level the agent should use for the resource proxy. | ||
| # Valid values are trace, debug, info, warn and error. | ||
| # Default: "info" | ||
| principal.log.resource-proxy-level: "info" | ||
| # principal.log.redis-proxy-level: The log level the agent should use for the redis proxy. | ||
| # Valid values are trace, debug, info, warn and error. | ||
| # Default: "info" | ||
| principal.log.redis-proxy-level: "info" | ||
| # principal.log.grpc-level: The log level the agent should use for grpc events. | ||
| # Valid values are trace, debug, info, warn and error. | ||
| # Default: "info" |
There was a problem hiding this comment.
Use “principal” (not “agent”) in subsystem log comments.
These keys live under principal.*, so the current wording is misleading.
💡 Suggested fix
- # principal.log.resource-proxy-level: The log level the agent should use for the resource proxy.
+ # principal.log.resource-proxy-level: The log level the principal should use for the resource proxy.
...
- # principal.log.redis-proxy-level: The log level the agent should use for the redis proxy.
+ # principal.log.redis-proxy-level: The log level the principal should use for the redis proxy.
...
- # principal.log.grpc-level: The log level the agent should use for grpc events.
+ # principal.log.grpc-level: The log level the principal should use for gRPC events.🤖 Prompt for AI Agents
In `@install/kubernetes/principal/principal-params-cm.yaml` around lines 16 - 26,
The comments for the log level keys (principal.log.resource-proxy-level,
principal.log.redis-proxy-level, principal.log.grpc-level) incorrectly refer to
"the agent"; update each comment to say "the principal" (or "principal"
subsystem) instead so the description matches the keys' namespace—keep the rest
of the text and valid values unchanged.
| func (l *CentralizedLogger) ErrorLogger(module, errorType string, err error) *logrus.Entry { | ||
| return l.logger.WithFields(logrus.Fields{ | ||
| logfields.Module: module, | ||
| logfields.ErrorType: errorType, | ||
| logfields.Error: err.Error(), | ||
| }) |
There was a problem hiding this comment.
Guard against nil error to avoid panic in ErrorLogger.
err.Error() will panic if err is nil. Logging should be safe even when callers don’t have an error instance.
🐛 Suggested fix
func (l *CentralizedLogger) ErrorLogger(module, errorType string, err error) *logrus.Entry {
- return l.logger.WithFields(logrus.Fields{
- logfields.Module: module,
- logfields.ErrorType: errorType,
- logfields.Error: err.Error(),
- })
+ fields := logrus.Fields{
+ logfields.Module: module,
+ logfields.ErrorType: errorType,
+ }
+ if err != nil {
+ fields[logfields.Error] = err.Error()
+ }
+ return l.logger.WithFields(fields)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (l *CentralizedLogger) ErrorLogger(module, errorType string, err error) *logrus.Entry { | |
| return l.logger.WithFields(logrus.Fields{ | |
| logfields.Module: module, | |
| logfields.ErrorType: errorType, | |
| logfields.Error: err.Error(), | |
| }) | |
| func (l *CentralizedLogger) ErrorLogger(module, errorType string, err error) *logrus.Entry { | |
| fields := logrus.Fields{ | |
| logfields.Module: module, | |
| logfields.ErrorType: errorType, | |
| } | |
| if err != nil { | |
| fields[logfields.Error] = err.Error() | |
| } | |
| return l.logger.WithFields(fields) | |
| } |
🤖 Prompt for AI Agents
In `@internal/logging/logging.go` around lines 233 - 238, The ErrorLogger method
on CentralizedLogger calls err.Error() unguarded which will panic if err is nil;
update CentralizedLogger.ErrorLogger to check whether the err parameter is nil
and only call err.Error() when non-nil (otherwise pass an empty string or a
sentinel like "<nil>") into the logrus.Fields value for logfields.Error so
logging remains safe when callers provide a nil error.
| parsedReceived := parsedRedisCommandVal.parsedReceived | ||
|
|
||
| forwardRawCommandToPrincipalRedis := true // whether to send the command to principal redis: false if the command has already been processed, true otherwise. | ||
| forwardRawCommandToPrincipalRedis := true // whether to send the command to principal redis: falsGetDefaultLogger().e if the command has already been processed, true otherwise. |
There was a problem hiding this comment.
Fix typo in comment.
The comment contains a corrupted word: falsGetDefaultLogger().e should be false.
📝 Proposed fix
- forwardRawCommandToPrincipalRedis := true // whether to send the command to principal redis: falsGetDefaultLogger().e if the command has already been processed, true otherwise.
+ forwardRawCommandToPrincipalRedis := true // whether to send the command to principal redis: false if the command has already been processed, true otherwise.🤖 Prompt for AI Agents
In `@principal/redisproxy/redisproxy.go` at line 245, The inline comment for the
variable forwardRawCommandToPrincipalRedis contains a typo
("falsGetDefaultLogger().e") — update the comment to read "false" so it
correctly explains that the flag is false when the command has already been
processed and true otherwise; locate the declaration of
forwardRawCommandToPrincipalRedis in redisproxy.go and correct the comment text
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
principal/event.go (1)
791-798: Inconsistent logger usage inStartEventProcessor.Line 794 still uses the old
log()pattern while the rest of the event processing code usess.options.grpcEventLogger. Consider updating for consistency.✏️ Proposed fix
func (s *Server) StartEventProcessor(ctx context.Context) error { var err error go func() { - log().Infof("Starting event processor") + s.options.grpcEventLogger.ModuleLogger("grpc").Info("Starting event processor") err = s.eventProcessor(ctx) }() return err }agent/resource.go (1)
47-51: Add default initialization for resourceProxyLogger to prevent nil pointer dereference.The
resourceProxyLoggeris not initialized whenNewAgent()is called without theWithSubsystemLoggersoption. SinceenableResourceProxydefaults to true, callingprocessIncomingResourceRequest()will panic at line 47 when invokinga.resourceProxyLogger.ModuleLogger()on a nil receiver.Unlike the principal server (which provides
logging.GetDefaultLogger()as a fallback in itsWithSubsystemLoggers), the agent lacks this protection. Either provide a default logger inNewAgent()or ensureWithSubsystemLoggersis always called with non-nil arguments.principal/apis/eventstream/eventstream.go (1)
101-114: Potential nil pointer dereference: missing default logger initialization.Unlike
ResourceProxy.New(lines 112-114 in resourceproxy.go) andNewServerin server.go (lines 216-226), this constructor doesn't defaultoptions.loggerwhenWithLoggerisn't called. This will cause a nil pointer dereference at line 425 whenPushis invoked.🔧 Proposed fix
func NewServer(queues queue.QueuePair, eventWriters *event.EventWritersMap, metrics *metrics.PrincipalMetrics, clusterMgr *cluster.Manager, opts ...ServerOption) *Server { options := &ServerOptions{} for _, o := range opts { o(options) } + if options.logger == nil { + options.logger = logging.GetDefaultLogger() + } return &Server{ queues: queues, options: options, eventWriters: eventWriters, metrics: metrics, clusterMgr: clusterMgr, } }principal/resourceproxy/resourceproxy.go (1)
119-131: Inconsistent logger usage for HTTP error logging.Line 121 uses the
log()helper function which always returnsGetDefaultLogger().ModuleLogger("proxy"), bypassing the injectedrp.logger. This means HTTP server errors won't respect a custom subsystem log level if one was configured.🔧 Proposed fix
p.mux = http.NewServeMux() // proxyHandler is the main HTTP handler, used to process every request p.mux.HandleFunc("/", p.proxyHandler) - httpLogger := log().WriterLevel(logrus.ErrorLevel) + httpLogger := p.logger.ModuleLogger("proxy").WriterLevel(logrus.ErrorLevel) // Configure the HTTP server p.server = &http.Server{principal/listen.go (1)
103-129: Use the actual listen error in the retry log.
Line 118 logserr, which is the outer ExponentialBackoff error and is likely nil during retries. Uselerrto preserve the root cause.🐛 Proposed fix
- s.options.grpcEventLogger.ModuleLogger("grpc").WithError(err).Debugf("Retrying to start TCP listener on %s (retry %d/%d)", bind, try, listenerRetries) + s.options.grpcEventLogger.ModuleLogger("grpc").WithError(lerr).Debugf("Retrying to start TCP listener on %s (retry %d/%d)", bind, try, listenerRetries)
♻️ Duplicate comments (16)
docs/configuration/principal/configuration.md (1)
377-395: Fix subsection heading levels under “Logging and Debugging”.
The Redis Proxy Log Level and GRPC Event Log Level headings should use####to match the Resource Proxy Log Level subsection hierarchy.✏️ Proposed fix
-### Redis Proxy Log Level +#### Redis Proxy Log Level ... -### GRPC Event Log Level +#### GRPC Event Log Levelinstall/kubernetes/principal/principal-params-cm.yaml (2)
10-11: Repair the corrupted listen-port comment.
The inline description is merged and unclear; split it for readability.✏️ Proposed fix
- # principal.listen.port: The port the gRPC server should listenprincipal # Default: 8443 + # principal.listen.port: The port the gRPC server should listen on. + # Default: 8443
16-26: Use “principal” in subsystem log level comments.
These keys live underprincipal.*, so the comments should reference the principal component.✏️ Proposed fix
- # principal.log.resource-proxy-level: The log level the agent should use for the resource proxy. + # principal.log.resource-proxy-level: The log level the principal should use for the resource proxy. ... - # principal.log.redis-proxy-level: The log level the agent should use for the redis proxy. + # principal.log.redis-proxy-level: The log level the principal should use for the redis proxy. ... - # principal.log.grpc-level: The log level the agent should use for grpc events. + # principal.log.grpc-level: The log level the principal should use for gRPC events.principal/redisproxy/redisproxy.go (1)
245-245: Fix typo in inline comment.
The comment text is corrupted (“falsGetDefaultLogger().e”).✏️ Proposed fix
- forwardRawCommandToPrincipalRedis := true // whether to send the command to principal redis: falsGetDefaultLogger().e if the command has already been processed, true otherwise. + forwardRawCommandToPrincipalRedis := true // whether to send the command to principal redis: false if the command has already been processed, true otherwise.internal/logging/logging.go (1)
233-238: Guard against nil error inErrorLoggerto prevent panic.
err.Error()will panic whenerris nil.🐛 Proposed fix
func (l *CentralizedLogger) ErrorLogger(module, errorType string, err error) *logrus.Entry { - return l.logger.WithFields(logrus.Fields{ - logfields.Module: module, - logfields.ErrorType: errorType, - logfields.Error: err.Error(), - }) + fields := logrus.Fields{ + logfields.Module: module, + logfields.ErrorType: errorType, + } + if err != nil { + fields[logfields.Error] = err.Error() + } + return l.logger.WithFields(fields) }cmd/cmdutil/log.go (2)
111-127: Duplicate log output risk when using hooks.
Because the logger writes to its default output in addition to the stdout/stderr hooks, log lines can be duplicated unless the base output is discarded.🛠️ Proposed fix
func CreateLogger(logLevel string, logFormat string) *logrus.Logger { logger := logrus.New() + logger.SetOutput(io.Discard) // prevent duplicate output when hooks are installed if logLevel == "" { logLevel = "info" }
95-104: Redundant condition and misleading error message.
ThelogLevel != ""check is always true after defaulting, and the error text is subsystem-specific in a generic helper.✏️ Proposed fix
if logLevel == "" { logLevel = "info" } - if logLevel != "" { - lvl, err := StringToLoglevel(logLevel) - if err != nil { - Fatal("invalid resource proxy log level: %s. Available levels are %s", logLevel, AvailableLogLevels()) - } - logger.SetLevel(lvl) - } + lvl, err := StringToLoglevel(logLevel) + if err != nil { + Fatal("invalid log level: %s. Available levels are %s", logLevel, AvailableLogLevels()) + } + logger.SetLevel(lvl)agent/agent.go (1)
120-123: Typo in comment (“contorl”).✏️ Proposed fix
- // below are loggers to contorl log levels of different subsystems + // below are loggers to control log levels of different subsystemscmd/argocd-agent/agent.go (1)
143-147: Fix typo in variable name:resorceProxyLogger→resourceProxyLogger.The variable name has a typo - missing the letter 'u' in "resource".
✏️ Proposed fix
- resorceProxyLogger := cmdutil.CreateLogger(logLevelResourceProxy, logFormat) + resourceProxyLogger := cmdutil.CreateLogger(logLevelResourceProxy, logFormat) redisProxyLogger := cmdutil.CreateLogger(logLevelRedisProxy, logFormat) grpcEventLogger := cmdutil.CreateLogger(logLevelGrpcEvent, logFormat) - agentOpts = append(agentOpts, agent.WithSubsystemLoggers(resorceProxyLogger, redisProxyLogger, grpcEventLogger)) + agentOpts = append(agentOpts, agent.WithSubsystemLoggers(resourceProxyLogger, redisProxyLogger, grpcEventLogger))internal/logging/README.md (1)
130-136: Fix ErrorLogger example to use the instance-based API.Line 134 calls
logging.ErrorLogger(...), but the new API exposesErrorLoggeras a method on the default logger. This is inconsistent with all other examples in this document and won't compile.📝 Suggested fix
#### Error Handling ```go err := errors.New("connection failed") -logger := logging.ErrorLogger("ModuleName", "ConnectionError", err) +logger := logging.GetDefaultLogger().ErrorLogger("ModuleName", "ConnectionError", err) logger.Error("Error occurred")</details> </blockquote></details> <details> <summary>install/kubernetes/principal/principal-deployment.yaml (1)</summary><blockquote> `42-59`: **Env var naming inconsistency for subsystem log levels.** </blockquote></details> <details> <summary>principal/resourceproxy/options.go (1)</summary><blockquote> `48-55`: **Make `WithLogger` a standalone option function (receiver not needed).** <details> <summary>🔧 Proposed fix</summary> ```diff -func (rp *ResourceProxy) WithLogger(logger *logging.CentralizedLogger) ResourceProxyOption { +func WithLogger(logger *logging.CentralizedLogger) ResourceProxyOption { return func(p *ResourceProxy) error { p.logger = logger return nil } }install/helm-repo/argocd-agent-agent/templates/agent-deployment.yaml (1)
52-59: Fix redis/grpc log-level env var names to match the agent code. The current names include an extraLOG_segment, so the agent won’t pick them up.🐛 Proposed fix
- - name: ARGOCD_AGENT_LOG_REDIS_PROXY_LEVEL + - name: ARGOCD_AGENT_REDIS_PROXY_LOG_LEVEL valueFrom: configMapKeyRef: name: {{ include "argocd-agent-agent.paramsConfigMapName" . }} key: agent.log.redis-proxy-level optional: true - - name: ARGOCD_AGENT_LOG_GRPC_LOG_LEVEL + - name: ARGOCD_AGENT_GRPC_LOG_LEVEL valueFrom: configMapKeyRef: name: {{ include "argocd-agent-agent.paramsConfigMapName" . }} key: agent.log.grpc-level optional: trueinstall/helm-repo/argocd-agent-agent/templates/tests/test-configMap.yaml (1)
78-103: Fix typos in the new ConfigMap tests (they will fail as written). The file name and error message contain copy‑paste mistakes.🐛 Proposed fix
- # Test agent.log.level + # Test agent.log.resource-proxy-level if [ -f /etc/config/agent.log.resource-proxy-level ]; then echo "✓ agent.log.resource-proxy-level exists" - cat /etc/config/agent.log.resource-proxy-levellevel + cat /etc/config/agent.log.resource-proxy-level else - echo "✗ agent.log.resrouce-proxy-levellevel missing" + echo "✗ agent.log.resource-proxy-level missing" exit 1 fi - # Test agent.log.level + # Test agent.log.redis-proxy-level if [ -f /etc/config/agent.log.redis-proxy-level ]; then echo "✓ agent.log.redis-proxy-level exists" cat /etc/config/agent.log.redis-proxy-level else echo "✗ agent.log.redis-proxy-level missing" exit 1 fi - # Test agent.log.level + # Test agent.log.grpc-level if [ -f /etc/config/agent.log.grpc-level ]; then echo "✓ agent.log.grpc-level exists" cat /etc/config/agent.log.grpc-level else echo "✗ agent.log.grpc-level missing" exit 1 fiinstall/kubernetes/agent/agent-deployment.yaml (1)
42-59: Environment variable naming inconsistency.The naming patterns are inconsistent across the three new environment variables:
ARGOCD_AGENT_RESOURCE_PROXY_LOG_LEVEL— uses*_<COMPONENT>_LOG_LEVELARGOCD_AGENT_LOG_REDIS_PROXY_LEVEL— uses*_LOG_<COMPONENT>_LEVELARGOCD_AGENT_LOG_GRPC_LOG_LEVEL— has redundantLOG(LOG_GRPC_LOG_LEVEL)Consider adopting a consistent pattern such as
ARGOCD_AGENT_<COMPONENT>_LOG_LEVEL:
ARGOCD_AGENT_RESOURCE_PROXY_LOG_LEVEL✓ARGOCD_AGENT_REDIS_PROXY_LOG_LEVELARGOCD_AGENT_GRPC_EVENT_LOG_LEVELdocs/configuration/agent/configuration.md (1)
204-222: Inconsistent heading levels for new subsystem log options.Lines 204 and 214 use
###heading level while the preceding "Resource Proxy Log Level" (line 194) and all other parameters use####. This creates inconsistent document hierarchy.📝 Suggested fix
-### Redis Proxy Log Level +#### Redis Proxy Log Level ... -### GRPC Event Log Level +#### GRPC Event Log Level
🧹 Nitpick comments (3)
internal/logging/logging_test.go (1)
416-416: Fix typo in test name:TestSeperateLoggerLevels→TestSeparateLoggerLevels.Minor spelling correction for consistency.
✏️ Proposed fix
-func TestSeperateLoggerLevels(t *testing.T) { +func TestSeparateLoggerLevels(t *testing.T) {principal/apis/eventstream/eventstream.go (1)
496-499: Remove commented-out dead code.This commented-out helper function and the uncertainty comment should be removed. The logging refactor is complete—call sites now use the injected logger instance directly.
🧹 Proposed cleanup
-// Not sure what to do with below! -// func log() *logrus.Entry { -// return logging.GetDefaultLogger().ModuleLogger("grpc.AppStream") -// }principal/resourceproxy/resourceproxy.go (1)
236-238: Consider removing unusedlog()helper or updating its usage.The
log()helper is now only used at line 121 for HTTP error logging. If line 121 is updated to usep.logger, this helper can be removed entirely to complete the centralized logging migration.
cjcocokrisp
force-pushed
the
feat/toggle-logging-for-subsystems
branch
4 times, most recently
from
80d6a8a to
fd5068d
Compare
|
Great stuff, @cjcocokrisp. Let me think a while about your thoughts of having package-scoped constants. I think we used a package-private function to initialize/get the logger for a while, not sure if we still do. But that might work too, i.e. it was/is However, I have one thought already: I think having dedicated command line flags (and all that which comes with it, such as new environment variables, changes to K8s resources, etc) will not scale. I would prefer keeping a single So all we need to introduce for new components is a new logger, a new keyword to parse and surrounding logic to initialize that specific logger, instead of having to introduce a new command line switch etc. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/argocd-agent/agent.go`:
- Around line 142-147: CreateLogger currently hard-codes "resource proxy" in its
invalid-level error which is misleading when used for redis/grpc loggers; change
cmdutil.CreateLogger to either accept a subsystem name parameter (e.g.,
CreateLogger(level, format, subsystemName)) or make its error message generic
("invalid log level: %q") and include a passed-in subsystemName in the emitted
log/returned error. Update all call sites (resourceProxyLogger,
redisProxyLogger, grpcEventLogger where cmdutil.CreateLogger is invoked) to pass
the correct subsystem identifier or rely on the generic message, and ensure
agent.WithSubsystemLoggers continues to receive the three logger objects
unchanged.
|
@jannfis Sounds good thanks for the prompt review and thoughts so far. I do agree that the comma separated values is a better way to scale. Will go and make those changes tomorrow. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/cmdutil/log_test.go`:
- Around line 113-124: Tests mutate global logrus state via logrus.SetOutput and
logrus.SetLevel; save the original global state at the start of each subtest
(e.g., origLevel := logrus.GetLevel() and origOut :=
logrus.StandardLogger().Out) and restore it with a defer
(logrus.SetLevel(origLevel); logrus.SetOutput(origOut)) so ParseLogLevels and
test-level output changes do not leak between tests; update the loop surrounding
ParseLogLevels/SubSystemLoggers accordingly.
♻️ Duplicate comments (1)
cmd/cmdutil/log.go (1)
102-126: Avoid duplicate log lines by discarding the default logger output.
With hooks installed, leaving the logger’s default output enabled can emit duplicates.💡 Proposed fix
func CreateLogger(logFormat string) *logrus.Logger { logger := logrus.New() + logger.SetOutput(io.Discard) // route output solely through hooks if formatter, err := LogFormatter(logFormat); err != nil { Fatal("%s", err.Error()) } else { logger.SetFormatter(formatter) }
|
I altered the log level CLI flag to now use the comma separated list and added a parsing helper function for that along with some tests for it let me know what you think about doing that. I made it so the general level (not specifying a subsystem) can be placed anywhere in the list just for convince. So to add a logger you would just need to update the parser function and add an entry to the struct then link to the component its used for. I also reverted the k8s manifests. I did think about one thing for the constants for the logs. As I mentioned the big issue is needing to replace the calls to the Still need to fix a regression in the unit tests I'm probably just not setting a default log somewhere. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/configuration/principal/configuration.md`:
- Around line 341-347: The example in the log level documentation uses an
invalid level "warning"; update the example string to use a documented level
(e.g., replace "info,grpc-event=warning" with "info,grpc-event=warn") so it
matches the valid values list; ensure the Format/Example section referencing the
log level and the example string is updated accordingly.
bfa345e to
bc179be
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@agent/agent.go`:
- Around line 175-176: Remove the leftover debugging stdout prints: delete the
two fmt.Println(...) calls that print a.redisProxyLogger and
logging.GetDefaultLogger() in agent.go; if the intent was to inspect those
values, replace them with structured logging via the existing logger (e.g., use
a.redisProxyLogger.Debugf/Infof or logging.GetDefaultLogger().Debug/Info) so
output goes through the app's logging system instead of raw fmt.Println.
4775b80 to
ff5de48
Compare
|
@cjcocokrisp #712 has changed the documentation structure and layout a bit, I think that's where the conflict is coming from. |
cjcocokrisp
force-pushed
the
feat/toggle-logging-for-subsystems
branch
from
ff5de48 to
e1e81dd
Compare
|
Just fixed the conflicts. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@cmd/argocd-agent/agent.go`:
- Around line 257-260: The --log-level flag help text is unclear about accepted
syntax; update the StringSliceVar call that registers the flag
(command.Flags().StringSliceVar with variable logLevels and flag name
"log-level") to describe that values are comma-separated and may be qualified by
subsystem (e.g. subsystem:level or subsystem1,subsystem2:level), and provide the
default of "info" in the message so users know both format and default.
In `@cmd/argocd-agent/principal.go`:
- Around line 344-346: Update the flag help for the "log-level" flag (where
command.Flags().StringSliceVar(&logLevels, "log-level", ... ) is defined) to
explicitly state that values are comma-separated and that
subsystem/component-qualified entries are allowed in the form "subsystem=level"
(e.g. "grpc=debug,store=warn") and note the default "info"; change the
description string accordingly to reflect this syntax.
In `@docs/configuration/reference/principal.md`:
- Around line 402-406: The docs list of log levels in principal.md is missing
"fatal" while the parser (StringToLoglevel in cmd/cmdutil/log.go) accepts it;
update the documentation to include `fatal` in the **Valid Values (level)** list
(trace, debug, info, warning, error, fatal) so the docs match the parser, or
alternatively remove/deny `fatal` from StringToLoglevel—ensure the change is
applied consistently and mention StringToLoglevel in the commit message.
In `@internal/logging/logging_test.go`:
- Around line 395-399: The test calls defaultLogger.SetupLogging and
newLogger.SetupLogging without checking their returned errors, so failures are
silent; update the test to capture the returned error from
defaultLogger.SetupLogging(LogLevelInfo, LogFormatJSON, &defaultBuf) and from
newLogger.SetupLogging(LogLevelDebug, LogFormatJSON, &newBuf) and immediately
fail the test if non-nil (e.g., using t.Fatalf or require.NoError) so any setup
failure is surfaced before further assertions.
In `@principal/listen.go`:
- Around line 116-119: The log is attaching the wrong error variable; in the
retry block after c, lerr = net.Listen("tcp", bind) replace the outer-scope err
used in the logger with the actual listen error lerr by changing the WithError
call to use lerr (locate the ModuleLogger("grpc").WithError(err).Debugf(...)
call in the retry loop inside listen.go and swap err -> lerr).
In `@principal/resourceproxy/options.go`:
- Around line 48-55: The WithLogger option currently assigns whatever is passed
into p.logger which can set it to nil and later cause panics; update the
WithLogger(ResourceProxyOption) closure to guard against nil by checking the
incoming logger and assigning logging.GetDefaultLogger() (or the package's
default logger accessor) when logger == nil, then set p.logger accordingly and
return nil—this preserves the ResourceProxy default logger behavior and prevents
nil derefs in methods on ResourceProxy.
♻️ Duplicate comments (3)
internal/logging/logging.go (1)
233-238: Guard nil error in ErrorLogger to avoid panic.
err.Error()will panic iferris nil.🛠️ Suggested fix
func (l *CentralizedLogger) ErrorLogger(module, errorType string, err error) *logrus.Entry { - return l.logger.WithFields(logrus.Fields{ - logfields.Module: module, - logfields.ErrorType: errorType, - logfields.Error: err.Error(), - }) + fields := logrus.Fields{ + logfields.Module: module, + logfields.ErrorType: errorType, + } + if err != nil { + fields[logfields.Error] = err.Error() + } + return l.logger.WithFields(fields) }internal/logging/README.md (1)
131-135: Fix ErrorLogger example to use instance-based API.The package-level
logging.ErrorLoggerno longer exists.📝 Suggested doc fix
-logger := logging.ErrorLogger("ModuleName", "ConnectionError", err) +logger := logging.GetDefaultLogger().ErrorLogger("ModuleName", "ConnectionError", err)cmd/cmdutil/log.go (1)
102-128: Avoid duplicate log output by discarding the base logger output.Line 103 creates a new logger and adds hooks, but without
SetOutput(io.Discard)the default output will still emit, producing duplicate entries.🛠️ Proposed fix
func CreateLogger(logFormat string) *logrus.Logger { logger := logrus.New() + logger.SetOutput(io.Discard) // prevent duplicate output (hooks handle routing) if formatter, err := LogFormatter(logFormat); err != nil { Fatal("%s", err.Error()) } else { logger.SetFormatter(formatter) }
🧹 Nitpick comments (3)
internal/logging/logging_test.go (1)
416-416: Minor naming nit: “Separate” spelling.✍️ Suggested tweak
-func TestSeperateLoggerLevels(t *testing.T) { +func TestSeparateLoggerLevels(t *testing.T) {agent/inbound.go (1)
637-638: Preserve existing log context when unmanaging resources.
Using the prebuiltlogCtxkeeps method/resource fields attached to the warning.♻️ Suggested adjustment
- a.grpcEventLogger.ModuleLogger("Agent").Warnf("Could not unmanage app %s: %v", app.QualifiedName(), err) + logCtx.WithError(err).Warnf("Could not unmanage app %s", app.QualifiedName()) - a.grpcEventLogger.ModuleLogger("Agent").Warnf("Could not unmanage appProject %s: %v", project.Name, err) + logCtx.WithError(err).Warnf("Could not unmanage appProject %s", project.Name) - a.grpcEventLogger.ModuleLogger("Agent").Warnf("Could not unmanage repository %s: %v", repo.Name, err) + logCtx.WithError(err).Warnf("Could not unmanage repository %s", repo.Name)Also applies to: 757-758, 874-875
principal/apis/eventstream/eventstream.go (1)
95-98: Consider routing Subscribe/recv/send logs through the injected logger too.
Those paths still use the global logrus entry, so subsystem log levels won’t apply there.
| command.Flags().StringSliceVar(&logLevels, "log-level", | ||
| env.StringSliceWithDefault("ARGOCD_AGENT_LOG_LEVEL", nil, []string{"info"}), | ||
| "The log level for the agent") | ||
|
|
There was a problem hiding this comment.
Clarify --log-level help text for subsystem syntax.
The description should reflect comma-separated/component-qualified values.
✏️ Suggested tweak
- "The log level for the agent")
+ "Log levels (comma-separated; optional subsystem=level, e.g. \"info,grpc-event=trace\")")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| command.Flags().StringSliceVar(&logLevels, "log-level", | |
| env.StringSliceWithDefault("ARGOCD_AGENT_LOG_LEVEL", nil, []string{"info"}), | |
| "The log level for the agent") | |
| command.Flags().StringSliceVar(&logLevels, "log-level", | |
| env.StringSliceWithDefault("ARGOCD_AGENT_LOG_LEVEL", nil, []string{"info"}), | |
| "Log levels (comma-separated; optional subsystem=level, e.g. \"info,grpc-event=trace\")") |
🤖 Prompt for AI Agents
In `@cmd/argocd-agent/agent.go` around lines 257 - 260, The --log-level flag help
text is unclear about accepted syntax; update the StringSliceVar call that
registers the flag (command.Flags().StringSliceVar with variable logLevels and
flag name "log-level") to describe that values are comma-separated and may be
qualified by subsystem (e.g. subsystem:level or subsystem1,subsystem2:level),
and provide the default of "info" in the message so users know both format and
default.
| command.Flags().StringSliceVar(&logLevels, "log-level", | ||
| env.StringSliceWithDefault("ARGOCD_PRINCIPAL_LOG_LEVEL", nil, []string{"info"}), | ||
| "The log level to use") |
There was a problem hiding this comment.
Clarify --log-level help text for subsystem syntax.
Current description doesn’t indicate comma-separated or component-qualified values.
✏️ Suggested tweak
- "The log level to use")
+ "Log levels (comma-separated; optional subsystem=level, e.g. \"info,redis-proxy=debug\")")🤖 Prompt for AI Agents
In `@cmd/argocd-agent/principal.go` around lines 344 - 346, Update the flag help
for the "log-level" flag (where command.Flags().StringSliceVar(&logLevels,
"log-level", ... ) is defined) to explicitly state that values are
comma-separated and that subsystem/component-qualified entries are allowed in
the form "subsystem=level" (e.g. "grpc=debug,store=warn") and note the default
"info"; change the description string accordingly to reflect this syntax.
| | **Type** | String (comma-separated list) | | ||
| | **Default** | `info` | | ||
| | **Valid Values** | `trace`, `debug`, `info`, `warn`, `error` | | ||
| | **Format** | `[<component>=]<level>` | | ||
| | **Valid Values (component)** | `resource-proxy`, `redis-proxy`, `grpc-event` | | ||
| | **Valid Values (level)** | `trace`, `debug`, `info`, `warning`, `error` | |
There was a problem hiding this comment.
Align documented log levels with actual parser.
Line 406 lists levels but omits fatal, which is accepted by StringToLoglevel in cmd/cmdutil/log.go. Either document fatal here or reject it in parsing for consistency.
📝 Suggested doc tweak
-| **Valid Values (level)** | `trace`, `debug`, `info`, `warning`, `error` |
+| **Valid Values (level)** | `trace`, `debug`, `info`, `warning`, `error`, `fatal` |🤖 Prompt for AI Agents
In `@docs/configuration/reference/principal.md` around lines 402 - 406, The docs
list of log levels in principal.md is missing "fatal" while the parser
(StringToLoglevel in cmd/cmdutil/log.go) accepts it; update the documentation to
include `fatal` in the **Valid Values (level)** list (trace, debug, info,
warning, error, fatal) so the docs match the parser, or alternatively
remove/deny `fatal` from StringToLoglevel—ensure the change is applied
consistently and mention StringToLoglevel in the commit message.
| defaultLogger.SetupLogging(LogLevelInfo, LogFormatJSON, &defaultBuf) | ||
|
|
||
| newLogger := New(nil) | ||
| newLogger.SetupLogging(LogLevelDebug, LogFormatJSON, &newBuf) | ||
|
|
There was a problem hiding this comment.
Add error checks for SetupLogging in this test.
Right now failures would be silent and could produce misleading assertions.
🧪 Proposed fix
- defaultLogger.SetupLogging(LogLevelInfo, LogFormatJSON, &defaultBuf)
+ require.NoError(t, defaultLogger.SetupLogging(LogLevelInfo, LogFormatJSON, &defaultBuf))
newLogger := New(nil)
- newLogger.SetupLogging(LogLevelDebug, LogFormatJSON, &newBuf)
+ require.NoError(t, newLogger.SetupLogging(LogLevelDebug, LogFormatJSON, &newBuf))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defaultLogger.SetupLogging(LogLevelInfo, LogFormatJSON, &defaultBuf) | |
| newLogger := New(nil) | |
| newLogger.SetupLogging(LogLevelDebug, LogFormatJSON, &newBuf) | |
| require.NoError(t, defaultLogger.SetupLogging(LogLevelInfo, LogFormatJSON, &defaultBuf)) | |
| newLogger := New(nil) | |
| require.NoError(t, newLogger.SetupLogging(LogLevelDebug, LogFormatJSON, &newBuf)) |
🤖 Prompt for AI Agents
In `@internal/logging/logging_test.go` around lines 395 - 399, The test calls
defaultLogger.SetupLogging and newLogger.SetupLogging without checking their
returned errors, so failures are silent; update the test to capture the returned
error from defaultLogger.SetupLogging(LogLevelInfo, LogFormatJSON, &defaultBuf)
and from newLogger.SetupLogging(LogLevelDebug, LogFormatJSON, &newBuf) and
immediately fail the test if non-nil (e.g., using t.Fatalf or require.NoError)
so any setup failure is surfaced before further assertions.
| c, lerr = net.Listen("tcp", bind) | ||
| if lerr != nil { | ||
| log().WithError(err).Debugf("Retrying to start TCP listener on %s (retry %d/%d)", bind, try, listenerRetries) | ||
| s.options.grpcEventLogger.ModuleLogger("grpc").WithError(err).Debugf("Retrying to start TCP listener on %s (retry %d/%d)", bind, try, listenerRetries) | ||
| try += 1 |
There was a problem hiding this comment.
Log the actual listen error.
Line 118 logs err from the outer scope, which is often nil; it should log lerr from the failed net.Listen call.
🐛 Proposed fix
- s.options.grpcEventLogger.ModuleLogger("grpc").WithError(err).Debugf("Retrying to start TCP listener on %s (retry %d/%d)", bind, try, listenerRetries)
+ s.options.grpcEventLogger.ModuleLogger("grpc").WithError(lerr).Debugf("Retrying to start TCP listener on %s (retry %d/%d)", bind, try, listenerRetries)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| c, lerr = net.Listen("tcp", bind) | |
| if lerr != nil { | |
| log().WithError(err).Debugf("Retrying to start TCP listener on %s (retry %d/%d)", bind, try, listenerRetries) | |
| s.options.grpcEventLogger.ModuleLogger("grpc").WithError(err).Debugf("Retrying to start TCP listener on %s (retry %d/%d)", bind, try, listenerRetries) | |
| try += 1 | |
| c, lerr = net.Listen("tcp", bind) | |
| if lerr != nil { | |
| s.options.grpcEventLogger.ModuleLogger("grpc").WithError(lerr).Debugf("Retrying to start TCP listener on %s (retry %d/%d)", bind, try, listenerRetries) | |
| try += 1 |
🤖 Prompt for AI Agents
In `@principal/listen.go` around lines 116 - 119, The log is attaching the wrong
error variable; in the retry block after c, lerr = net.Listen("tcp", bind)
replace the outer-scope err used in the logger with the actual listen error lerr
by changing the WithError call to use lerr (locate the
ModuleLogger("grpc").WithError(err).Debugf(...) call in the retry loop inside
listen.go and swap err -> lerr).
| // WithLogger sets the logger for the proxy to what is passed in. If this option is not provided | ||
| // the New function will set it to the default logger | ||
| func WithLogger(logger *logging.CentralizedLogger) ResourceProxyOption { | ||
| return func(p *ResourceProxy) error { | ||
| p.logger = logger | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against nil to preserve default logger behavior.
If a caller passes nil, rp.logger can become nil and later logging will panic. Consider defaulting to GetDefaultLogger() when logger is nil.
🛠️ Suggested fix
func WithLogger(logger *logging.CentralizedLogger) ResourceProxyOption {
+ if logger == nil {
+ logger = logging.GetDefaultLogger()
+ }
return func(p *ResourceProxy) error {
p.logger = logger
return nil
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // WithLogger sets the logger for the proxy to what is passed in. If this option is not provided | |
| // the New function will set it to the default logger | |
| func WithLogger(logger *logging.CentralizedLogger) ResourceProxyOption { | |
| return func(p *ResourceProxy) error { | |
| p.logger = logger | |
| return nil | |
| } | |
| } | |
| // WithLogger sets the logger for the proxy to what is passed in. If this option is not provided | |
| // the New function will set it to the default logger | |
| func WithLogger(logger *logging.CentralizedLogger) ResourceProxyOption { | |
| if logger == nil { | |
| logger = logging.GetDefaultLogger() | |
| } | |
| return func(p *ResourceProxy) error { | |
| p.logger = logger | |
| return nil | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@principal/resourceproxy/options.go` around lines 48 - 55, The WithLogger
option currently assigns whatever is passed into p.logger which can set it to
nil and later cause panics; update the WithLogger(ResourceProxyOption) closure
to guard against nil by checking the incoming logger and assigning
logging.GetDefaultLogger() (or the package's default logger accessor) when
logger == nil, then set p.logger accordingly and return nil—this preserves the
ResourceProxy default logger behavior and prevents nil derefs in methods on
ResourceProxy.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #711 +/- ##
==========================================
+ Coverage 46.06% 46.14% +0.07%
==========================================
Files 99 99
Lines 12273 12413 +140
==========================================
+ Hits 5654 5728 +74
- Misses 6068 6130 +62
- Partials 551 555 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…rus logger Signed-off-by: Christopher Coco <ccoco@redhat.com>
… changes Signed-off-by: Christopher Coco <ccoco@redhat.com>
…eady created Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
…x typo i made on docs and in the principal cmd Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
cjcocokrisp
force-pushed
the
feat/toggle-logging-for-subsystems
branch
from
e1e81dd to
906da23
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
principal/redisproxy/redisproxy.go (2)
88-112: Inconsistent logger usage inStart().The
Start()method uses the package-levellog()helper (lines 92, 102, 109) instead of the instance'srp.logger. This defeats the purpose of having a configurable per-instance logger.Proposed fix
func (rp *RedisProxy) Start() error { l, err := net.Listen("tcp", rp.listenAddress) if err != nil { - log().WithError(err).Error("error occurred on listening to addr: " + rp.listenAddress) + rp.logger.ModuleLogger("redisProxy").WithError(err).Error("error occurred on listening to addr: " + rp.listenAddress) return err } rp.listener = l // Start server and connection handler go func() { for { conn, err := l.Accept() if err != nil { - log().WithError(err).Error("error occurred on accepting connection") + rp.logger.ModuleLogger("redisProxy").WithError(err).Error("error occurred on accepting connection") return } go rp.handleConnection(conn) } }() - log().Infof("Redis proxy started on %s", rp.listenAddress) + rp.logger.ModuleLogger("redisProxy").Infof("Redis proxy started on %s", rp.listenAddress) return nil }
708-725: UselogCtxinstead oflog()in error path.Line 715 uses the package-level
log()helper whilelogCtxis available as a parameter. This loses the connection context and bypasses the instance logger.Proposed fix
func (are *argoCDRedisWriterInternal) writeToArgoCDRedisSocket(logCtx *logrus.Entry, bytes []byte) error { are.argoCDWriteLock.Lock() defer are.argoCDWriteLock.Unlock() _, err := are.fromArgoCDWrite.Write(bytes) if err != nil { - log().WithError(err).Error("unable to write to Argo CD Redis socket") + logCtx.WithError(err).Error("unable to write to Argo CD Redis socket") return err }
🤖 Fix all issues with AI agents
In `@internal/logging/logging_test.go`:
- Line 416: Rename the test function TestSeperateLoggerLevels to
TestSeparateLoggerLevels (fixing the "Seperate" → "Separate" typo) and update
any references or calls to that symbol (e.g., in test suites or other files) so
the new name is used consistently; run `go test` to ensure the renamed test is
discovered and passes.
♻️ Duplicate comments (10)
principal/resourceproxy/options.go (1)
48-55: Guard against nil to preserve default logger behavior.If a caller passes
nil,rp.loggerwill be set tonil. AlthoughNew()has a fallback that assignsGetDefaultLogger()whenp.logger == nil, callingWithLogger(nil)on an existingResourceProxyinstance (viaWithRequestMatcherpattern or direct mutation) could still result in a nil logger.🛠️ Suggested fix
func WithLogger(logger *logging.CentralizedLogger) ResourceProxyOption { + if logger == nil { + logger = logging.GetDefaultLogger() + } return func(p *ResourceProxy) error { p.logger = logger return nil } }internal/logging/logging_test.go (1)
391-414: Add error checks forSetupLoggingcalls.The
SetupLoggingcalls at lines 395 and 398 ignore the returned error, which could cause silent failures and misleading test assertions.🧪 Proposed fix
- defaultLogger.SetupLogging(LogLevelInfo, LogFormatJSON, &defaultBuf) + require.NoError(t, defaultLogger.SetupLogging(LogLevelInfo, LogFormatJSON, &defaultBuf)) newLogger := New(nil) - newLogger.SetupLogging(LogLevelDebug, LogFormatJSON, &newBuf) + require.NoError(t, newLogger.SetupLogging(LogLevelDebug, LogFormatJSON, &newBuf))docs/configuration/reference/principal.md (1)
402-408: Documentation updates look good overall.The new format documentation correctly describes the comma-separated log level syntax with optional component qualifiers. The component list (
resource-proxy,redis-proxy,grpc-event) aligns with theAvailableSubSystemsconstant incmd/cmdutil/log.go.However, the valid log levels list is still missing
fatal, whichStringToLoglevelaccepts. This was flagged in a previous review.cmd/cmdutil/log.go (1)
102-129: MissingSetOutput(io.Discard)will cause duplicate log output.The
CreateLoggerfunction adds writer hooks but doesn't discard the default output. Since logrus writes to both the default output and hooks, logs will be duplicated (once to stderr/stdout via the default writer, and again via the hooks).Proposed fix
func CreateLogger(logFormat string) *logrus.Logger { logger := logrus.New() + logger.SetOutput(io.Discard) // Route all logs through hooks only if formatter, err := LogFormatter(logFormat); err != nil { Fatal("%s", err.Error()) } else {principal/redisproxy/redisproxy.go (1)
245-245: Fix typo in comment.The comment contains a corrupted word:
falsGetDefaultLogger().eshould befalse. This was flagged in a previous review.Proposed fix
- forwardRawCommandToPrincipalRedis := true // whether to send the command to principal redis: falsGetDefaultLogger().e if the command has already been processed, true otherwise. + forwardRawCommandToPrincipalRedis := true // whether to send the command to principal redis: false if the command has already been processed, true otherwise.principal/listen.go (1)
116-121: Logging the wrong error variable.Line 118 logs
errfrom the outer scope (which is often nil at this point) instead oflerrfrom the failednet.Listencall. This was flagged in a previous review.Proposed fix
c, lerr = net.Listen("tcp", bind) if lerr != nil { - s.options.grpcEventLogger.ModuleLogger("grpc").WithError(err).Debugf("Retrying to start TCP listener on %s (retry %d/%d)", bind, try, listenerRetries) + s.options.grpcEventLogger.ModuleLogger("grpc").WithError(lerr).Debugf("Retrying to start TCP listener on %s (retry %d/%d)", bind, try, listenerRetries) try += 1 return false, lerr }internal/logging/README.md (1)
130-136: ErrorLogger example still uses the old package-level API.Line 134 calls
logging.ErrorLogger(...)directly instead oflogging.GetDefaultLogger().ErrorLogger(...). This example won't compile with the new instance-based API.Proposed fix
#### Error Handling ```go err := errors.New("connection failed") -logger := logging.ErrorLogger("ModuleName", "ConnectionError", err) +logger := logging.GetDefaultLogger().ErrorLogger("ModuleName", "ConnectionError", err) logger.Error("Error occurred")</details> </blockquote></details> <details> <summary>internal/logging/logging.go (1)</summary><blockquote> `233-238`: **Guard against nil error to avoid panic in ErrorLogger.** `err.Error()` will panic when `err` is nil. Logging should be safe even with a nil error. <details> <summary>🐛 Proposed fix</summary> ```diff func (l *CentralizedLogger) ErrorLogger(module, errorType string, err error) *logrus.Entry { - return l.logger.WithFields(logrus.Fields{ - logfields.Module: module, - logfields.ErrorType: errorType, - logfields.Error: err.Error(), - }) + fields := logrus.Fields{ + logfields.Module: module, + logfields.ErrorType: errorType, + } + if err != nil { + fields[logfields.Error] = err.Error() + } + return l.logger.WithFields(fields) }cmd/argocd-agent/agent.go (1)
257-259: Clarify--log-levelhelp text for new syntax.
The flag now supports comma-separated and subsystem-qualified values, but the description still reads like a single global level.✏️ Suggested tweak
- "The log level for the agent") + "Log levels (comma-separated; optional subsystem=level, e.g. \"info,redis-proxy=debug\")")cmd/argocd-agent/principal.go (1)
344-346: Clarify--log-levelhelp text for new syntax.
The flag now accepts comma-separated and subsystem-qualified values, but the description still reads like a single global level.✏️ Suggested tweak
- "The log level to use") + "Log levels (comma-separated; optional subsystem=level, e.g. \"info,redis-proxy=debug\")")
🧹 Nitpick comments (4)
principal/resourceproxy/resourceproxy.go (1)
121-121: Inconsistent logger usage betweenlog()helper and instance logger.The
log()helper (used at line 121 for HTTP error logging) returnsGetDefaultLogger().ModuleLogger("proxy"), while the rest of the file usesrp.logger.ModuleLogger("proxy"). This means HTTP server error logs will always use the default logger's level, ignoring any custom logger injected viaWithLogger.Consider using the instance logger consistently:
♻️ Suggested refactor
httpLogger := log().WriterLevel(logrus.ErrorLevel) + // Note: Using log() here means HTTP server errors use the default logger, + // not the instance's configured logger. Consider: + // httpLogger := rp.logger.ModuleLogger("proxy").WriterLevel(logrus.ErrorLevel)Alternatively, if retaining
log()for backward compatibility:-func log() *logrus.Entry { - return logging.GetDefaultLogger().ModuleLogger("proxy") -} +// log is retained for HTTP server error logging using the default logger. +// Instance-specific logging should use rp.logger.ModuleLogger("proxy"). +func log() *logrus.Entry { + return logging.GetDefaultLogger().ModuleLogger("proxy") +}Also applies to: 236-238
cmd/cmdutil/log.go (3)
133-141: Use a map forseenand remove impossible condition.
seenis used for membership checks, butslices.Containsis O(n). With a map, lookups are O(1).len(split) == 0is impossible—strings.Splitalways returns at least one element (an empty string ifeis empty).Proposed fix
func ParseLogLevels(input []string, ss *SubSystemLoggers) { - seen := []string{} + seen := make(map[string]bool) for _, e := range input { split := strings.Split(e, "=") - if len(split) > 2 || len(split) == 0 { + if len(split) > 2 { logrus.Warnf("%s is invalid please use the format subsystem=loglevel, skipping", e) continue }
154-163: Updateseenchecks to use map syntax.Following the refactor to use a map for
seen, update the membership checks and assignments accordingly.Proposed fix
- if !slices.Contains(seen, "resource-proxy") { + if !seen["resource-proxy"] { ss.ResourceProxyLogger.SetLevel(level) } - if !slices.Contains(seen, "redis-proxy") { + if !seen["redis-proxy"] { ss.RedisProxyLogger.SetLevel(level) } - if !slices.Contains(seen, "grpc-event") { + if !seen["grpc-event"] { ss.GrpcEventLogger.SetLevel(level) }And for the switch cases (lines 175-183):
case "resource-proxy": ss.ResourceProxyLogger.SetLevel(level) - seen = append(seen, "resource-proxy") + seen["resource-proxy"] = true case "redis-proxy": ss.RedisProxyLogger.SetLevel(level) - seen = append(seen, "redis-proxy") + seen["redis-proxy"] = true case "grpc-event": ss.GrpcEventLogger.SetLevel(level) - seen = append(seen, "grpc-event") + seen["grpc-event"] = true
21-21: Remove unused import after refactor.If the
seenslice is converted to a map, theslicesimport will become unused and should be removed.
cjcocokrisp
force-pushed
the
feat/toggle-logging-for-subsystems
branch
from
8def3fd to
7450284
Compare
|
Changed how the subsystem loggers are interacted with to use the |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
principal/logging.go (1)
62-68: Fix log message to match stream interceptor.The stream interceptor still logs “unary,” which is misleading.
💡 Proposed fix
- }).Debug("Processing unary gRPC request") + }).Debug("Processing stream gRPC request")
🤖 Fix all issues with AI agents
In `@principal/redisproxy/redisproxy.go`:
- Around line 129-131: The code currently calls rp.logger.ModuleLogger(...)
which can panic when rp.logger is nil; replace direct rp.logger access with
rp.log() so the call becomes
rp.log().ModuleLogger("redisProxy").WithField("function", "redisFxn") and then
.WithField("connUUID", connUUID) (update the variable logCtx assignment). Search
for other direct rp.logger.ModuleLogger usages in the redisFxn scope and swap
them to rp.log() to ensure safe fallback to the default logger.
♻️ Duplicate comments (4)
internal/logging/logging.go (1)
233-238: Guard against nil error to avoid panic.
err.Error()will panic iferris nil. Logging should be safe when callers pass nil.🐛 Proposed fix
func (l *CentralizedLogger) ErrorLogger(module, errorType string, err error) *logrus.Entry { - return l.logger.WithFields(logrus.Fields{ - logfields.Module: module, - logfields.ErrorType: errorType, - logfields.Error: err.Error(), - }) + fields := logrus.Fields{ + logfields.Module: module, + logfields.ErrorType: errorType, + } + if err != nil { + fields[logfields.Error] = err.Error() + } + return l.logger.WithFields(fields) }principal/redisproxy/redisproxy.go (1)
245-245: Fix typo in comment.The inline comment has a corrupted word.
📝 Proposed fix
- forwardRawCommandToPrincipalRedis := true // whether to send the command to principal redis: falsGetDefaultLogger().e if the command has already been processed, true otherwise. + forwardRawCommandToPrincipalRedis := true // whether to send the command to principal redis: false if the command has already been processed, true otherwise.principal/listen.go (1)
116-119: Use the actual listen error in retry log.The retry log uses
errfrom the outer scope, which is often nil; it should loglerrfromnet.Listen.🐛 Proposed fix
- s.logGrpcEvent().WithError(err).Debugf("Retrying to start TCP listener on %s (retry %d/%d)", bind, try, listenerRetries) + s.logGrpcEvent().WithError(lerr).Debugf("Retrying to start TCP listener on %s (retry %d/%d)", bind, try, listenerRetries)internal/logging/logging_test.go (1)
391-399: Add error checks for SetupLogging in this test.Setup failures would be silent and could produce misleading assertions.
🧪 Proposed fix
- defaultLogger.SetupLogging(LogLevelInfo, LogFormatJSON, &defaultBuf) + require.NoError(t, defaultLogger.SetupLogging(LogLevelInfo, LogFormatJSON, &defaultBuf)) newLogger := New(nil) - newLogger.SetupLogging(LogLevelDebug, LogFormatJSON, &newBuf) + require.NoError(t, newLogger.SetupLogging(LogLevelDebug, LogFormatJSON, &newBuf))
🧹 Nitpick comments (1)
principal/resourceproxy/resourceproxy_test.go (1)
47-47: Consider using the resource-proxy logger for consistency.♻️ Suggested tweak
- logging.GetDefaultLogger().ModuleLogger("RedisProxy").Tracef("Writing OK") + p.log().Tracef("Writing OK")
Signed-off-by: Christopher Coco <ccoco@redhat.com>
cjcocokrisp
force-pushed
the
feat/toggle-logging-for-subsystems
branch
from
7450284 to
911fb16
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
principal/logging.go (1)
62-68: Fix log message for stream interceptor.
It currently says “unary” in the stream path.📝 Proposed fix
- }).Debug("Processing unary gRPC request") + }).Debug("Processing stream gRPC request")
♻️ Duplicate comments (4)
principal/listen.go (1)
117-120: Fix: Log the actual listen error.The log statement uses
err(outer scope, likely nil at this point) instead oflerr(the actual error fromnet.Listen). This was flagged in a previous review but remains unfixed.🐛 Proposed fix
- s.logGrpcEvent().WithError(err).Debugf("Retrying to start TCP listener on %s (retry %d/%d)", bind, try, listenerRetries) + s.logGrpcEvent().WithError(lerr).Debugf("Retrying to start TCP listener on %s (retry %d/%d)", bind, try, listenerRetries)principal/redisproxy/redisproxy.go (1)
245-245: Fix typo in inline comment.
Minor but confusing doc glitch.📝 Proposed fix
- forwardRawCommandToPrincipalRedis := true // whether to send the command to principal redis: falsGetDefaultLogger().e if the command has already been processed, true otherwise. + forwardRawCommandToPrincipalRedis := true // whether to send the command to principal redis: false if the command has already been processed, true otherwise.internal/logging/logging.go (1)
233-238: Guard against nilerrto avoid panic.
err.Error()will panic whenerris nil.🐛 Proposed fix
func (l *CentralizedLogger) ErrorLogger(module, errorType string, err error) *logrus.Entry { - return l.logger.WithFields(logrus.Fields{ - logfields.Module: module, - logfields.ErrorType: errorType, - logfields.Error: err.Error(), - }) + fields := logrus.Fields{ + logfields.Module: module, + logfields.ErrorType: errorType, + } + if err != nil { + fields[logfields.Error] = err.Error() + } + return l.logger.WithFields(fields) }internal/logging/logging_test.go (1)
395-399: Add error checks for SetupLogging in this test.
Silent failures can mask setup issues.🧪 Proposed fix
- defaultLogger.SetupLogging(LogLevelInfo, LogFormatJSON, &defaultBuf) + require.NoError(t, defaultLogger.SetupLogging(LogLevelInfo, LogFormatJSON, &defaultBuf)) newLogger := New(nil) - newLogger.SetupLogging(LogLevelDebug, LogFormatJSON, &newBuf) + require.NoError(t, newLogger.SetupLogging(LogLevelDebug, LogFormatJSON, &newBuf))
🧹 Nitpick comments (1)
principal/server.go (1)
216-227: Optional: Redundant nil-to-default logger assignments.The nil checks for
resourceProxyLoggerandredisProxyLoggerare redundant because bothresourceproxy.New()(line 110-112) andredisproxy.New()(line 71-73) already handle nil loggers by defaulting toGetDefaultLogger(). Similarly,logGrpcEvent()usesSelectLogger()which handles nil.This defensive initialization is harmless but creates duplication. Consider removing these checks to keep the default-logger logic in one place (the constructors/accessors).
3 participants
What does this PR do / why we need it:
This PR addresses #459. This issue states that some subsystems are a bit too noisy and it would be nice to have configurable log levels for the subsystems. I accomplished this by adjusting the centralized logging class to now have a struct called
CentralizedLoggerwhich has the same functionality as before just with a replaceablelogrus.Logger.I made adjustments to the areas of code that used the default one and then for the subsystems I replaced the logCtx assignments to be below.
Before:
After:
I do have one issue with this however that I would like some feedback on. To me it seems long and also the name of the module is hard coded (this can easily be fixed by having a package level constant for the name of that or it could go in the log fields not sure which would be better gut is saying the package level constant). The object calling like so also seems a bit more to me. If there are any suggestions on a better way to accomplish this it would be much appreciated.
I updated the logging docs to reflect the changes I made and updated the Kubernetes manifests to include the new env variables. Let me know if anything is missing or should be done differently.
(Also my lsp was doing some formatting fixes removing some blank lines, adding separation lines in markdown files, so I apologize for a lot of the lines being that)
Which issue(s) this PR fixes:
Fixes #459
How to test changes / Special notes to the reviewer:
See new unit tests added for the
CentralizedLogger.Run the agent with these changes
Checklist
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.