HYPERFLEET-458 - Migrate from glog to slog-based structured logging #28
Conversation
Replace AMD64-only base image with multi-arch eclipse-temurin:17-jdk. Auto-detect architecture to download correct Go binary (arm64/amd64). Download openapi-generator-cli JAR directly from Maven Central. This enables `make generate` to work on ARM64 Macs without QEMU crashes.
|
[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 |
WalkthroughThe PR replaces glog with Go's structured slog across the codebase and introduces a new logging subsystem in pkg/logger (global initialization, context-aware handler, legacy-wrapper, HTTP logging helpers). It adds a LoggingConfig and wires it into ApplicationConfig, updates many files to use slog or the new logger API, and adds context helpers for tracing fields. The Dockerfile.openapi is replaced to use an Eclipse Temurin Java base, installs Go per-architecture, downloads the openapi-generator-cli JAR, and invokes it via java -jar. Sequence Diagram(s)(Skipped — changes are primarily logging library replacement, new logger package, config wiring, and build recipe updates; there is no new multi-component runtime control flow requiring a sequence diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-12-12T11:04:29.971ZApplied to files:
📚 Learning: 2025-12-12T11:04:29.971ZApplied to files:
📚 Learning: 2025-12-12T11:04:29.971ZApplied to files:
📚 Learning: 2025-12-12T11:04:29.971ZApplied to files:
📚 Learning: 2025-12-12T11:04:29.971ZApplied to files:
📚 Learning: 2025-12-12T11:04:29.971ZApplied to files:
📚 Learning: 2025-12-12T11:04:29.971ZApplied to files:
🧬 Code graph analysis (1)test/helper.go (1)
🔇 Additional comments (3)
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: 8
🤖 Fix all issues with AI Agents
In @cmd/hyperfleet-api/server/metrics_server.go:
- Line 58: The TLS startup log in metrics_server.go currently uses
env().Config.Server.BindAddress but should use the metrics-specific bind
address; update the slog.Info call that logs "Serving Metrics with TLS" to
reference env().Config.Metrics.BindAddress (matching the existing non-TLS log
that uses env().Config.Metrics.BindAddress) so both TLS and non-TLS logs
consistently show the metrics bind address.
In @Dockerfile.openapi:
- Around line 19-21: The Dockerfile currently downloads
openapi-generator-cli-7.16.0.jar via the wget command; update the URL and
filename to use version 7.18.0 (change occurrences of
openapi-generator-cli-7.16.0.jar to openapi-generator-cli-7.18.0.jar in the RUN
wget line) unless 7.16.0 is intentionally pinned, in which case add a clarifying
comment above the RUN wget line explaining the stability or compatibility reason
for pinning to 7.16.0.
In @pkg/api/error.go:
- Line 6: Replace direct usage of log/slog with the project's structured logger:
remove the "log/slog" import and import pkg/logger instead; update all logging
calls in this file (the error-handling helpers and any places referenced at the
comment locations) to use logger.FromContext(ctx).Error/Info (or the appropriate
level) and attach fields via logger.Extra() (e.g.,
logger.FromContext(ctx).Error("msg", logger.Extra("trace_id", traceID,
"span_id", spanID, "request_id", reqID, "err", err))). If any functions that log
do not currently accept a context.Context, add a ctx parameter or obtain a
context where appropriate so you can call logger.FromContext(ctx); keep message
text but replace slog field usage with logger.Extra() calls.
In @pkg/auth/authz_middleware_mock.go:
- Line 4: Replace the direct "log/slog" usage with the project logger
(pkg/logger) and use its Extra() context-enrichment API: remove the "log/slog"
import, import pkg/logger, and change any slog.* calls in
authz_middleware_mock.go to calls that obtain the logger and add required fields
via logger.Extra(...).Specifically, ensure the log invocation uses the logger
package's Extra() to include component, version, hostname and the correlation
fields (trace_id, span_id, request_id) (e.g., logger.Extra(ctx,
...).Info/Error(...)) so mock logging follows the same structured format as
production code.
In @pkg/db/db_session/testcontainer.go:
- Line 71: The slog.Info call currently logs the full connStr (variable connStr)
which may contain credentials; update the logging in the testcontainer code so
it does not emit sensitive data: either remove connStr from the slog.Info call
or parse/sanitize connStr to log only non-sensitive fields (e.g., host, port,
database name) or a safe placeholder like "<redacted-credentials>". Locate the
slog.Info invocation that references connStr and replace it with a sanitized log
message that omits username/password.
In @pkg/logger/logger.go:
- Around line 147-164: The Default() function has a race: after releasing
globalMu read lock another goroutine may initialize globalLogger, causing double
initialization; change the check-and-initialize to be atomic by either using a
package-level sync.Once (e.g. a once variable and call once.Do(func(){
Initialize(...) })) or by upgrading to a write lock for the initialize path (use
globalMu.Lock()/Unlock() to check nil and call Initialize), and ensure you still
return globalLogger; update references to Default(), globalMu, Initialize,
globalLogger accordingly.
- Around line 196-213: The NewOCMLogger function is injecting operation_id into
attrs, causing duplicate operation_id when contextHandler.Handle also adds it;
remove the operation_id addition from NewOCMLogger by stopping the append of
slog.String("operation_id", ...) (leave account_id and tx_id logic intact) so
that only contextHandler.Handle is responsible for adding operation_id before
calling l.With(...).
🧹 Nitpick comments (11)
pkg/handlers/openapi.go (1)
41-41: Consider adding structured fields to startup logs.These
slog.Infocalls could benefit from structured fields for consistency with the coding guidelines. For example, adding context like the source file path or component name would align with the structured logging pattern used elsewhere in this file (lines 64-66, 76-78).🔎 Example structured logging approach
- slog.Info("Loaded fully resolved OpenAPI specification from embedded pkg/api/openapi/api/openapi.yaml") + slog.Info("Loaded fully resolved OpenAPI specification", "source", "pkg/api/openapi/api/openapi.yaml")- slog.Info("Loaded OpenAPI UI HTML from embedded file") + slog.Info("Loaded OpenAPI UI HTML", "source", "openapi-ui.html")Also applies to: 51-51
test/helper.go (1)
206-206: Consider clarifying the comment.The comment references the
'v'flag which was specific to glog's verbosity flag. Since the migration to slog, this reference may be less clear. Consider updating the comment to be more generic about avoiding flag redefinition conflicts.🔎 Suggested comment update
- // Also on reset, we don't care to be re-defining 'v' and other flags + // Also on reset, we don't care about redefining flagstest/integration/integration_test.go (1)
5-5: Consider using the centralized logger for consistency.The test code uses
slogdirectly rather than the centralizedpkg/loggerpackage. While this works, usinglogger.Info()andlogger.Warn()frompkg/loggerwould ensure consistency with the rest of the codebase and automatically include standard fields like component, version, and hostname.🔎 Example refactor to use centralized logger
Import the logger package:
import ( "flag" - "log/slog" "os" "path/filepath" "runtime" "testing" "github.com/openshift-hyperfleet/hyperfleet-api/test" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" )Initialize the logger before use:
func TestMain(m *testing.M) { flag.Parse() + logger.Initialize(logger.Config{ + Level: "info", + Format: "text", + Output: "stdout", + }) - slog.Info("Starting integration test", "go_version", runtime.Version()) + logger.Info("Starting integration test", "go_version", runtime.Version())Update other log calls similarly.
Based on learnings, pkg/**/*.go should use structured logging via pkg/logger/logger.go.
Also applies to: 16-16, 25-25, 38-38, 41-41
cmd/hyperfleet-api/server/logging/formatter_json.go (1)
19-23: Consider caching the debug flag for better performance.The
isDebugEnabled()function reads theLOG_LEVELenvironment variable on every call. While the performance impact is likely minimal for logging operations, caching this value at initialization would be more efficient and align with standard practices for configuration that doesn't change at runtime.🔎 Proposed refactor to cache the debug flag
+var debugEnabled bool + +func init() { + level := strings.ToLower(os.Getenv("LOG_LEVEL")) + debugEnabled = (level == "debug") +} + -// isDebugEnabled checks if debug logging is enabled via LOG_LEVEL func isDebugEnabled() bool { - level := strings.ToLower(os.Getenv("LOG_LEVEL")) - return level == "debug" + return debugEnabled }pkg/config/logging.go (2)
22-41: Add validation for logging configuration values.The configuration accepts
Level,Format, andOutputas strings without validation. Invalid values (e.g.,Level="invalid",Format="xml",Output="file") could be passed through topkg/logger, potentially causing unexpected behavior or silent failures.🔎 Proposed validation in NewLoggingConfig
func NewLoggingConfig() *LoggingConfig { cfg := &LoggingConfig{ Level: "info", Format: "text", Output: "stdout", } // Apply environment variables as overrides to defaults // Flags (via AddFlags + Parse) will override these if explicitly set if level := os.Getenv("LOG_LEVEL"); level != "" { + level = strings.ToLower(level) + if level != "debug" && level != "info" && level != "warn" && level != "error" { + // Log warning and use default + level = "info" + } cfg.Level = level } if format := os.Getenv("LOG_FORMAT"); format != "" { + format = strings.ToLower(format) + if format != "text" && format != "json" { + format = "text" + } cfg.Format = format } if output := os.Getenv("LOG_OUTPUT"); output != "" { + output = strings.ToLower(output) + if output != "stdout" && output != "stderr" { + output = "stdout" + } cfg.Output = output } return cfg }Note: Add
"strings"import if not already present.
45-49: Add flag value validation to prevent invalid configurations.The flag registration doesn't validate that users provide valid values. Consider adding validation or using enum-like types to constrain the valid options.
🔎 Proposed flag validation
One approach is to use pflag's ValidValues or add custom validation after flag parsing. Example:
// In AddFlags, document valid values more explicitly flagset.StringVar(&c.Level, "log-level", c.Level, "Minimum log level (valid: debug, info, warn, error)") flagset.StringVar(&c.Format, "log-format", c.Format, "Log output format (valid: text, json)") flagset.StringVar(&c.Output, "log-output", c.Output, "Log output destination (valid: stdout, stderr)")Then add a Validate() method:
// Validate checks if the configuration values are valid func (c *LoggingConfig) Validate() error { level := strings.ToLower(c.Level) if level != "debug" && level != "info" && level != "warn" && level != "error" { return fmt.Errorf("invalid log level: %s", c.Level) } format := strings.ToLower(c.Format) if format != "text" && format != "json" { return fmt.Errorf("invalid log format: %s", c.Format) } output := strings.ToLower(c.Output) if output != "stdout" && output != "stderr" { return fmt.Errorf("invalid log output: %s", c.Output) } return nil }Call this after flag parsing in the main initialization flow.
cmd/hyperfleet-api/server/healthcheck_server.go (1)
53-60: Consider usingpkg/loggerinstead ofslogdirectly.The logging migration uses
slog.Infodirectly, but the newpkg/loggerpackage provides a context-aware logger that automatically enriches logs withcomponent,version,hostname, and correlation IDs. For consistency with the HyperFleet Logging Specification mentioned in the PR objectives, consider using the logger package:🔎 Proposed refactor to use pkg/logger
import ( "context" "fmt" - "log/slog" "net" "net/http" health "github.com/docker/go-healthcheck" "github.com/gorilla/mux" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" )Then update the log calls:
- slog.Info("Serving HealthCheck with TLS", "address", env().Config.HealthCheck.BindAddress) + logger.Default().Info("Serving HealthCheck with TLS", "address", env().Config.HealthCheck.BindAddress)Based on learnings, use structured logging via pkg/logger/logger.go.
cmd/hyperfleet-api/server/api_server.go (2)
121-130: Consider usingpkg/loggerfor consistency with the logging specification.Similar to the healthcheck server, these log statements use
slogdirectly rather than the centralizedpkg/loggerpackage which adds standard fields (component, version, hostname) and context-based correlation IDs.🔎 Proposed refactor
- slog.Info("Serving with TLS", "address", env().Config.Server.BindAddress) + logger.Default().Info("Serving with TLS", "address", env().Config.Server.BindAddress)- slog.Info("Serving without TLS", "address", env().Config.Server.BindAddress) + logger.Default().Info("Serving without TLS", "address", env().Config.Server.BindAddress)- slog.Info("Web server terminated") + logger.Default().Info("Web server terminated")Based on learnings, use structured logging via pkg/logger/logger.go.
151-153: Consider usingpkg/loggerfor the database error log.This should also use the centralized logger for consistency.
- slog.Error("Error closing database connection", "error", err) + logger.Default().Error("Error closing database connection", "error", err)pkg/logger/context.go (2)
10-21: Inconsistent naming convention for context keys.The context key values use mixed naming conventions:
trace_id,span_id,request_iduse snake_caseaccountIDuses camelCasetxidis an abbreviationFor consistency with the HyperFleet Logging Specification (which uses snake_case for log fields like
trace_id,span_id,request_id), consider aligning all keys:🔎 Proposed fix
const ( // TraceIDKey is the context key for distributed trace ID (OpenTelemetry) TraceIDKey contextKey = "trace_id" // SpanIDKey is the context key for current span ID (OpenTelemetry) SpanIDKey contextKey = "span_id" // RequestIDKey is the context key for HTTP request ID RequestIDKey contextKey = "request_id" // AccountIDKey is the context key for account ID - AccountIDKey contextKey = "accountID" + AccountIDKey contextKey = "account_id" // TransactionIDKey is the context key for transaction ID - TransactionIDKey contextKey = "txid" + TransactionIDKey contextKey = "transaction_id" )Note: If these keys must match existing context values set elsewhere in the codebase, verify that those locations are also updated.
66-72: Simplify GetAccountID to match other getter patterns.The nested nil check differs from the pattern used in other getters. Consider using the same single type assertion pattern for consistency:
🔎 Proposed fix
// GetAccountID retrieves the account ID from context func GetAccountID(ctx context.Context) string { - if accountID := ctx.Value(AccountIDKey); accountID != nil { - if s, ok := accountID.(string); ok { - return s - } + if accountID, ok := ctx.Value(AccountIDKey).(string); ok { + return accountID } return "" }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
Dockerfile.openapicmd/hyperfleet-api/environments/framework.gocmd/hyperfleet-api/main.gocmd/hyperfleet-api/migrate/cmd.gocmd/hyperfleet-api/servecmd/cmd.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/healthcheck_server.gocmd/hyperfleet-api/server/logging/formatter_json.gocmd/hyperfleet-api/server/metrics_server.gocmd/hyperfleet-api/server/server.gogo.modpkg/api/error.gopkg/auth/authz_middleware_mock.gopkg/config/config.gopkg/config/logging.gopkg/db/db_session/test.gopkg/db/db_session/testcontainer.gopkg/db/migrations.gopkg/errors/errors.gopkg/handlers/openapi.gopkg/logger/context.gopkg/logger/http.gopkg/logger/logger.gotest/helper.gotest/integration/integration_test.go
🧰 Additional context used
📓 Path-based instructions (7)
pkg/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use structured logging via pkg/logger/logger.go with Extra() method for adding context fields
Files:
pkg/errors/errors.gopkg/db/migrations.gopkg/config/logging.gopkg/auth/authz_middleware_mock.gopkg/logger/context.gopkg/handlers/openapi.gopkg/db/db_session/testcontainer.gopkg/config/config.gopkg/db/db_session/test.gopkg/logger/http.gopkg/api/error.gopkg/logger/logger.go
test/integration/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Testcontainers to spin up isolated PostgreSQL instances for integration tests
Files:
test/integration/integration_test.go
pkg/{handlers,services}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Retrieve database sessions from context via db.NewContext() rather than creating new GORM connections
Files:
pkg/handlers/openapi.go
pkg/handlers/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/handlers/**/*.go: Automatically convert errors to OpenAPI error responses using errors.SendError() in HTTP handlers
Parse HTTP request bodies using json.NewDecoder(r.Body) in handler functions
Write HTTP responses using json.NewEncoder(w) for consistency with request parsing
Ensure all HTTP handlers follow the pattern: parse request, call service/DAO, handle errors, send response
Implement handlers as methods on handler structs that accept (w http.ResponseWriter, r *http.Request) parameters
Set HTTP status codes correctly: 201 Created for POST, 200 OK for GET, 204 No Content for DELETE operations
Files:
pkg/handlers/openapi.go
pkg/{dao,handlers}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Support pagination in list operations via ListArgs parameter with configurable limit and offset
Files:
pkg/handlers/openapi.go
pkg/{handlers,services,dao}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Return ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields in error handling
Files:
pkg/handlers/openapi.go
pkg/{db/transaction_middleware.go,handlers}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Store request operation IDs in context for tracing and debugging purposes in error logs
Files:
pkg/handlers/openapi.go
🧠 Learnings (25)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/**/*.go : Use structured logging via pkg/logger/logger.go with Extra() method for adding context fields
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/**/*.go : Use structured logging via pkg/logger/logger.go with Extra() method for adding context fields
Applied to files:
pkg/errors/errors.gotest/integration/integration_test.gocmd/hyperfleet-api/server/logging/formatter_json.gocmd/hyperfleet-api/server/api_server.gopkg/db/migrations.gopkg/config/logging.gocmd/hyperfleet-api/migrate/cmd.gocmd/hyperfleet-api/environments/framework.gopkg/auth/authz_middleware_mock.gocmd/hyperfleet-api/server/metrics_server.gopkg/logger/context.gopkg/handlers/openapi.gopkg/db/db_session/testcontainer.gopkg/config/config.gopkg/db/db_session/test.gopkg/logger/http.gopkg/api/error.gocmd/hyperfleet-api/servecmd/cmd.gotest/helper.gocmd/hyperfleet-api/server/server.gopkg/logger/logger.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/api/openapi_embed.go : Embed OpenAPI specification at compile time using //go:embed directive
Applied to files:
pkg/errors/errors.gotest/integration/integration_test.goDockerfile.openapipkg/handlers/openapi.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/api/openapi/model_*.go : Support JSON request/response serialization with proper struct tags on generated OpenAPI models
Applied to files:
pkg/errors/errors.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/handlers/**/*.go : Automatically convert errors to OpenAPI error responses using errors.SendError() in HTTP handlers
Applied to files:
pkg/errors/errors.gopkg/handlers/openapi.gopkg/api/error.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to cmd/hyperfleet/server/**/*.go : Extract user account ID and username from JWT claims for audit logging and authorization checks
Applied to files:
pkg/errors/errors.gocmd/hyperfleet-api/server/api_server.gopkg/handlers/openapi.gopkg/api/error.gocmd/hyperfleet-api/servecmd/cmd.gotest/helper.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to cmd/hyperfleet/server/**/*.go : Validate JWT tokens from Red Hat SSO in middleware before allowing requests to handlers
Applied to files:
pkg/errors/errors.gocmd/hyperfleet-api/server/api_server.gopkg/auth/authz_middleware_mock.gopkg/api/error.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{handlers,services,dao}/**/*.go : Return ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields in error handling
Applied to files:
pkg/errors/errors.gopkg/api/error.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/db/migrations/**/*.go : Use GORM AutoMigrate for schema management instead of manual SQL scripts
Applied to files:
test/integration/integration_test.gopkg/db/migrations.gocmd/hyperfleet-api/migrate/cmd.gocmd/hyperfleet-api/main.gopkg/db/db_session/testcontainer.gopkg/db/db_session/test.gogo.mod
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to test/integration/**/*.go : Use Testcontainers to spin up isolated PostgreSQL instances for integration tests
Applied to files:
test/integration/integration_test.gopkg/db/db_session/testcontainer.gopkg/db/db_session/test.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to cmd/hyperfleet/environments/**/*.go : Use environment-based configuration via OCM_ENV to select development, unit_testing, integration_testing, or production settings
Applied to files:
test/integration/integration_test.gocmd/hyperfleet-api/server/logging/formatter_json.gocmd/hyperfleet-api/main.gocmd/hyperfleet-api/environments/framework.gocmd/hyperfleet-api/servecmd/cmd.gotest/helper.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to test/factories/**/*.go : Provide test data factories in test/factories/ for consistent test fixture creation
Applied to files:
test/integration/integration_test.gotest/helper.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/handlers/**/*.go : Write HTTP responses using json.NewEncoder(w) for consistency with request parsing
Applied to files:
cmd/hyperfleet-api/server/logging/formatter_json.gopkg/api/error.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Regenerate Go models from openapi/openapi.yaml using make generate command after specification changes
Applied to files:
Dockerfile.openapi
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{handlers,services}/**/*.go : Retrieve database sessions from context via db.NewContext() rather than creating new GORM connections
Applied to files:
cmd/hyperfleet-api/server/api_server.gopkg/db/migrations.gocmd/hyperfleet-api/migrate/cmd.gocmd/hyperfleet-api/server/healthcheck_server.gopkg/logger/context.gopkg/handlers/openapi.gopkg/db/db_session/testcontainer.gopkg/db/db_session/test.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/db/migrations/**/*.go : Store cluster and node pool specifications as JSONB columns to allow flexible cloud provider configurations
Applied to files:
pkg/db/migrations.gocmd/hyperfleet-api/migrate/cmd.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{db/migrations,dao}/**/*.go : Use polymorphic status tables with owner_type and owner_id columns to support multiple resource types
Applied to files:
pkg/db/migrations.gopkg/db/db_session/testcontainer.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{db/transaction_middleware.go,handlers}/**/*.go : Store request operation IDs in context for tracing and debugging purposes in error logs
Applied to files:
pkg/db/migrations.gocmd/hyperfleet-api/migrate/cmd.gopkg/logger/context.gopkg/db/db_session/test.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/db/migrations/**/*.go : Enforce foreign key relationships between node pools and clusters via owner_references constraint
Applied to files:
pkg/db/migrations.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/db/transaction_middleware.go : Store database sessions in HTTP request context via middleware at request entry point
Applied to files:
cmd/hyperfleet-api/migrate/cmd.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{dao,services}/**/*.go : Use context.Context as the first parameter in all DAO and service method signatures
Applied to files:
cmd/hyperfleet-api/migrate/cmd.gopkg/logger/context.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/handlers/**/*.go : Ensure all HTTP handlers follow the pattern: parse request, call service/DAO, handle errors, send response
Applied to files:
cmd/hyperfleet-api/server/healthcheck_server.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/dao/**/*.go : Query JSONB columns using GORM's native operators for filtering on nested properties
Applied to files:
pkg/db/db_session/test.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/handlers/**/*.go : Implement handlers as methods on handler structs that accept (w http.ResponseWriter, r *http.Request) parameters
Applied to files:
pkg/logger/http.gopkg/api/error.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Use FIPS-compliant crypto with CGO_ENABLED=1 and GOEXPERIMENT=boringcrypto build flags
Applied to files:
test/helper.go
🧬 Code graph analysis (8)
cmd/hyperfleet-api/server/api_server.go (4)
pkg/logger/logger.go (1)
Config(25-30)pkg/client/ocm/client.go (1)
Config(17-24)cmd/hyperfleet-api/server/server.go (1)
Server(11-16)cmd/hyperfleet-api/environments/types.go (1)
Database(36-38)
cmd/hyperfleet-api/environments/framework.go (2)
pkg/logger/logger.go (2)
Initialize(33-108)Config(25-30)cmd/hyperfleet-api/environments/types.go (3)
Database(36-38)Clients(67-69)Services(44-47)
cmd/hyperfleet-api/server/healthcheck_server.go (2)
pkg/logger/logger.go (1)
Config(25-30)cmd/hyperfleet-api/server/server.go (1)
Server(11-16)
cmd/hyperfleet-api/server/metrics_server.go (2)
pkg/logger/logger.go (1)
Config(25-30)cmd/hyperfleet-api/server/server.go (1)
Server(11-16)
pkg/db/db_session/testcontainer.go (1)
pkg/db/migrations.go (1)
Migrate(17-24)
pkg/config/config.go (1)
pkg/config/logging.go (2)
LoggingConfig(10-17)NewLoggingConfig(22-42)
test/helper.go (2)
pkg/logger/logger.go (2)
Initialize(33-108)Config(25-30)cmd/hyperfleet-api/environments/framework.go (1)
Environment(55-57)
pkg/logger/logger.go (2)
pkg/logger/context.go (5)
GetTraceID(29-34)GetSpanID(42-47)GetRequestID(55-60)GetAccountID(66-73)GetTransactionID(76-81)pkg/logger/operationid_middleware.go (1)
GetOperationID(38-43)
🔇 Additional comments (27)
pkg/db/migrations.go (1)
34-35: LGTM! Clean migration to structured logging.The migration from
glog.Fatalftoslog.Errorwith explicitos.Exit(1)is correct and maintains backward compatibility. The structured fields "migrationID" and "error" provide clear context for debugging migration failures.pkg/db/db_session/test.go (1)
54-55: LGTM! Consistent structured logging.Both error logging conversions correctly use
slog.Errorwith the "error" field, maintaining the same control flow as the original implementation.Also applies to: 59-60
pkg/errors/errors.go (1)
116-116: LGTM! Appropriate structured logging for developer errors.The conversion to
slog.Errorwith a "code" field correctly logs undefined error codes, which helps identify programming errors during development.go.mod (1)
67-67: LGTM! Dependency correctly marked as indirect.Moving
github.com/golang/glogto the indirect dependencies section is correct, as it's no longer directly imported following the migration toslog.cmd/hyperfleet-api/migrate/cmd.go (1)
6-7: LGTM - Migration to slog for bootstrap errors.The migration from glog to slog with structured logging is correctly implemented. Using direct slog for these early initialization errors is appropriate since the custom pkg/logger may not yet be initialized at this stage.
Also applies to: 35-36, 41-42
cmd/hyperfleet-api/servecmd/cmd.go (1)
4-5: LGTM - Appropriate slog usage for initialization errors.The migration to slog with structured logging is correctly implemented for these bootstrap error paths. Using direct slog is appropriate for early-stage initialization failures before the custom pkg/logger is initialized.
Also applies to: 22-23, 32-33
test/helper.go (1)
9-9: LGTM - Test helper migration to slog.The migration from glog to slog in the test helper is correctly implemented with appropriate structured logging. Using direct slog in test infrastructure is acceptable.
Also applies to: 79-80, 83-83, 91-92, 142-143, 146-146, 148-148, 162-162, 164-164, 170-171, 178-178, 180-180, 186-186, 189-189, 195-195, 199-199, 209-210, 216-217
cmd/hyperfleet-api/main.go (1)
4-5: LGTM - Appropriate slog usage in main entry point.The migration to slog for the main command error handling is correctly implemented. Using direct slog at the top-level entry point is appropriate for catching command execution failures.
Also applies to: 36-37
pkg/config/config.go (1)
21-21: LGTM! Clean integration of LoggingConfig.The
LoggingConfigintegration follows the established pattern for other configuration components (Server, Metrics, Database, etc.). The initialization, flag registration, and file reading hooks are all properly wired.Also applies to: 31-31, 42-42, 55-55
cmd/hyperfleet-api/environments/framework.go (2)
69-76: Good practice: Logger initialization before first use.The logger is properly initialized early with configuration from
e.Config.Loggingand the application version. This ensures all subsequent logging operations have the correct settings and context.
77-185: Consistent migration to structured logging.All logging calls have been successfully migrated from
glogtoslogwith structured key-value pairs. The error handling pattern ofslog.Errorfollowed byos.Exit(1)is consistent throughout.pkg/db/db_session/testcontainer.go (4)
7-8: LGTM!The new imports correctly support the slog-based structured logging migration and the fatal error handling pattern using
os.Exit(1).
134-154: LGTM!The Close method appropriately uses slog.Error for non-fatal errors during cleanup and slog.Info for lifecycle events. The error handling correctly continues cleanup even when individual steps fail.
156-176: LGTM!The ResetDB method uses structured logging with appropriate context fields ("table", "error") and correctly continues processing remaining tables even when one fails.
178-187: LGTM!The NewListener method appropriately logs errors without terminating the process, allowing the caller to handle the failure gracefully.
Dockerfile.openapi (3)
1-2: LGTM!The switch to
eclipse-temurin:17-jdkprovides multi-arch support while using a well-maintained, official OpenJDK distribution with LTS support.
38-42: LGTM!The OpenAPI generation command correctly invokes the JAR with appropriate parameters. The paths are consistent with the Dockerfile structure.
8-17: Go 1.24.0 is a valid and available release.The architecture detection logic is well-implemented, correctly mapping ARM variants and defaulting to AMD64. Go 1.24.0 is available at https://go.dev/dl/ and the download URLs follow the standard Go naming convention for both amd64 and arm64 architectures.
cmd/hyperfleet-api/server/server.go (1)
4-4: LGTM!The migration from glog to slog is correctly implemented with proper structured logging using the "error" field.
Also applies to: 28-28
cmd/hyperfleet-api/server/metrics_server.go (1)
61-61: LGTM!The non-TLS serving and termination messages use appropriate structured logging with clear, informative messages.
Also applies to: 65-65
pkg/logger/http.go (1)
1-57: Well-designed HTTP logging utilities.The API provides clean, composable structured logging attributes for HTTP requests and responses. The design follows Go conventions and aligns well with the HyperFleet logging specification.
cmd/hyperfleet-api/server/api_server.go (2)
41-44: Authentication logger setup looks good.The migration to
sdk.NewStdLoggerBuilder()with explicit stdout/stderr streams and disabled debug mode is appropriate for the OCM SDK authentication handler.
143-144: Error handling withos.Exit(1)is appropriate for fatal startup failure.Exiting on listener failure is a reasonable approach since the API server cannot function without a network listener.
pkg/logger/logger.go (4)
241-252:Extra()method implementation looks correct.The method creates a new slice, copies existing attributes, appends the new one, and returns a new
legacyLoggerinstance. This immutable pattern is appropriate.This aligns with the coding guidelines: "Use structured logging via pkg/logger/logger.go with Extra() method for adding context fields."
266-269:Fatal()exits immediately without flushing.The
Fatal()method logs and exits, but doesn't flush the logger. For slog, this is generally fine as it writes synchronously, but verify that buffered outputs (if any) are flushed.
94-108: Context handler and global logger initialization looks well-structured.The implementation correctly:
- Wraps the base handler with
contextHandlerfor attribute injection- Adds base attributes (component, version, hostname)
- Sets the global default via
slog.SetDefault()
232-239: V() level mapping collapses granular levels to debug.The
V(level)implementation maps all levels > 1 to debug, collapsing granular verbosity. Current codebase usage (V(10) in db_session and V(Threshold) in logging/writer) relies only on high-level debug messages, so this mapping poses no compatibility issue. Worth documenting if other V() levels are introduced in future migrations.
…ging Implement HyperFleet Logging Specification with Go's slog package: - Add LoggingConfig with LOG_LEVEL, LOG_FORMAT, LOG_OUTPUT env vars - Rewrite pkg/logger to use slog internally (OCMLogger interface preserved) - Add context helpers for trace_id, request_id, account_id correlation - Support JSON format (production) and text format (development) - Include required fields: timestamp, level, message, component, version, hostname Replace all glog imports across 18 files with slog equivalents.
rafabene
force-pushed
the
HYPERFLEET-458-migrate-to-slog
branch
from
f9cff41 to
94e573f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @Dockerfile.openapi:
- Around line 8-17: The Dockerfile installs Go 1.24.0 but your toolchain
requires go1.24.9; update the hardcoded version string in the RUN block so the
wget URL and the filenames used by tar and rm use go1.24.9 instead of go1.24.0
(the logic that sets ARCH/GO_ARCH can stay the same — update only the version in
the wget URL, the tar -xzf argument, and the rm filename).
In @test/helper.go:
- Line 199: Fix the typo in the log message by updating the slog.Info call that
currently reads "Reseting testing environment" to "Resetting testing
environment" so the log uses the correct spelling in the slog.Info(...)
invocation.
🧹 Nitpick comments (5)
cmd/hyperfleet-api/migrate/cmd.go (1)
40-43: Consider adding success logging for operational visibility.The migration command logs errors but doesn't log success. Adding a success message would improve operational visibility and provide confirmation when migrations complete successfully.
🔎 Suggested enhancement
connection := db_session.NewProdFactory(dbConfig) if err := db.Migrate(connection.New(context.Background())); err != nil { slog.Error("Migration failed", "error", err) os.Exit(1) } + slog.Info("Migration completed successfully") }cmd/hyperfleet-api/server/logging/formatter_json.go (1)
19-23: Consider using centralized logging configuration.The
isDebugEnabled()function readsLOG_LEVELfrom the environment on every call without caching. According to the PR summary, logging configuration (level, format, output) is now centralized inpkg/config/logging.go. Consider using the centralized configuration instead of direct environment variable access to ensure consistent log level handling across the codebase.📋 Potential approach
The formatter could accept a reference to the logging config during initialization, or access a global logger configuration instance to check debug status, ensuring consistency with how log levels are determined elsewhere in the application.
Dockerfile.openapi (2)
8-17: Architecture detection may silently default to amd64 for unsupported platforms.The logic maps
arm64/aarch64toarm64and defaults everything else toamd64. If the build runs on an unsupported architecture (e.g., s390x, ppc64le), it will download the amd64 binary which will fail at runtime.🔎 Suggested enhancement for explicit architecture validation
-RUN ARCH=$(dpkg --print-architecture) && \ - if [ "$ARCH" = "arm64" ] || [ "$ARCH" = "aarch64" ]; then \ - GO_ARCH="arm64"; \ - else \ - GO_ARCH="amd64"; \ - fi && \ +RUN ARCH=$(dpkg --print-architecture) && \ + case "$ARCH" in \ + arm64|aarch64) GO_ARCH="arm64" ;; \ + amd64|x86_64) GO_ARCH="amd64" ;; \ + *) echo "Unsupported architecture: $ARCH" && exit 1 ;; \ + esac && \ wget https://go.dev/dl/go1.24.0.linux-${GO_ARCH}.tar.gz && \ tar -C /usr/local -xzf go1.24.0.linux-${GO_ARCH}.tar.gz && \ rm go1.24.0.linux-${GO_ARCH}.tar.gz
19-21: Consider adding checksum verification for the JAR download.Downloading executable artifacts without checksum verification poses a supply chain security risk. Consider verifying the SHA256 checksum of the JAR file after download.
🔎 Example with checksum verification
# Download openapi-generator-cli -RUN wget https://repo1.maven.org/maven2/org/openapitools/openapi-generator-cli/7.18.0/openapi-generator-cli-7.18.0.jar \ - -O /usr/local/bin/openapi-generator-cli.jar +RUN wget https://repo1.maven.org/maven2/org/openapitools/openapi-generator-cli/7.18.0/openapi-generator-cli-7.18.0.jar \ + -O /usr/local/bin/openapi-generator-cli.jar && \ + wget https://repo1.maven.org/maven2/org/openapitools/openapi-generator-cli/7.18.0/openapi-generator-cli-7.18.0.jar.sha1 \ + -O /tmp/openapi-generator-cli.jar.sha1 && \ + echo "$(cat /tmp/openapi-generator-cli.jar.sha1) /usr/local/bin/openapi-generator-cli.jar" | sha1sum -c - && \ + rm /tmp/openapi-generator-cli.jar.sha1Maven Central provides
.sha1files alongside JARs for verification.pkg/logger/context.go (1)
10-21: Consider standardizing context key values.The context key values are inconsistent:
TraceIDKey,SpanIDKey, andRequestIDKeyuse snake_case ("trace_id", "span_id", "request_id"), whileAccountIDKeyuses camelCase ("accountID") andTransactionIDKeyuses an abbreviation ("txid"). Consider standardizing to snake_case ("account_id", "transaction_id") unless the current values are required for backward compatibility.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
Dockerfile.openapicmd/hyperfleet-api/environments/framework.gocmd/hyperfleet-api/main.gocmd/hyperfleet-api/migrate/cmd.gocmd/hyperfleet-api/servecmd/cmd.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/healthcheck_server.gocmd/hyperfleet-api/server/logging/formatter_json.gocmd/hyperfleet-api/server/metrics_server.gocmd/hyperfleet-api/server/server.gogo.modpkg/api/error.gopkg/auth/authz_middleware_mock.gopkg/config/config.gopkg/config/logging.gopkg/db/db_session/test.gopkg/db/db_session/testcontainer.gopkg/db/migrations.gopkg/errors/errors.gopkg/handlers/openapi.gopkg/logger/context.gopkg/logger/http.gopkg/logger/logger.gotest/helper.gotest/integration/integration_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- cmd/hyperfleet-api/server/api_server.go
- pkg/db/migrations.go
- cmd/hyperfleet-api/environments/framework.go
- pkg/db/db_session/test.go
- pkg/errors/errors.go
- cmd/hyperfleet-api/main.go
- cmd/hyperfleet-api/server/metrics_server.go
- cmd/hyperfleet-api/server/healthcheck_server.go
- pkg/auth/authz_middleware_mock.go
- test/integration/integration_test.go
- pkg/handlers/openapi.go
🧰 Additional context used
📓 Path-based instructions (1)
pkg/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use structured logging via pkg/logger/logger.go with Extra() method for adding context fields
Files:
pkg/config/config.gopkg/config/logging.gopkg/logger/context.gopkg/db/db_session/testcontainer.gopkg/logger/http.gopkg/api/error.gopkg/logger/logger.go
🧠 Learnings (21)
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/**/*.go : Use structured logging via pkg/logger/logger.go with Extra() method for adding context fields
Applied to files:
pkg/config/config.gotest/helper.gopkg/config/logging.gocmd/hyperfleet-api/migrate/cmd.gocmd/hyperfleet-api/servecmd/cmd.gopkg/logger/context.gopkg/db/db_session/testcontainer.gocmd/hyperfleet-api/server/logging/formatter_json.gopkg/logger/http.gocmd/hyperfleet-api/server/server.gopkg/api/error.gopkg/logger/logger.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to cmd/hyperfleet/server/**/*.go : Extract user account ID and username from JWT claims for audit logging and authorization checks
Applied to files:
test/helper.gocmd/hyperfleet-api/servecmd/cmd.gopkg/logger/context.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to test/factories/**/*.go : Provide test data factories in test/factories/ for consistent test fixture creation
Applied to files:
test/helper.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Use FIPS-compliant crypto with CGO_ENABLED=1 and GOEXPERIMENT=boringcrypto build flags
Applied to files:
test/helper.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to cmd/hyperfleet/environments/**/*.go : Use environment-based configuration via OCM_ENV to select development, unit_testing, integration_testing, or production settings
Applied to files:
test/helper.gocmd/hyperfleet-api/servecmd/cmd.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/db/migrations/**/*.go : Use GORM AutoMigrate for schema management instead of manual SQL scripts
Applied to files:
cmd/hyperfleet-api/migrate/cmd.gogo.modpkg/db/db_session/testcontainer.gopkg/api/error.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/db/migrations/**/*.go : Store cluster and node pool specifications as JSONB columns to allow flexible cloud provider configurations
Applied to files:
cmd/hyperfleet-api/migrate/cmd.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{handlers,services}/**/*.go : Retrieve database sessions from context via db.NewContext() rather than creating new GORM connections
Applied to files:
cmd/hyperfleet-api/migrate/cmd.gopkg/logger/context.gopkg/db/db_session/testcontainer.gopkg/api/error.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/db/transaction_middleware.go : Store database sessions in HTTP request context via middleware at request entry point
Applied to files:
cmd/hyperfleet-api/migrate/cmd.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{db/transaction_middleware.go,handlers}/**/*.go : Store request operation IDs in context for tracing and debugging purposes in error logs
Applied to files:
cmd/hyperfleet-api/migrate/cmd.gopkg/logger/context.gopkg/db/db_session/testcontainer.gopkg/api/error.gopkg/logger/logger.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{dao,services}/**/*.go : Use context.Context as the first parameter in all DAO and service method signatures
Applied to files:
cmd/hyperfleet-api/migrate/cmd.gopkg/logger/context.gopkg/db/db_session/testcontainer.gopkg/api/error.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to cmd/hyperfleet/server/**/*.go : Validate JWT tokens from Red Hat SSO in middleware before allowing requests to handlers
Applied to files:
cmd/hyperfleet-api/servecmd/cmd.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to test/integration/**/*.go : Use Testcontainers to spin up isolated PostgreSQL instances for integration tests
Applied to files:
pkg/db/db_session/testcontainer.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/handlers/**/*.go : Write HTTP responses using json.NewEncoder(w) for consistency with request parsing
Applied to files:
cmd/hyperfleet-api/server/logging/formatter_json.gopkg/api/error.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/handlers/**/*.go : Implement handlers as methods on handler structs that accept (w http.ResponseWriter, r *http.Request) parameters
Applied to files:
pkg/logger/http.gopkg/api/error.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/handlers/**/*.go : Automatically convert errors to OpenAPI error responses using errors.SendError() in HTTP handlers
Applied to files:
pkg/api/error.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/handlers/**/*.go : Ensure all HTTP handlers follow the pattern: parse request, call service/DAO, handle errors, send response
Applied to files:
pkg/api/error.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/services/**/*.go : Implement service layer between handlers and DAOs for business logic like status aggregation and generation tracking
Applied to files:
pkg/api/error.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/handlers/**/*.go : Set HTTP status codes correctly: 201 Created for POST, 200 OK for GET, 204 No Content for DELETE operations
Applied to files:
pkg/api/error.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Regenerate Go models from openapi/openapi.yaml using make generate command after specification changes
Applied to files:
Dockerfile.openapi
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/api/openapi_embed.go : Embed OpenAPI specification at compile time using //go:embed directive
Applied to files:
Dockerfile.openapi
🧬 Code graph analysis (6)
pkg/config/config.go (1)
pkg/config/logging.go (2)
LoggingConfig(10-17)NewLoggingConfig(22-42)
test/helper.go (1)
pkg/logger/logger.go (2)
Initialize(34-109)Config(26-31)
cmd/hyperfleet-api/migrate/cmd.go (3)
pkg/db/db_session/default.go (1)
NewProdFactory(33-37)pkg/db/migrations.go (1)
Migrate(17-24)pkg/errors/errors.go (1)
New(111-130)
pkg/db/db_session/testcontainer.go (2)
pkg/logger/logger.go (1)
NewOCMLogger(197-222)pkg/db/migrations.go (1)
Migrate(17-24)
pkg/api/error.go (1)
pkg/logger/logger.go (1)
NewOCMLogger(197-222)
pkg/logger/logger.go (2)
pkg/logger/context.go (5)
GetTraceID(29-34)GetSpanID(42-47)GetRequestID(55-60)GetAccountID(71-78)GetTransactionID(86-91)pkg/logger/operationid_middleware.go (1)
GetOperationID(38-43)
🔇 Additional comments (44)
cmd/hyperfleet-api/migrate/cmd.go (2)
6-7: LGTM! Clean migration to standard library logging.The addition of
log/slogandosimports correctly supports the migration from glog to Go's native structured logging. Using standard library packages reduces external dependencies.
35-36: No action required—logging pattern is correct and consistent with codebase standards.The code correctly uses direct
slog.Errorwith structured key-value pairs. All CLI commands in the codebase (main.go, servecmd/cmd.go, etc.) uniformly use directslogfor early initialization errors beforepkg/loggeris initialized in the environment setup. Thepkg/loggerpattern applies to application runtime code (pkg/**/*.go) and request-scoped logging, not CLI initialization.cmd/hyperfleet-api/server/server.go (1)
4-4: LGTM! Clean migration to structured logging.The migration from glog to slog is implemented correctly with proper structured field naming for the error value.
Also applies to: 28-28
go.mod (1)
67-67: LGTM! Correct dependency update.Moving glog from direct to indirect correctly reflects the migration to slog. The library remains available as a transitive dependency for any remaining indirect usages.
cmd/hyperfleet-api/server/logging/formatter_json.go (1)
31-31: LGTM! Debug-gated logging updated correctly.The replacement of
glog.V(10)withisDebugEnabled()maintains the existing behavior of only including sensitive request/response details when debug logging is enabled.Also applies to: 45-45
Dockerfile.openapi (2)
1-2: LGTM! Multi-arch base image selected.The
eclipse-temurin:17-jdkbase image provides excellent multi-arch support for ARM64 and AMD64 builds, resolving the original ARM64 Mac compatibility issue.
38-42: LGTM! OpenAPI generation command updated correctly.The java -jar invocation properly references the downloaded JAR and specifies the correct input/output paths for OpenAPI code generation.
cmd/hyperfleet-api/servecmd/cmd.go (1)
22-23: The learning applies to pkg/ files only, not cmd/ files.The cited learning specifically states "Applies to pkg/**/*.go" and covers structured logging standards within the package directory. The file under review (
cmd/hyperfleet-api/servecmd/cmd.go) is in thecmd/directory and falls outside that scope. Directslogusage is the established pattern throughoutcmd/for initialization and CLI entry points, including command flag setup and error handling at startup.Likely an incorrect or invalid review comment.
pkg/db/db_session/testcontainer.go (7)
7-8: LGTM!The new imports are appropriate:
net/urlfor sanitizing connection strings,osfor process termination, andpkgloggerfor structured logging.Also applies to: 21-21
43-62: LGTM!Logger initialization and container startup follow the structured logging pattern correctly, using
Extra()for context fields.
66-78: Good sanitization of connection string.The URL parsing and selective logging of only
hostanddatabaseeffectively prevents credential leakage. This addresses the security concern from the previous review.
80-109: LGTM!Database connection setup with consistent error handling and structured logging throughout.
111-118: LGTM!Migration logging is clear and follows the structured logging pattern with appropriate error handling.
141-162: LGTM!The
Closefunction appropriately logs errors without terminating the process, which is correct for cleanup operations.
164-198: LGTM!Both
ResetDBandNewListeneruse structured logging correctly. Error handling is appropriate for their non-critical nature (reset and listener setup).pkg/logger/http.go (1)
1-57: LGTM!Well-structured HTTP logging helpers with consistent field naming. The aggregator functions provide a convenient API for common logging patterns, and duration in milliseconds is a good choice for observability.
pkg/logger/context.go (1)
23-60: LGTM!TraceID, SpanID, and RequestID helpers follow a consistent, safe pattern with proper type assertions and sensible defaults.
test/helper.go (5)
9-9: LGTM!Using
log/slogdirectly in test helper code is appropriate. Test utilities don't require the full production logging infrastructure frompkg/logger.
79-92: LGTM!Error handling in
NewHelperfollows a consistent pattern with structured logging and proper process termination on critical failures.
142-149: LGTM!API server lifecycle logging appropriately uses Debug level for start/stop messages and Error level with process termination for critical failures.
159-182: LGTM!Metrics and health check server logging consistently uses Debug level for lifecycle events and Error level for failures.
184-221: LGTM!Server restart and environment reset functions use appropriate log levels and consistent error handling patterns.
pkg/api/error.go (5)
4-4: LGTM!Import of
contextandpkg/loggerenables structured logging with context-aware fields, addressing the past review comment about using the project's logging infrastructure.Also applies to: 11-11
42-42: LGTM!Logging now uses
pkg/loggerwith theExtra()method for structured fields and extracts context from the request, enabling automatic inclusion of trace/span/request IDs. This addresses the past review comment.
62-62: LGTM!Consistent structured logging with context awareness.
72-72: LGTM!Panic handler logging follows the same structured pattern.
98-98: LGTM!Initialization error logging appropriately uses
context.Background()and follows the structured logging pattern with fatal error handling.pkg/config/config.go (1)
21-21: LGTM! Consistent integration of LoggingConfig.The addition of the
Loggingfield toApplicationConfigfollows the established pattern for other configuration components. The integration is complete across constructor, flag registration, and file reading lifecycle methods.Also applies to: 31-31, 42-42, 55-55
pkg/config/logging.go (4)
9-17: LGTM! Clean configuration structure.The
LoggingConfigstruct is well-documented with clear field descriptions for Level, Format, and Output.
22-42: LGTM! Correct precedence implementation.The constructor correctly implements the documented precedence (flags → environment variables → defaults) by:
- Setting defaults first
- Applying environment variable overrides
- Allowing flags (via
AddFlags) to override during parsing
45-49: LGTM! Proper flag registration.The flag defaults use the current field values, which may include environment variable overrides, ensuring correct precedence when flags are parsed.
54-56: LGTM! Intentional no-op with clear rationale.The comment correctly explains that environment variables are read in
NewLoggingConfig()to maintain proper precedence, makingReadFilesunnecessary for this config type.pkg/logger/logger.go (12)
14-23: LGTM! Proper initialization infrastructure.The global variables are well-organized with
sync.Oncefor safe lazy initialization, addressing the race condition from previous reviews.
25-31: LGTM! Clean configuration structure.The
Configstruct provides clear, simple configuration options for the logger.
34-109: LGTM! Comprehensive initialization with HyperFleet standard compliance.The
Initializefunction correctly:
- Configures output and log level with sensible defaults
- Renames
time→timestampandmsg→messageper HyperFleet Logging Specification- Adds required base attributes (component, version, hostname)
- Wraps the handler to inject context-based correlation fields
112-146: LGTM! Correct context-based attribute injection.The
contextHandlerproperly extracts and injects correlation fields (trace_id, span_id, request_id, operation_id) from context without modifying the original context. TheWithAttrsandWithGroupmethods correctly preserve the wrapper.
148-168: LGTM! Race condition resolved with sync.Once.The
Default()function now usessync.Onceto ensure atomic initialization, addressing the race condition flagged in previous reviews.
170-173: LGTM! Convenient helper function.Simple wrapper for adding attributes to the default logger.
176-222: LGTM! Duplicate operation_id issue resolved.The
NewOCMLoggerfunction correctly avoids duplicateoperation_idattributes by relying oncontextHandler.Handleto inject it (as noted in the comment on line 201). Onlyaccount_idandtx_idare added here, which are not handled by the context handler. This addresses the duplication issue from previous reviews.
224-231: LGTM! Correct verbosity level handling.The
V()method returns a new logger instance with the specified verbosity level, maintaining immutability.
233-240: LGTM! Appropriate level mapping.The
Infofmethod correctly maps verbosity levels: V(1) → Info, V(>1) → Debug, aligning with common logging conventions.
242-253: LGTM! Immutable attribute accumulation.The
Extramethod correctly creates a new logger instance with additional attributes, following immutable logger patterns. The pre-allocated capacity (len+2) for the new slice is efficient.As per coding guidelines, this implements the required pattern for adding context fields via structured logging.
255-270: LGTM! Standard logging methods with proper context propagation.All logging methods (
Info,Warning,Error,Fatal) correctly use*Contextmethods to propagate context for attribute extraction. TheFatalmethod appropriately logs at Error level before exiting.
273-278: LGTM! Safe string formatting helper.The
sprintfhelper correctly handles the case when no arguments are provided, avoiding unnecessary formatting.
- Update Go from 1.24.0 to 1.24.9 in Dockerfile.openapi - Fix typo: "Reseting" -> "Resetting" in test/helper.go
| // NewOCMLogger creates a new logger instance with a default verbosity of 1 | ||
| // This maintains backward compatibility with existing code | ||
| func NewOCMLogger(ctx context.Context) OCMLogger { |
There was a problem hiding this comment.
I don't think we need to maintain compatibility at this MVP stage, IMO it would be better to simplify the code
| "strings" | ||
| ) | ||
|
|
||
| func NewJSONLogFormatter() *jsonLogFormatter { |
There was a problem hiding this comment.
Do we need a JSON formatter? doesn't slog come already with support for this?
| authnLogger, err := sdk.NewGlogLoggerBuilder(). | ||
| InfoV(glog.Level(1)). | ||
| DebugV(glog.Level(5)). | ||
| authnLogger, err := sdk.NewStdLoggerBuilder(). |
There was a problem hiding this comment.
I was curios and looked at the definition of this, which says
// NewStdLoggerBuilder has been moved to the logging package.
func NewStdLoggerBuilder() *StdLoggerBuilder {
return logging.NewStdLoggerBuilder()
}
We could update it to call that logging.NewStdLoggerBuilder() directly and not the depcrecated one.
|
|
||
| if l.accountID != "" { | ||
| prefix = fmt.Sprintf("[accountID=%s]%s", l.accountID, prefix) | ||
| type legacyLogger struct { |
There was a problem hiding this comment.
Is this legacyLogger required?
| @@ -1,17 +1,27 @@ | |||
| FROM openapitools/openapi-generator-cli:v7.16.0 | |||
| # Multi-arch base image with Java for openapi-generator | |||
| FROM eclipse-temurin:17-jdk | |||
There was a problem hiding this comment.
I'm curious about the error for arm64 , since I never had an error working on a Macbook M3 and also the image openapitools/openapi-generator-cli:v7.16.0 and is the one used in Sentinel....
was that one also giving you problems?
Either way, it doesn't hurt if this fixes errors, and we may transition to discard completely the Java based generator in favor of a faster go-based one (without containers)
|
|
||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" | ||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" | ||
| pkglogger "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" |
There was a problem hiding this comment.
If I understand this correctly, the pkglogger is needed since there is already an import for gorm.io/gorm/logger
I think it would be better to use logger for the hyperfleet-api/pkg/logger the same as the rest of the application, and change the gorm one
| github.com/go-openapi/jsonpointer v0.21.0 // indirect | ||
| github.com/go-openapi/swag v0.23.0 // indirect | ||
| github.com/go-sql-driver/mysql v1.8.1 // indirect | ||
| github.com/golang/glog v1.2.5 // indirect |
There was a problem hiding this comment.
ouch, still here!
(seems to come from github.com/openshift-online/ocm-sdk-go/logging so nothing to do, just wanted to point this)
|
@rafabene: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Closing this PR as it's duplicated with #29 |
2 participants
Summary
Changes
Logging Migration (HYPERFLEET-458)
pkg/logger/logger.goto use slog with custom contextHandlerpkg/logger/context.gofor trace/span/request ID helperspkg/logger/http.gofor HTTP-specific logging fieldspkg/config/logging.gofor log configuration (level, format, output)Dockerfile Fix (HYPERFLEET-464)
eclipse-temurin:17-jdkTest plan
go build ./...passesgo test ./pkg/...passes (unit tests)LOG_FORMAT=jsonmake generateworks on ARM64 MacSummary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.