HYPERFLEET-887 - feat: migrate Sentinel logger to slog with shared handler - #242
Open
kuudori wants to merge 2 commits into
Open
HYPERFLEET-887 - feat: migrate Sentinel logger to slog with shared handler#242kuudori wants to merge 2 commits into
kuudori wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change replaces the internal logger with Go’s Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 5 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 2397 lines (>500) | +2 |
| Sensitive paths | cmd/ | +2 |
| Test coverage | Missing tests for: cmd/sentinel internal/client internal/config internal/metrics | +1 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/health/health.go (1)
63-68: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPass the request context to
writeJSON.
context.Background()discards request correlation fields when JSON encoding fails. Add actx context.Contextparameter towriteJSONand passreq.Context()from both handlers.As per coding guidelines: “Thread
context.Contextthrough calls and preserve correlation keys.” As per path instructions: “Flagcontext.Background()when parent context exists.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/health/health.go` around lines 63 - 68, Update ReadinessChecker.writeJSON to accept a context.Context parameter and use it in the encoding-error slog.ErrorContext call instead of context.Background(). Update both handler call sites to pass req.Context(), preserving the request context and correlation fields.Sources: Coding guidelines, Path instructions
🧹 Nitpick comments (1)
internal/metrics/metrics.go (1)
275-280: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftPreserve caller context for metric validation logs.
These helpers create new background contexts. Their warning logs cannot include the poll operation, trace, or span fields. Thread
ctxthrough the metric APIs, or return validation failures for Sentinel to log at its service boundary.As per coding guidelines: “Thread
context.Contextthrough calls and preserve correlation keys” and “Log at service or system boundaries rather than deep in the call stack.” As per path instructions: “Flagcontext.Background()when parent context exists.”Also applies to: 311-314, 344-347, 377-385, 413-416, 445-448, 468-469
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/metrics/metrics.go` around lines 275 - 280, Update the metric helper APIs, including UpdatePendingResourcesMetric and the other validation helpers at the referenced call sites, to accept and use the caller’s context instead of creating context.Background(). Propagate ctx from Sentinel through every call so warning logs preserve poll, trace, and span correlation fields; alternatively return validation failures for Sentinel to log at its service boundary.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/config.go`:
- Around line 234-235: Move configuration diagnostics from LoadConfig to the
command boundary: remove both load-start and load-success slog.InfoContext calls
in internal/config/config.go, then after initLogging succeeds in
cmd/sentinel/main.go, emit the configuration-success log through the configured
default logger, preserving the existing context and file metadata.
In `@internal/logctx/logctx.go`:
- Around line 10-15: Add internal/logctx/logctx_test.go with a unit test for the
exported ContextFields function. Verify the returned fields register both
TopicKey ("topic") and DecisionReasonKey ("decision_reason"), preserving the
expected field order if the helper exposes it.
In `@pkg/telemetry/otel.go`:
- Around line 105-107: Add source comments at both intentional fallback
branches: the unrecognized sampler case in the sampler selection logic, and the
invalid or out-of-range OTEL_TRACES_SAMPLER_ARG handling that retains
defaultSamplingRate. Keep the existing logging and fallback behavior unchanged;
document that each branch intentionally degrades configuration and continues.
In `@test/integration/integration_test.go`:
- Around line 422-426: Add the logctx import and pass
hfl.WithContextFields(logctx.ContextFields()...) to the hfl.NewHandler options
so the integration handler registers decision_reason and topic on Published
event records.
- Around line 427-429: In the integration test setup, install the capture logger
before constructing the broker publisher so broker.NewPublisher receives and
stores that logger. Reorder the existing slog.SetDefault setup and publisher
creation while preserving restoration via prevDefault, ensuring publishing event
is emitted through the capture handler.
In `@test/integration/testcontainer.go`:
- Around line 62-65: Update the error path after broker.NewPublisher in the
container setup flow to check the error returned by container.Terminate(ctx) and
preserve both the publisher-creation and cleanup failures in the returned error.
Ensure every error return is handled without discarding the termination failure.
- Line 52: Update the RabbitMQ startup log in the testcontainer initialization
flow to stop logging the credential-bearing amqpURL value. Remove the "amqp_url"
field from the slog.InfoContext call, or replace it with a properly redacted URL
that cannot expose credentials.
---
Outside diff comments:
In `@internal/health/health.go`:
- Around line 63-68: Update ReadinessChecker.writeJSON to accept a
context.Context parameter and use it in the encoding-error slog.ErrorContext
call instead of context.Background(). Update both handler call sites to pass
req.Context(), preserving the request context and correlation fields.
---
Nitpick comments:
In `@internal/metrics/metrics.go`:
- Around line 275-280: Update the metric helper APIs, including
UpdatePendingResourcesMetric and the other validation helpers at the referenced
call sites, to accept and use the caller’s context instead of creating
context.Background(). Propagate ctx from Sentinel through every call so warning
logs preserve poll, trace, and span correlation fields; alternatively return
validation failures for Sentinel to log at its service boundary.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 90377db4-305f-4276-b11f-f383d035cb69
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (21)
AGENTS.mdcmd/sentinel/main.gogo.modinternal/client/client.gointernal/config/config.gointernal/health/health.gointernal/health/health_test.gointernal/logctx/logctx.gointernal/metrics/metrics.gointernal/payload/builder.gointernal/payload/builder_test.gointernal/sentinel/sentinel.gointernal/sentinel/sentinel_test.gopkg/logger/broker_adapter.gopkg/logger/logger.gopkg/logger/logger_test.gopkg/logger/operationid_middleware.gopkg/telemetry/otel.gopkg/telemetry/otel_test.gotest/integration/integration_test.gotest/integration/testcontainer.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (4)
- pkg/logger/operationid_middleware.go
- pkg/logger/broker_adapter.go
- pkg/logger/logger.go
- pkg/logger/logger_test.go
…ndler Replace custom pkg/logger with stdlib log/slog backed by the shared hyperfleet-logger handler. Context-based field enrichment (resource_type, topic, decision_reason, trace/span IDs) propagates automatically to all downstream log calls via the handler's context extraction. - Use hyperfleet-logger generic Key[T] API for type-safe context fields - Delete pkg/logger/ (633-line custom logger + tests) - Delete internal/brokeradapter/ (broker v1.2 accepts *slog.Logger directly) - Add internal/logctx/ for Sentinel-specific context field registration - Add context propagation and health handler log assertion tests - Fix structured logging in testcontainer (fmt.Sprintf -> attrs) Co-Authored-By: Claude <noreply@anthropic.com>
kuudori
force-pushed
the
HYPERFLEET-887-slog-migration
branch
from
August 5, 2026 20:48
e5170c2 to
e3ddb3a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/integration/integration_test.go`:
- Around line 529-530: Update the Sentinel event-log validation around the
resource_type check to preserve the documented subset compatibility field:
validate that entry["subset"] is present, or consistently migrate the contract
and all dependent consumers from subset to resource_type. Do not leave the test
asserting only resource_type while the established contract still requires
subset.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e196e871-f29a-4391-bd64-10ad7d24cd75
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (22)
AGENTS.mdcmd/sentinel/main.gogo.modinternal/client/client.gointernal/config/config.gointernal/health/health.gointernal/health/health_test.gointernal/logctx/logctx.gointernal/logctx/logctx_test.gointernal/metrics/metrics.gointernal/payload/builder.gointernal/payload/builder_test.gointernal/sentinel/sentinel.gointernal/sentinel/sentinel_test.gopkg/logger/broker_adapter.gopkg/logger/logger.gopkg/logger/logger_test.gopkg/logger/operationid_middleware.gopkg/telemetry/otel.gopkg/telemetry/otel_test.gotest/integration/integration_test.gotest/integration/testcontainer.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (5)
- pkg/logger/operationid_middleware.go
- internal/config/config.go
- pkg/logger/broker_adapter.go
- pkg/logger/logger_test.go
- pkg/logger/logger.go
🚧 Files skipped from review as they are similar to previous changes (15)
- go.mod
- AGENTS.md
- internal/logctx/logctx.go
- internal/health/health.go
- internal/client/client.go
- internal/payload/builder_test.go
- internal/metrics/metrics.go
- pkg/telemetry/otel.go
- cmd/sentinel/main.go
- test/integration/testcontainer.go
- internal/payload/builder.go
- pkg/telemetry/otel_test.go
- internal/health/health_test.go
- internal/sentinel/sentinel.go
- internal/sentinel/sentinel_test.go
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
1 participant
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.
Summary
pkg/logger(633-line logger + 824-line tests) with stdliblog/slogbacked by the sharedhyperfleet-loggerhandlerKey[T]APIinternal/brokeradapter/- broker v1.2 (HYPERFLEET-890) now accepts*slog.Loggerdirectly, no adapter neededinternal/logctx/for Sentinel-specific context field registration (topic, decision_reason)What changed
pkg/logger.HyperFleetLogger(633 LOC)log/slog+hyperfleet-loggerhandlercontext.WithValue+ extraction inbuildEntry()hfl.Set(ctx, key, val)+ handler auto-extractioninternal/brokeradapter/wrapping customLoggerinterfaceslog.Default()passed directly to brokerslog.With(...))Net
+376 / -1,961 lines across 22 files. Four packages deleted (
pkg/logger/,internal/brokeradapter/), one added (internal/logctx/- 15 lines).Test plan
make verify- go vet + format checkmake lint- golangci-lint, 0 issuesmake test-unit- all unit tests passTestTrigger_ContextFieldsPropagateToLogsverifies resource_type, topic, decision_reason appear in log output via context enrichmentTestHealthzHandler_LogsOnInvalidConfig,TestHealthzHandler_LogsOnStalePoll,TestReadyzHandler_LogsOnCheckFailure)make test-integration- requires Dockermake test-helm- no chart changes