RHCLOUD-44973: Replace slog with zerolog and remove printf statements#66
Open
akoserwal wants to merge 13 commits intoproject-kessel:mainfrom
Open
RHCLOUD-44973: Replace slog with zerolog and remove printf statements#66akoserwal wants to merge 13 commits intoproject-kessel:mainfrom
akoserwal wants to merge 13 commits intoproject-kessel:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a zerolog-backed observer layer and wires new observer interfaces through the config loader, provider/registry construction, caching, key rotation, JWKS, trust validation, and server lifecycle; supplies concrete zerolog probe implementations and replaces direct prints/logging with observer callbacks and per-event loggers. Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as Config Loader
participant Provider as Provider / Registry Builder
participant Keys as Key Rotator / Provider
participant DataSrc as DataSource Cache
participant JWKS as JWKS Server
participant Server as Server (gRPC/HTTP)
rect rgba(200,200,255,0.5)
Loader->>Provider: Build registries (pass observers)
Provider->>Keys: Construct signers/providers (attach KeyRotation/Provider observers)
Provider->>DataSrc: Construct caching datasource (attach cache observer)
end
rect rgba(200,255,200,0.5)
Server->>JWKS: Start JWKS (observer wired)
JWKS-->>Server: Initial cache failure -> JWKSObserver.InitialCachePopulationFailed
Keys-->>Provider: Rotation events -> KeyRotationObserver callbacks
DataSrc-->>Provider: Cache events -> DataSourceCacheObserver callbacks
Server-->>Provider: Serve error -> ServerLifecycleObserver.GRPCServeFailed / HTTPServeFailed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/server/jwks_test.go (1)
3-12:⚠️ Potential issue | 🟠 MajorFix import formatting with
goimports.Line 8 breaks goimports ordering, and lint is failing on this file.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/jwks_test.go` around lines 3 - 12, The import block in jwks_test.go is mis-ordered (line 8) and violates goimports conventions; run goimports (or manually reorder) to group standard library imports first (context, crypto/..., testing), leave a blank line, then third-party imports (github.com/rs/zerolog and github.com/project-kessel/parsec/internal/service), ensuring imports used by tests remain present and formatted so the file passes goimports/lint.internal/server/jwks_cache_test.go (1)
3-14:⚠️ Potential issue | 🟠 MajorResolve goimports failure in imports.
Line 8 is not goimports-formatted, and this is also reported by the pipeline (
golangci-lint).As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/jwks_cache_test.go` around lines 3 - 14, The import block is not goimports-formatted; reorder and format imports to group standard library packages (context, crypto/..., testing, time), third-party packages (github.com/rs/zerolog) and internal packages (github.com/project-kessel/parsec/internal/clock, github.com/project-kessel/parsec/internal/service), remove any unused imports, and apply goimports/gofmt (e.g., run goimports -w or gofmt -s -w) so the file including symbols like zerolog, clock and service compiles cleanly under golangci-lint.internal/server/server_test_helper_test.go (1)
3-21:⚠️ Potential issue | 🟠 MajorRun
goimportson this file to unblock lint.Line 7 import ordering/grouping is not goimports-compliant, and lint is currently failing for this file.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/server_test_helper_test.go` around lines 3 - 21, The import block in server_test_helper_test.go is not goimports-compliant (third-party and stdlib are mixed); reorder/group the imports into standard library first (context, encoding/json, io, net, net/http, testing, time), then a blank line, then external modules (github.com/rs/zerolog, google.golang.org/... packages), then a blank line, then local project imports (github.com/project-kessel/parsec/...), or simply run the goimports tool on server_test_helper_test.go to automatically fix ordering and grouping so lint passes.internal/server/jwks_server_test.go (1)
3-16:⚠️ Potential issue | 🟠 MajorApply
goimportsto this test file.Line 7 import grouping is out of goimports format, which is failing lint.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/jwks_server_test.go` around lines 3 - 16, The import block in jwks_server_test.go is not formatted per goimports (standard libs grouped together, blank line, then external and local packages); run goimports on the file (or manually reorder the import block so context, encoding/json, io, net/http, testing, time are grouped as standard library imports, leave github.com/rs/zerolog and the github.com/project-kessel/... packages after a blank line) to fix the lint failure and ensure correct grouping/order.
🧹 Nitpick comments (1)
internal/config/observability.go (1)
102-114: Consider removing the no-op hook.The
eventFilteringHookis attached to the logger but itsRun()method performs no filtering, as explained in the comment. Attaching an unused hook introduces minor overhead on every log call without providing any benefit.Since per-event filtering is handled in
probe/logging.goviaEventLevel(), consider removing the hook attachment at lines 53-57 until a functional implementation is needed.Proposed fix
baseLogger := zerolog.New(writer).With().Timestamp().Logger().Level(defaultLevel) - eventLevels := buildEventLevels(cfg) - if len(eventLevels) > 0 { - baseLogger = baseLogger.Hook(&eventFilteringHook{ - eventLevels: eventLevels, - }) - } - return baseLogger }The
buildEventLevelsfunction andeventFilteringHooktype can also be removed if the hook is not used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/observability.go` around lines 102 - 114, The eventFilteringHook type and its no-op Run method (eventFilteringHook, Run) add unnecessary overhead since per-event filtering is already implemented via EventLevel in probe/logging.go; remove the hook attachment and delete the eventFilteringHook type and its helper buildEventLevels function (if present) from internal/config/observability.go, and also remove any code that constructs or attaches this hook to the logger so there is no unused hook instantiation left behind.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/keys/dual_slot_signer.go`:
- Around line 83-85: The struct field comment for Logger claims it defaults to
zerolog.Nop() when zero-valued, but the constructor (NewDualSlotSigner / the
function that assigns cfg.Logger) currently assigns cfg.Logger directly and
doesn't implement that defaulting; either update the constructor to perform the
defaulting (check if cfg.Logger equals the zero-value and set Logger =
zerolog.Nop()) before assigning to the struct, or change the comment on the
Logger field to remove the claim about defaulting so documentation matches the
existing behavior; reference the Logger field and the constructor function that
assigns cfg.Logger to implement the fix.
---
Outside diff comments:
In `@internal/server/jwks_cache_test.go`:
- Around line 3-14: The import block is not goimports-formatted; reorder and
format imports to group standard library packages (context, crypto/..., testing,
time), third-party packages (github.com/rs/zerolog) and internal packages
(github.com/project-kessel/parsec/internal/clock,
github.com/project-kessel/parsec/internal/service), remove any unused imports,
and apply goimports/gofmt (e.g., run goimports -w or gofmt -s -w) so the file
including symbols like zerolog, clock and service compiles cleanly under
golangci-lint.
In `@internal/server/jwks_server_test.go`:
- Around line 3-16: The import block in jwks_server_test.go is not formatted per
goimports (standard libs grouped together, blank line, then external and local
packages); run goimports on the file (or manually reorder the import block so
context, encoding/json, io, net/http, testing, time are grouped as standard
library imports, leave github.com/rs/zerolog and the
github.com/project-kessel/... packages after a blank line) to fix the lint
failure and ensure correct grouping/order.
In `@internal/server/jwks_test.go`:
- Around line 3-12: The import block in jwks_test.go is mis-ordered (line 8) and
violates goimports conventions; run goimports (or manually reorder) to group
standard library imports first (context, crypto/..., testing), leave a blank
line, then third-party imports (github.com/rs/zerolog and
github.com/project-kessel/parsec/internal/service), ensuring imports used by
tests remain present and formatted so the file passes goimports/lint.
In `@internal/server/server_test_helper_test.go`:
- Around line 3-21: The import block in server_test_helper_test.go is not
goimports-compliant (third-party and stdlib are mixed); reorder/group the
imports into standard library first (context, encoding/json, io, net, net/http,
testing, time), then a blank line, then external modules (github.com/rs/zerolog,
google.golang.org/... packages), then a blank line, then local project imports
(github.com/project-kessel/parsec/...), or simply run the goimports tool on
server_test_helper_test.go to automatically fix ordering and grouping so lint
passes.
---
Nitpick comments:
In `@internal/config/observability.go`:
- Around line 102-114: The eventFilteringHook type and its no-op Run method
(eventFilteringHook, Run) add unnecessary overhead since per-event filtering is
already implemented via EventLevel in probe/logging.go; remove the hook
attachment and delete the eventFilteringHook type and its helper
buildEventLevels function (if present) from internal/config/observability.go,
and also remove any code that constructs or attaches this hook to the logger so
there is no unused hook instantiation left behind.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
go.modinternal/cli/serve.gointernal/config/loader.gointernal/config/observability.gointernal/keys/awskms.gointernal/keys/dual_slot_signer.gointernal/probe/logging.gointernal/server/jwks.gointernal/server/jwks_cache_test.gointernal/server/jwks_server_test.gointernal/server/jwks_test.gointernal/server/server.gointernal/server/server_test_helper_test.go
akoserwal
changed the title
| Str("http_addr", httpAddr). | ||
| Str("trust_domain", provider.TrustDomain()). | ||
| Str("config", configPath). | ||
| Msg("parsec is running") |
Contributor
There was a problem hiding this comment.
check for some of missing print statements --> logger statements
akoserwal
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/config/observability.go (1)
23-39:⚠️ Potential issue | 🟠 Major
NewObserverWithLoggerdrops the caller logger for composite observers.The composite branch ignores the
loggerargument, so sub-observers won’t actually share the provided logger despite the API contract.Proposed fix
func NewObserverWithLogger(cfg *ObservabilityConfig, logger zerolog.Logger) (service.ApplicationObserver, error) { @@ case "composite": - return newCompositeObserver(cfg) + return newCompositeObserverWithLogger(cfg, logger) @@ -func newCompositeObserver(cfg *ObservabilityConfig) (service.ApplicationObserver, error) { +func newCompositeObserverWithLogger(cfg *ObservabilityConfig, logger zerolog.Logger) (service.ApplicationObserver, error) { if len(cfg.Observers) == 0 { return nil, fmt.Errorf("composite observer requires at least one sub-observer") } @@ - observer, err := NewObserver(&subCfg) + observer, err := NewObserverWithLogger(&subCfg, logger) if err != nil { return nil, fmt.Errorf("failed to create observer %d: %w", i, err) }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 55-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/observability.go` around lines 23 - 39, NewObserverWithLogger currently ignores the provided logger when creating composite observers (call sites: NewObserverWithLogger, newCompositeObserver), so update the composite branch to propagate the caller's zerolog.Logger into the composite's sub-observers: change newCompositeObserver to accept the logger (e.g., newCompositeObserver(cfg, logger)) or add a new constructor (e.g., newCompositeObserverWithLogger) that uses the logger when constructing each sub-observer (the same way probe.NewLoggingObserverWithConfig uses Logger and buildEventLevels), and ensure any code that builds sub-observers inside the composite uses the passed logger rather than creating or defaulting a different logger.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/probe/logging.go`:
- Around line 44-46: The current code in logging.go re-enables a disabled logger
by checking logger.GetLevel() == zerolog.Disabled and replacing it with
zerolog.New(os.Stdout)...; remove that fallback so the function respects the
caller's disabled logger choice—delete the conditional branch that calls
zerolog.New(os.Stdout).With().Timestamp().Logger() (and any related assignments
to logger) so that when logger.GetLevel() == zerolog.Disabled the original
logger is left untouched; optionally add a short comment near the logger
variable to document that a disabled logger is intentionally preserved.
In `@internal/server/server.go`:
- Around line 151-153: The gRPC Serve error is always being logged
(s.grpcServer.Serve), including the expected shutdown sentinel
grpc.ErrServerStopped; update the handler to mirror the HTTP logic by filtering
that sentinel before logging — i.e., if err != nil and !errors.Is(err,
grpc.ErrServerStopped) (or err != grpc.ErrServerStopped) then call
s.logger.Error().Err(err).Msg("gRPC server error"); ensure you import errors if
using errors.Is and reference s.grpcServer.Serve, grpc.ErrServerStopped, and the
existing s.logger.Error().Err(...).Msg call when making the change.
---
Outside diff comments:
In `@internal/config/observability.go`:
- Around line 23-39: NewObserverWithLogger currently ignores the provided logger
when creating composite observers (call sites: NewObserverWithLogger,
newCompositeObserver), so update the composite branch to propagate the caller's
zerolog.Logger into the composite's sub-observers: change newCompositeObserver
to accept the logger (e.g., newCompositeObserver(cfg, logger)) or add a new
constructor (e.g., newCompositeObserverWithLogger) that uses the logger when
constructing each sub-observer (the same way probe.NewLoggingObserverWithConfig
uses Logger and buildEventLevels), and ensure any code that builds sub-observers
inside the composite uses the passed logger rather than creating or defaulting a
different logger.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
internal/config/observability.gointernal/keys/dual_slot_signer.gointernal/probe/logging.gointernal/server/server.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/observability.go`:
- Around line 76-88: eventLogger currently ignores per-event log format; update
eventLogger to check eventCfg.LogFormat (on EventLoggingConfig) and, when set,
adjust the returned zerolog.Logger's output/format accordingly (e.g., use
zerolog.ConsoleWriter for "console"/human format or set JSON output for "json"),
while preserving existing Enabled and LogLevel handling and falling back to the
base logger format when LogFormat is empty; reference the eventLogger function,
the eventCfg.LogFormat field, and existing parseLogLevel/Logger construction to
locate where to apply the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c210629-df5a-4341-bc42-8aac84042594
📒 Files selected for processing (2)
internal/config/observability.gointernal/probe/logging.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/config/observability.go (1)
23-40:⚠️ Potential issue | 🟠 MajorPropagate the injected logger in the
compositepath.
NewObserverWithLoggeraccepts a shared logger, but"composite"delegates tonewCompositeObserver(cfg), which builds sub-observers viaNewObserver(&subCfg)and drops the injected logger. This breaks logger consistency across composed observers.Proposed fix
func NewObserverWithLogger(cfg *ObservabilityConfig, logger zerolog.Logger) (service.ApplicationObserver, error) { @@ case "composite": - return newCompositeObserver(cfg) + return newCompositeObserverWithLogger(cfg, logger) @@ } +func newCompositeObserverWithLogger(cfg *ObservabilityConfig, logger zerolog.Logger) (service.ApplicationObserver, error) { + if len(cfg.Observers) == 0 { + return nil, fmt.Errorf("composite observer requires at least one sub-observer") + } + + var observers []service.ApplicationObserver + for i, subCfg := range cfg.Observers { + observer, err := NewObserverWithLogger(&subCfg, logger) + if err != nil { + return nil, fmt.Errorf("failed to create observer %d: %w", i, err) + } + observers = append(observers, observer) + } + + return service.NewCompositeObserver(observers...), nil +}As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/observability.go` around lines 23 - 40, The composite branch of NewObserverWithLogger currently calls newCompositeObserver(cfg) and loses the injected zerolog.Logger; update the composite path to propagate the provided logger into composite creation (e.g., change newCompositeObserver to accept the logger or call NewObserverWithLogger for each sub-config) so all sub-observers are built with the same injected logger; modify newCompositeObserver signature and its internal creation of sub-observers (or replace NewObserver calls with NewObserverWithLogger) to use the provided zerolog.Logger, and update any callers accordingly (symbols to change: NewObserverWithLogger, newCompositeObserver, and the points where NewObserver is used inside the composite builder).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/config/observability.go`:
- Around line 23-40: The composite branch of NewObserverWithLogger currently
calls newCompositeObserver(cfg) and loses the injected zerolog.Logger; update
the composite path to propagate the provided logger into composite creation
(e.g., change newCompositeObserver to accept the logger or call
NewObserverWithLogger for each sub-config) so all sub-observers are built with
the same injected logger; modify newCompositeObserver signature and its internal
creation of sub-observers (or replace NewObserver calls with
NewObserverWithLogger) to use the provided zerolog.Logger, and update any
callers accordingly (symbols to change: NewObserverWithLogger,
newCompositeObserver, and the points where NewObserver is used inside the
composite builder).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0188c66-47a3-4f6a-86d9-244c72bb71ea
📒 Files selected for processing (2)
internal/config/observability.gointernal/server/server.go
akoserwal
force-pushed
the
RHCLOUD-44973-improve-logging
branch
from
335ada1 to
60568dd
Compare
akoserwal
changed the title
Collaborator
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
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)
internal/keys/awskms.go (1)
110-116:⚠️ Potential issue | 🟠 MajorError handling bug: all errors from
getKeyIDFromAliasare silently ignored.Since
getKeyIDFromAliasnow returns("", err)on any AWS error (line 156), the condition at line 112 (err != nil && oldKeyID == "") will always be true for errors, causing theelse ifbranch (line 114) to never execute. This means authentication failures, network errors, or rate limits are treated as "alias doesn't exist," potentially creating duplicate aliases or proceeding with rotation incorrectly.Proposed fix
Check for "not found" errors specifically instead of all errors:
// 2. Get current alias to find old key (if exists) oldKeyID, err := m.getKeyIDFromAlias(ctx, aliasName) - if err != nil && oldKeyID == "" { - // Alias doesn't exist, that's fine - } else if err != nil { - return fmt.Errorf("failed to check existing alias: %w", err) + if err != nil { + // Check if it's a "not found" error - alias doesn't exist yet + var notFoundErr *types.NotFoundException + if !errors.As(err, ¬FoundErr) { + return fmt.Errorf("failed to check existing alias: %w", err) + } + // Alias doesn't exist, that's fine - oldKeyID stays empty + oldKeyID = "" }You'll need to add
"errors"to the imports.internal/probe/logging.go (1)
51-78:⚠️ Potential issue | 🟠 MajorKeep the request-scoped fields on the probe logger.
Each
*Startedmethod adds request context only to the initial"Starting ..."entry, then returns a probe built from the bare event logger. All laterSucceeded/Failed/Endlogs therefore lose the subject/actor/scope/request fields for that same flow, which makes failures much harder to correlate.Build a child logger once in each starter and pass that child into the probe.
Suggested direction
func (o *loggingObserver) TokenIssuanceStarted( ctx context.Context, subject *trust.Result, actor *trust.Result, scope string, tokenTypes []service.TokenType, ) (context.Context, service.TokenIssuanceProbe) { - event := o.tokenIssuanceLogger.Debug(). - Str("scope", scope). - Interface("token_types", tokenTypes) + logger := o.tokenIssuanceLogger.With(). + Str("scope", scope). + Interface("token_types", tokenTypes). + Logger() if subject != nil { - event = event. - Str("subject_id", subject.Subject). - Str("subject_trust_domain", subject.TrustDomain) + logger = logger.With(). + Str("subject_id", subject.Subject). + Str("subject_trust_domain", subject.TrustDomain). + Logger() } if actor != nil { - event = event. - Str("actor_id", actor.Subject). - Str("actor_trust_domain", actor.TrustDomain) + logger = logger.With(). + Str("actor_id", actor.Subject). + Str("actor_trust_domain", actor.TrustDomain). + Logger() } - event.Msg("Starting token issuance") + logger.Debug().Msg("Starting token issuance") return ctx, &loggingTokenIssuanceProbe{ - logger: o.tokenIssuanceLogger, + logger: logger, } }Apply the same pattern to
TokenExchangeStartedandAuthzCheckStarted.Also applies to: 125-141, 197-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/probe/logging.go` around lines 51 - 78, The starter methods on loggingObserver (e.g., TokenIssuanceStarted, TokenExchangeStarted, AuthzCheckStarted) add request-scoped fields only to the one "Starting ..." event but return a probe that uses the base logger, causing subsequent probe logs (loggingTokenIssuanceProbe, loggingTokenExchangeProbe, loggingAuthzCheckProbe) to lose those fields; fix by creating a child logger with the request fields (using o.tokenIssuanceLogger.ForContext / With / child logger API used in this package) inside each *Started method, use that child logger for the initial Msg and pass it into the returned probe struct (set logger: childLogger) so Succeeded/Failed/End methods retain the subject/actor/scope/request fields.
🧹 Nitpick comments (1)
internal/cli/serve.go (1)
205-214: Startup logging is well-structured; URL scheme is hardcoded.The structured logging with multiple fields is good practice. Note that the URLs use hardcoded
http://scheme—if TLS support is added later, this will need updating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/serve.go` around lines 205 - 214, The URL scheme is hardcoded as "http://" in the logger.Info() block; update the code to derive a scheme string from the server/config TLS state (e.g., tlsEnabled or ServerConfig.TLS) and use scheme + "://"+httpAddr when building token_exchange_url, jwks_url, and jwks_wellknown_url so the logs reflect http vs https correctly; change the logger.Info() usage that references grpcAddr, httpAddr, provider.TrustDomain(), and configPath to use the computed scheme variable for those URL fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/observability.go`:
- Around line 77-84: Event-level log_format currently forces createWriter to use
os.Stdout, overriding the caller's logger sink; change the implementation so
per-event formatting preserves the original writer instead of hardcoding stdout:
update createWriter to accept a fallback io.Writer (or explicitly pass the
caller-provided writer) and use that as the Out target for console/json writers
rather than os.Stdout, then modify EventLogger to pass the existing logger sink
(or accept an io.Writer parameter) into createWriter; ensure
NewObserverWithLogger threads the original writer through to EventLogger so
per-event formatting only changes format, not destination.
---
Outside diff comments:
In `@internal/probe/logging.go`:
- Around line 51-78: The starter methods on loggingObserver (e.g.,
TokenIssuanceStarted, TokenExchangeStarted, AuthzCheckStarted) add
request-scoped fields only to the one "Starting ..." event but return a probe
that uses the base logger, causing subsequent probe logs
(loggingTokenIssuanceProbe, loggingTokenExchangeProbe, loggingAuthzCheckProbe)
to lose those fields; fix by creating a child logger with the request fields
(using o.tokenIssuanceLogger.ForContext / With / child logger API used in this
package) inside each *Started method, use that child logger for the initial Msg
and pass it into the returned probe struct (set logger: childLogger) so
Succeeded/Failed/End methods retain the subject/actor/scope/request fields.
---
Nitpick comments:
In `@internal/cli/serve.go`:
- Around line 205-214: The URL scheme is hardcoded as "http://" in the
logger.Info() block; update the code to derive a scheme string from the
server/config TLS state (e.g., tlsEnabled or ServerConfig.TLS) and use scheme +
"://"+httpAddr when building token_exchange_url, jwks_url, and
jwks_wellknown_url so the logs reflect http vs https correctly; change the
logger.Info() usage that references grpcAddr, httpAddr, provider.TrustDomain(),
and configPath to use the computed scheme variable for those URL fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 785896de-df0f-4329-8d8c-e7415dc7fa6b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
go.modinternal/cli/serve.gointernal/config/config.gointernal/config/datasources.gointernal/config/issuers.gointernal/config/loader.gointernal/config/observability.gointernal/config/provider.gointernal/config/reload_observer.gointernal/config/validators.gointernal/datasource/in_memory_caching_datasource.gointernal/datasource/observer.gointernal/keys/awskms.gointernal/keys/dual_slot_signer.gointernal/keys/observer.gointernal/probe/logging.gointernal/probe/logging_config.gointernal/probe/logging_datasource.gointernal/probe/logging_jwks.gointernal/probe/logging_keys.gointernal/probe/logging_server.gointernal/probe/logging_trust.gointernal/server/jwks.gointernal/server/jwks_cache_test.gointernal/server/jwks_server_test.gointernal/server/jwks_test.gointernal/server/observer.gointernal/server/server.gointernal/server/server_test_helper_test.gointernal/trust/filtered_store.gointernal/trust/observer.go
💤 Files with no reviewable changes (2)
- internal/server/jwks_cache_test.go
- internal/server/jwks_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
Collaborator
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/observability.go`:
- Around line 28-30: The composite branch loses the caller-provided
LoggerContext because newCompositeObserver rebuilds children using
NewObserver(&subCfg) which creates fresh logger contexts; change the composite
construction to thread the provided LoggerContext through so children use it.
Concretely: update newCompositeObserver (and any composite factory) to
accept/pass a LoggerContext and call NewObserverWithLogger for each subCfg (or
otherwise propagate logCtx into NewObserver calls), and adjust
NewObserverWithLogger/newCompositeObserver signatures and call sites so the
shared sink/formatter from LoggerContext is preserved across composite children.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83c88f1a-ff4b-4395-a64d-284fa3bdc130
📒 Files selected for processing (2)
internal/config/observability.gointernal/probe/logging.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/config/observability.go (1)
58-71:⚠️ Potential issue | 🟠 MajorKeep the raw sink in
LoggerContext, not the formatted writer.
NewLoggerContextcurrently persistsWriter: createWriter(...). When the base format is"text", that meansLoggerContext.Writeris already aConsoleWriter.EventLoggerthen feeds that writer back intocreateWriter, so a per-eventlog_format: jsonstill writes through the console formatter, and an explicittextoverride double-wraps it. Store the raw destination separately and derive format-specific writers from that raw sink when applying base and event formats.Also applies to: 101-103, 115-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/observability.go` around lines 58 - 71, NewLoggerContext should store the unwrapped raw sink (e.g., os.Stdout) in LoggerContext rather than the already-formatted writer; change LoggerContext to include a RawSink (io.Writer) and set RawSink = os.Stdout or the provided base sink, then create a formattedWriter := createWriter(cfg.LogFormat, RawSink) only to build the zerolog logger (zerolog.New(formattedWriter)...). Update EventLogger and any code paths around createWriter (referenced in the review around the EventLogger creation and the createWriter calls at the other locations) to call createWriter(eventFormat, LoggerContext.RawSink) when deriving per-event formatters so formats aren’t double-wrapped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cli/serve.go`:
- Around line 201-203: The direct startup/shutdown messages are using the base
logger (logger.Info()) instead of the server lifecycle logger, so replace those
direct calls to use the same event-scoped logger created for
LoggingServerLifecycleObserver; e.g., obtain the lifecycle logger via
config.EventLogger(logCtx, "server_lifecycle", infraCfg.serverLifecycle) (or
reuse the Observer.Logger) and call lifecycleLogger.Info(...) for the messages
around server start/stop (the places where LoggingServerLifecycleObserver is
constructed and the other spots noted), so the event=server_lifecycle field and
infraCfg.serverLifecycle settings are honored.
In `@internal/config/observability.go`:
- Around line 30-45: The composite branch in NewObserverWithLogger forwards the
parent LoggerContext unchanged so child observers lose their own
LogLevel/LogFormat defaults; fix by constructing a derived LoggerContext that
reuses the shared sink/output from the parent but lets subCfg defaults apply
(i.e., create a new LoggerContext for each subCfg using the parent's
sink/handler but not its LogLevel/LogFormat) before calling newCompositeObserver
and when iterating sub-configs (also apply the same change in the other
composite-handling site around the second occurrence referenced), ensuring
functions/types like NewObserverWithLogger, newCompositeObserver, LoggerContext,
ObservabilityConfig.LogLevel and LogFormat, and EventLogger are used to create
per-child contexts rather than passing the parent logCtx directly.
---
Duplicate comments:
In `@internal/config/observability.go`:
- Around line 58-71: NewLoggerContext should store the unwrapped raw sink (e.g.,
os.Stdout) in LoggerContext rather than the already-formatted writer; change
LoggerContext to include a RawSink (io.Writer) and set RawSink = os.Stdout or
the provided base sink, then create a formattedWriter :=
createWriter(cfg.LogFormat, RawSink) only to build the zerolog logger
(zerolog.New(formattedWriter)...). Update EventLogger and any code paths around
createWriter (referenced in the review around the EventLogger creation and the
createWriter calls at the other locations) to call createWriter(eventFormat,
LoggerContext.RawSink) when deriving per-event formatters so formats aren’t
double-wrapped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 577b8892-6338-4861-8a57-7c052f95ab14
📒 Files selected for processing (2)
internal/cli/serve.gointernal/config/observability.go
…text that shares the parent's raw output sink but applies the child config's LogLevel and/or LogFormat overrides. Per-event EventLogger overrides continue to work on top of the derived base. When a child specifies neither field, the parent context is returned as-is (zero-cost path).
…rvers Migrate the logging backend from log/slog to zerolog and replace all fmt.Printf/log.Printf calls with structured observer callbacks routed through domain-specific interfaces. Key changes: - Add observer interfaces in server, keys, datasource, trust, and config packages with logging implementations in probe/ - Add EventLogger and LoggerContext for per-event log level, format, and enabled/disabled overrides without runtime filtering handlers - Eliminate Provider temporal coupling by replacing SetObserver() with ProviderDeps struct injected at construction time - Add 7 new per-event config knobs: config_reload, datasource_cache, key_rotation, key_provider, trust_validation, jwks_cache, server_lifecycle - Validate log_level and log_format at startup for fast feedback on typos - Filter grpc.ErrServerStopped during shutdown to suppress spurious errors - Fix AWS KMS getKeyIDFromAlias to propagate real API errors instead of swallowing them as not-found - Extract test helpers to reduce NoopObserver boilerplate in tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Template:
Describe your changes
Key changes
Ticket reference (if applicable)
Fixes #
Checklist
Are the agreed upon acceptance criteria fulfilled?
Was the 4-eye-principle applied? (async PR review, pairing, ensembling)
Do your changes have passing automated tests and sufficient observability?
Are the work steps you introduced repeatable by others, either through automation or documentation?
The Changes were automatically built, tested, and - if needed, behind a feature flag - deployed to our production environment. (Please check this when the new deployment is done and you could verify it.)
Are the agreed upon coding/architectural practices applied?
Are security needs fullfilled? (e.g. no internal URL)
Is the corresponding Ticket in the right state? (should be on "review" now, put to done when this change made it to production)
For changes to the public API / code dependencies: Was the whole team (or a sufficient amount of ppl) able to review?
Summary by CodeRabbit
Refactor
New Features
Chores
Tests