feat: improve error access logs #1812
Conversation
StarpTech
changed the title
Router image scan passed✅ No security vulnerabilities found in image: |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
WalkthroughThis change introduces configurable log levels and stack trace options for access logs throughout the router. It updates configuration structs, schemas, and test data to support new Changes
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
router/pkg/config/config.schema.json (2)
673-677:add_stacktracecan leak PII – consider documenting/guarding its usageEnabling full stack traces in access logs is invaluable for debugging, but it can also dump request payloads, header values, and JWTs into logs.
You may want to:
- Add a short warning to the description (same place as for
log_level,dev_mode, etc.).- Optionally gate this flag behind
dev_modeusing a conditional schema (if:{ "properties": { "dev_mode": { "const": true } } }→then:{ "properties": { "add_stacktrace": { ... } } }).
3174-3176: Duplication between multiple enum blocks – consider centralising
response_erroris now added in at least two different enum lists (router & subgraph log fields).
To avoid future drift, think about moving the canonical list into a single$defsentry and referencing it via$refinstead of duplicating literals.router/cmd/instance.go (1)
196-196: Use consistent field reference for better maintainability.For consistency with the file logger configuration and the non-buffered stdout logger, use
c.AddStacktraceinstead ofcfg.AccessLogs.AddStacktrace.- Stacktrace: cfg.AccessLogs.AddStacktrace, + Stacktrace: c.AddStacktrace,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
router-tests/go.mod(2 hunks)router-tests/structured_logging_test.go(3 hunks)router-tests/telemetry/telemetry_test.go(2 hunks)router-tests/testenv/testenv.go(1 hunks)router/cmd/instance.go(2 hunks)router/core/engine_loader_hooks.go(2 hunks)router/core/errors.go(2 hunks)router/core/graph_server.go(1 hunks)router/core/graphql_handler.go(2 hunks)router/core/request_context_fields.go(7 hunks)router/core/router.go(1 hunks)router/go.mod(2 hunks)router/internal/requestlogger/requestlogger.go(5 hunks)router/internal/requestlogger/subgraphlogger.go(1 hunks)router/internal/requestlogger/subgraphlogger_test.go(7 hunks)router/pkg/config/config.go(1 hunks)router/pkg/config/config.schema.json(3 hunks)router/pkg/config/testdata/config_defaults.json(1 hunks)router/pkg/config/testdata/config_full.json(1 hunks)router/pkg/logging/logging.go(6 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: SkArchon
PR: wundergraph/cosmo#2004
File: router-tests/structured_logging_test.go:1946-1997
Timestamp: 2025-07-02T17:26:35.244Z
Learning: In the router's access logging system, feature flag headers (like X-Feature-Flag) are automatically added to log output through a separate mechanism, not requiring explicit AccessLogFields configuration to map the header to a log field.
router/pkg/config/testdata/config_full.json (1)
Learnt from: SkArchon
PR: wundergraph/cosmo#2004
File: router-tests/structured_logging_test.go:1946-1997
Timestamp: 2025-07-02T17:26:35.244Z
Learning: In the router's access logging system, feature flag headers (like X-Feature-Flag) are automatically added to log output through a separate mechanism, not requiring explicit AccessLogFields configuration to map the header to a log field.
router/pkg/config/testdata/config_defaults.json (1)
Learnt from: SkArchon
PR: wundergraph/cosmo#2004
File: router-tests/structured_logging_test.go:1946-1997
Timestamp: 2025-07-02T17:26:35.244Z
Learning: In the router's access logging system, feature flag headers (like X-Feature-Flag) are automatically added to log output through a separate mechanism, not requiring explicit AccessLogFields configuration to map the header to a log field.
router/core/router.go (1)
Learnt from: SkArchon
PR: wundergraph/cosmo#2004
File: router-tests/structured_logging_test.go:1946-1997
Timestamp: 2025-07-02T17:26:35.244Z
Learning: In the router's access logging system, feature flag headers (like X-Feature-Flag) are automatically added to log output through a separate mechanism, not requiring explicit AccessLogFields configuration to map the header to a log field.
router/core/graph_server.go (1)
Learnt from: SkArchon
PR: wundergraph/cosmo#2004
File: router-tests/structured_logging_test.go:1946-1997
Timestamp: 2025-07-02T17:26:35.244Z
Learning: In the router's access logging system, feature flag headers (like X-Feature-Flag) are automatically added to log output through a separate mechanism, not requiring explicit AccessLogFields configuration to map the header to a log field.
router-tests/structured_logging_test.go (1)
Learnt from: endigma
PR: wundergraph/cosmo#2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
router/cmd/instance.go (1)
Learnt from: SkArchon
PR: wundergraph/cosmo#2004
File: router-tests/structured_logging_test.go:1946-1997
Timestamp: 2025-07-02T17:26:35.244Z
Learning: In the router's access logging system, feature flag headers (like X-Feature-Flag) are automatically added to log output through a separate mechanism, not requiring explicit AccessLogFields configuration to map the header to a log field.
router/pkg/config/config.go (2)
Learnt from: SkArchon
PR: wundergraph/cosmo#2004
File: router-tests/structured_logging_test.go:1946-1997
Timestamp: 2025-07-02T17:26:35.244Z
Learning: In the router's access logging system, feature flag headers (like X-Feature-Flag) are automatically added to log output through a separate mechanism, not requiring explicit AccessLogFields configuration to map the header to a log field.
Learnt from: endigma
PR: wundergraph/cosmo#2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
router/pkg/config/config.schema.json (3)
undefined
<retrieved_learning>
Learnt from: endigma
PR: #2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
</retrieved_learning>
<retrieved_learning>
Learnt from: SkArchon
PR: #2004
File: router-tests/structured_logging_test.go:1946-1997
Timestamp: 2025-07-02T17:26:35.244Z
Learning: In the router's access logging system, feature flag headers (like X-Feature-Flag) are automatically added to log output through a separate mechanism, not requiring explicit AccessLogFields configuration to map the header to a log field.
</retrieved_learning>
<retrieved_learning>
Learnt from: SkArchon
PR: #1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
</retrieved_learning>
🧬 Code Graph Analysis (7)
router/core/router.go (1)
router/pkg/config/config.go (1)
CustomAttribute(39-43)
router/internal/requestlogger/subgraphlogger_test.go (1)
router/pkg/logging/logging.go (1)
NewZapLoggerWithCore(83-89)
router/core/graph_server.go (2)
router/internal/requestlogger/requestlogger.go (1)
WithLogLevelHandler(101-105)router/core/request_context_fields.go (1)
LogLevelHandler(106-122)
router/core/graphql_handler.go (1)
router/core/access_controller.go (1)
ErrUnauthorized(14-14)
router-tests/structured_logging_test.go (1)
router/pkg/logging/logging.go (1)
NewZapAccessLogger(111-129)
router/cmd/instance.go (1)
router/pkg/logging/logging.go (4)
ZapLogLevelFromString(168-185)NewZapAccessLogger(111-129)NewJSONZapBufferedLogger(146-158)BufferedLoggerOptions(136-144)
router-tests/testenv/testenv.go (1)
router/pkg/logging/logging.go (2)
NewZapLoggerWithCore(83-89)NewZapLogger(91-109)
🪛 GitHub Actions: Router CI
router/core/router.go
[error] 617-617: Failed to create graph server due to invalid configuration: maxEntriesPerBatch must be greater than 0.
[error] 617-617: Failed to create graph server due to invalid configuration: maxConcurrent must be greater than 0.
[error] 617-617: Failed to create graph server. Error: failed to build base mux: failed to create operation blocker: failed to compile non-persisted expression: line 1, column 15: type expr.RequestHeaders has no field Header.
[error] 617-617: Failed to create graph server. Error: failed to build base mux: failed to build pubsub configuration: failed to find Kafka provider with ID "my-kafka". Ensure the provider definition is part of the config.
[error] 617-617: Failed to create graph server. Error: failed to build base mux: failed to build pubsub configuration: failed to find Nats provider with ID "default". Ensure the provider definition is part of the config.
[error] 1249-1249: Error watching execution config: context canceled.
[error] 1249-1249: Error watching execution config: context canceled.
router/internal/requestlogger/requestlogger.go
[error] 195-195: HTTP 405 Method Not Allowed error on /graphql path for mutation updateEmployeeTag.
[error] 195-195: HTTP 413 Request Entity Too Large error on /graphql path.
[error] 195-195: HTTP 429 Too Many Requests error on /graphql path.
[error] 195-195: HTTP 401 Unauthorized error on /graphql path.
[error] 195-195: Failed to create gzip reader: gzip: invalid header.
🔇 Additional comments (33)
router/core/router.go (1)
153-160: Enforce compile-time log-level safety in AccessLogsConfigIntroduce a new
AccessLogLeveltype with predefined constants, updateAccessLogsConfig.Levelto use it, and validate the incoming value during config parsing to fail fast on unknown levels (avoids silent “no logs” in prod).• Create in
router/core/router.go(or a sharedconfigpackage):// AccessLogLevel restricts valid values at compile-time type AccessLogLevel string const ( AccessLogInfo AccessLogLevel = "info" AccessLogError AccessLogLevel = "error" )• Update the struct:
type AccessLogsConfig struct { Attributes []config.CustomAttribute - Level string + Level AccessLogLevel Logger *zap.Logger SubgraphEnabled bool SubgraphAttributes []config.CustomAttribute AddStacktrace bool }• In your config-loading logic, parse the raw string into
AccessLogLevel, map tozapcore.Level, and return an error if it’s not one of the constants.router/go.mod (1)
34-35: Verify the pseudo-version bump for graphql-go-toolsThe module is pinned to commit
430af8af6c64.
The surrounding comment (“Do not upgrade, it renames attributes we rely on”) still applies, so please confirm that this specific commit does not include the breaking rename.
If it does, the router will fail to compile at the nextgo mod tidy.router/pkg/config/testdata/config_full.json (1)
328-330: LGTM – new keys match the code changes
LevelandAddStacktraceare added consistently.
Be sure the JSON schema & documentation were updated alongside (looks like they were).router/pkg/config/testdata/config_defaults.json (1)
162-165: Defaults are sensibleKeeping the default at
"info"andfalsemaintains previous behaviour while exposing the knob – good choice.router-tests/go.mod (1)
29-30: Mirror dependency bump acknowledgedTest module tracks the same pseudo-version → avoids “go: module declares its path” mismatches.
router/core/engine_loader_hooks.go (2)
7-8: LGTM! Import reordering is appropriate.The reordering of imports for "slices" and "time" is clean and follows standard Go conventions.
153-157: Excellent implementation of conditional logging based on error presence.This change perfectly aligns with the PR objectives to implement configurable log levels. The conditional logic correctly logs errors at Error level and successful operations at Info level, enabling the router to differentiate between failed and successful subgraph operations in access logs.
router/internal/requestlogger/subgraphlogger_test.go (2)
5-6: LGTM! Import reordering is clean.The reordering of imports is appropriate and follows standard conventions.
24-24: Correct implementation of stacktrace parameter in logger constructors.The addition of the
trueparameter tologging.NewZapLoggerWithCorecalls correctly implements the new stacktrace functionality introduced in the logging package. This enables stack trace inclusion in test logs, which aligns with the PR's goal of adding configurable stacktrace options.Also applies to: 52-52, 81-81, 115-115, 149-149, 199-199
router-tests/testenv/testenv.go (1)
411-411: Proper implementation of stacktrace parameter in test environment loggers.Both logger constructor calls (
NewZapLoggerWithCoreandNewZapLogger) have been correctly updated to include the stacktrace boolean parameter set totrue. This ensures that the test environment benefits from the enhanced logging capabilities with stacktrace support, maintaining consistency with the broader logging infrastructure improvements.Also applies to: 418-418
router/core/graph_server.go (1)
899-899: LGTM! Proper integration of dynamic log level handling.The addition of the
LogLevelHandleroption correctly enables the access logger to dynamically set log levels based on request context errors, which aligns perfectly with the PR objectives for configurable access log levels.router/internal/requestlogger/subgraphlogger.go (2)
59-59: LGTM! Ensures complete field inclusion.The modification correctly appends subgraph-specific fields to the existing fields slice, ensuring all relevant fields are included in the logged output.
68-70: LGTM! Consistent error-level logging implementation.The new
Errormethod follows the same pattern as the existingInfomethod and properly supports the new configurable log level functionality introduced in this PR.router-tests/telemetry/telemetry_test.go (2)
6769-6770: LGTM: Improved test robustness by splitting error message assertions.The change from a single concatenated string check to two separate
require.Containsassertions improves test maintainability and robustness. This approach is order-independent and doesn't rely on specific error message concatenation formats, making it more resilient to future changes in error composition.
6780-6781: LGTM: Consistent error assertion pattern applied.The same improved assertion pattern is consistently applied here, ensuring both key error components are verified without depending on their exact arrangement in the status description.
router-tests/structured_logging_test.go (3)
13-18: LGTM! Good import organization.The import reordering improves code readability by grouping related imports together.
197-197: LGTM! Correctly updated function call signature.The function call properly includes the new log level and stacktrace parameters, aligning with the updated
NewZapAccessLoggersignature.
253-253: LGTM! Consistent function call update.The function call correctly implements the new signature, maintaining consistency with the previous test case.
router/pkg/config/config.go (1)
765-772: LGTM: Well-structured configuration additions.The new
LevelandAddStacktracefields are properly integrated with appropriate defaults, YAML tags, and environment variable mappings. This aligns well with the PR objective to provide configurable access log verbosity.router/internal/requestlogger/requestlogger.go (2)
101-105: LGTM: Clean option pattern implementation.The
WithLogLevelHandlerfunction follows the established option pattern and provides a clean way to configure dynamic log level determination.
189-195: LGTM: Proper dynamic log level implementation.The logic correctly defaults to
InfoLeveland delegates to the log level handler when available. This enables request-specific log level determination, which is essential for the error-aware logging feature.router/core/errors.go (2)
177-188: LGTM: Improved error tracking with explicit error handling.The refactoring from
propagateSubgraphErrorstotrackResponseErrorwith explicit error parameters improves clarity and makes error handling more predictable. The fallback to execution context error when no explicit error is provided maintains backward compatibility.
283-285: LGTM: Simplified response writing.Removing the unnecessary byte slice conversion streamlines the response writing logic.
router/core/graphql_handler.go (3)
180-182: LGTM: Explicit error tracking for synchronous responses.The deferred call to
trackResponseErrorwith the expliciterrparameter improves error handling clarity and ensures proper error propagation from the resolver.
214-216: LGTM: Consistent error tracking for subscriptions.The error tracking approach is consistent with the synchronous response handling, properly capturing resolver errors for subscription responses.
232-232: LGTM: Proper error tracking for unsupported plans.Using
trackResponseErrorwith the specificerrOperationPlanUnsupportederror maintains consistency with the new error tracking pattern.router/pkg/config/config.schema.json (2)
664-672: Schema fieldlevellooks good – double-check that the parser accepts lower-case strings onlyThe enum restricts the value to
"info" | "error", both lower-case.
If the router internally maps log-levels using the standard Golog/slogorzapenums, those usually expect upper-case (e.g."INFO"). Please confirm that the loader performs a case-insensitive match or explicit mapping, otherwise configurations with the default"info"could be rejected at runtime.
766-778: New context fieldresponse_error– verify collector & testsThe extra enum value is fine, but please ensure:
router/internal/requestlogger/*actually setsContextKeyResponseErrorfor both router and subgraph scopes.- Existing structured-logging tests cover the new field; otherwise metrics cardinality regressions can slip in unnoticed.
router/cmd/instance.go (1)
159-166: LGTM! Proper configuration setup with good error handling.The access log configuration correctly maps the new
LevelandAddStacktracefields, and includes appropriate error handling for invalid log levels.router/pkg/logging/logging.go (1)
65-81: Well-implemented stacktrace configuration.The conditional addition of stacktrace at ErrorLevel and above is a good design choice that balances debugging capability with log verbosity.
router/core/request_context_fields.go (3)
64-77: Robust error field handling with proper multiError support.The function correctly handles both single errors and multiError types, ensuring proper logging of error arrays.
106-122: Excellent defensive programming in log level determination.The function includes proper nil checks and correctly implements the dynamic log level feature based on error presence.
332-346: Clean error string extraction helper.The
getErrorStringfunction safely handles multiple input types and provides a centralized way to extract error messages.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Motivation and Context
This PR enhances our GraphQL router’s logging and error pipelines, giving you fine-grained control over access-log verbosity, richer diagnostics in both router and subgraph logs, and tidier error propagation.
🎯 What’s Changed
Configurable Access-Log Level
"level"to control which requests get logged ("info"vs"error").Optional Stack Traces
"add_stacktrace"flag lets you include full stack traces in your access logs when errors occur.New
response_errorcontext fieldCleaner Error Composition
⚙️ JSON Config Example
Related wundergraph/graphql-go-tools#1132
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation