From 1bbf05d1901186faf67ac4ecfc08c3164990ff2a Mon Sep 17 00:00:00 2001 From: Dmitrii Andreev Date: Fri, 21 Aug 2026 13:04:37 -0500 Subject: [PATCH] HYPERFLEET-889 - feat: Remove custom Logger wrapper from Adapter --- .tekton/hyperfleet-adapter-chart-tag.yaml | 2 +- .tekton/hyperfleet-adapter-tag.yaml | 2 +- AGENTS.md | 1 + Dockerfile | 2 +- charts/templates/_helpers.tpl | 42 +- cmd/adapter/main.go | 283 +++++----- cmd/adapter/main_test.go | 22 + docs/conventions/logging.md | 66 ++- go.mod | 3 +- go.sum | 6 +- internal/configloader/loader.go | 21 +- internal/configloader/loader_test.go | 66 +-- internal/configloader/validator.go | 14 +- internal/criteria/README.md | 6 +- internal/criteria/cel_evaluator_test.go | 3 +- internal/criteria/evaluator.go | 12 +- internal/criteria/evaluator_scenarios_test.go | 17 +- internal/criteria/evaluator_test.go | 24 +- internal/criteria/evaluator_version_test.go | 9 +- internal/executor/executor.go | 90 ++-- internal/executor/executor_test.go | 79 +-- internal/executor/handler.go | 22 +- internal/executor/param_extractor.go | 15 +- internal/executor/post_action_executor.go | 36 +- .../executor/post_action_executor_test.go | 13 +- internal/executor/precondition_executor.go | 48 +- internal/executor/resource_executor.go | 93 ++-- internal/executor/resource_executor_test.go | 40 +- internal/executor/types.go | 4 - internal/executor/utils.go | 54 +- internal/executor/utils_test.go | 4 +- internal/hyperfleetapi/client.go | 24 +- internal/hyperfleetapi/client_test.go | 53 +- internal/k8sclient/apply.go | 28 +- internal/k8sclient/apply_test.go | 3 - internal/k8sclient/client.go | 17 +- internal/k8sclient/discovery.go | 5 +- internal/logctx/logctx.go | 61 +++ internal/logctx/logctx_test.go | 397 ++++++++++++++ .../logger => internal/logctx}/stack_trace.go | 67 +-- internal/maestroclient/client.go | 31 +- internal/maestroclient/ocm_logger_adapter.go | 58 +- internal/maestroclient/operations.go | 45 +- internal/maestroclient/operations_test.go | 3 +- pkg/health/metrics.go | 13 +- pkg/health/server.go | 14 +- pkg/health/server_test.go | 47 +- pkg/logger/context.go | 227 -------- pkg/logger/logger.go | 303 ----------- pkg/logger/logger_test.go | 455 ---------------- pkg/logger/test_support.go | 83 --- pkg/logger/with_error_field_test.go | 498 ------------------ pkg/telemetry/otel.go | 41 +- pkg/telemetry/otel_test.go | 33 +- .../config_criteria_integration_test.go | 15 +- .../executor/executor_integration_test.go | 93 ++-- .../executor/executor_k8s_integration_test.go | 24 +- test/integration/executor/main_test.go | 5 +- test/integration/executor/setup_test.go | 7 - .../k8sclient/client_integration_test.go | 1 - .../k8sclient/helper_envtest_prebuilt.go | 6 +- test/integration/k8sclient/helper_selector.go | 7 - .../maestroclient/client_integration_test.go | 10 +- .../client_tls_integration_test.go | 19 +- 64 files changed, 1250 insertions(+), 2542 deletions(-) create mode 100644 cmd/adapter/main_test.go create mode 100644 internal/logctx/logctx.go create mode 100644 internal/logctx/logctx_test.go rename {pkg/logger => internal/logctx}/stack_trace.go (62%) delete mode 100644 pkg/logger/context.go delete mode 100644 pkg/logger/logger.go delete mode 100644 pkg/logger/logger_test.go delete mode 100644 pkg/logger/test_support.go delete mode 100644 pkg/logger/with_error_field_test.go diff --git a/.tekton/hyperfleet-adapter-chart-tag.yaml b/.tekton/hyperfleet-adapter-chart-tag.yaml index 569f35ae..a5247ff6 100644 --- a/.tekton/hyperfleet-adapter-chart-tag.yaml +++ b/.tekton/hyperfleet-adapter-chart-tag.yaml @@ -120,7 +120,7 @@ spec: description: Semantic version extracted from git tag ref steps: - name: extract - image: registry.access.redhat.com/ubi9-minimal:latest + image: registry.access.redhat.com/ubi9-minimal:latest@sha256:285fe1836090b985747a93cda2c3c07c0560a1d90a8522ab27877ed2fd5166ee script: | #!/usr/bin/env bash VERSION="${TAG_REF#refs/tags/v}" diff --git a/.tekton/hyperfleet-adapter-tag.yaml b/.tekton/hyperfleet-adapter-tag.yaml index 4e2366aa..c3c365e1 100644 --- a/.tekton/hyperfleet-adapter-tag.yaml +++ b/.tekton/hyperfleet-adapter-tag.yaml @@ -168,7 +168,7 @@ spec: description: Semantic version extracted from git tag ref steps: - name: extract - image: registry.access.redhat.com/ubi9-minimal:latest + image: registry.access.redhat.com/ubi9-minimal:latest@sha256:285fe1836090b985747a93cda2c3c07c0560a1d90a8522ab27877ed2fd5166ee script: | #!/usr/bin/env bash VERSION="${TAG_REF#refs/tags/v}" diff --git a/AGENTS.md b/AGENTS.md index 65322e06..244f7e14 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -89,6 +89,7 @@ IMPORTANT: These return `*ServiceError`, not `error`. Use `.AsError()` to conver - `internal/executor/` — event execution pipeline (params → preconditions → resources → post-actions) - `internal/transportclient/` — unified apply interface abstracting K8s direct and Maestro ManifestWork +- `internal/logctx/` — adapter-specific typed context keys and the stack-trace filter for the shared `hyperfleet-logger` handler (see `docs/conventions/logging.md`) ## Links diff --git a/Dockerfile b/Dockerfile index 5f7e19a6..7e2bb703 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,6 +1,6 @@ ARG BASE_IMAGE=registry.access.redhat.com/ubi9-micro:latest -FROM registry.access.redhat.com/ubi9/go-toolset:9.8-1786971605 AS builder +FROM registry.access.redhat.com/ubi9/go-toolset:9.8-1786971605@sha256:1a9bbbfa854931a97dbff276bd69dc0e32b36cb2fbce3b9813b2cf9892aa8d43 AS builder ARG GIT_SHA=unknown ARG GIT_DIRTY="" diff --git a/charts/templates/_helpers.tpl b/charts/templates/_helpers.tpl index bd8a99b5..196ba2f2 100644 --- a/charts/templates/_helpers.tpl +++ b/charts/templates/_helpers.tpl @@ -284,6 +284,12 @@ Per Helm Chart Conventions Standard section 9 (Deprecation and Migration Pattern {{- end -}} {{- end -}} {{- end -}} +{{- if hasKey .Values "serviceMonitor" }} +{{- fail "serviceMonitor has moved to monitoring.serviceMonitor. Please update your values (e.g. monitoring.serviceMonitor.enabled)." }} +{{- end -}} +{{- if hasKey .Values "tracing" }} +{{- fail "tracing has moved to monitoring.tracing. Please update your values (e.g. monitoring.tracing.enabled)." }} +{{- end -}} {{- end }} {{/* @@ -303,6 +309,27 @@ broker.type must be set explicitly — inference from sub-keys is not supported. {{- required "broker.type must be set to one of: googlepubsub, rabbitmq" .Values.broker.type -}} {{- end }} +{{/* +Convert a validated "" duration string (unit one of s/m/h/d) to seconds. +Callers must validate the format (via regexMatch) before calling this. +*/}} +{{- define "hyperfleet-adapter.durationToSeconds" -}} +{{- $d := . -}} +{{- $length := len $d -}} +{{- $lastIdx := sub $length 1 | int -}} +{{- $unit := substr $lastIdx $length $d -}} +{{- $num := substr 0 $lastIdx $d | int64 -}} +{{- if eq $unit "s" -}} +{{- $num -}} +{{- else if eq $unit "m" -}} +{{- mul $num 60 -}} +{{- else if eq $unit "h" -}} +{{- mul $num 3600 -}} +{{- else if eq $unit "d" -}} +{{- mul $num 86400 -}} +{{- end -}} +{{- end }} + {{/* Validate that required fields are set for the resolved broker type. */}} @@ -314,11 +341,22 @@ Validate that required fields are set for the resolved broker type. {{- if not (regexMatch "^(0|[1-9][0-9]*[smhd])$" $ttl) -}} {{- fail "broker.googlepubsub.expirationTTL must be \"0\" (never expire) or a duration like \"1d\", \"12h\", \"30m\", \"604800s\"" -}} {{- end -}} + {{- if ne $ttl "0" -}} + {{- $ttlSeconds := include "hyperfleet-adapter.durationToSeconds" $ttl | int64 -}} + {{- if lt $ttlSeconds 86400 -}} + {{- fail "broker.googlepubsub.expirationTTL must be \"0\" (never expire) or at least \"1d\" (Google Pub/Sub minimum)" -}} + {{- end -}} + {{- end -}} {{- end -}} {{- if .Values.broker.googlepubsub.messageRetentionDuration -}} - {{- if not (regexMatch "^[1-9][0-9]*[smhd]$" (.Values.broker.googlepubsub.messageRetentionDuration | toString)) -}} + {{- $retention := .Values.broker.googlepubsub.messageRetentionDuration | toString -}} + {{- if not (regexMatch "^[1-9][0-9]*[smhd]$" $retention) -}} {{- fail "broker.googlepubsub.messageRetentionDuration must be a duration like \"1d\", \"12h\", \"30m\", \"604800s\"" -}} {{- end -}} + {{- $retentionSeconds := include "hyperfleet-adapter.durationToSeconds" $retention | int64 -}} + {{- if or (lt $retentionSeconds 600) (gt $retentionSeconds 2678400) -}} + {{- fail "broker.googlepubsub.messageRetentionDuration must be between \"10m\" and \"31d\" (Google Pub/Sub limits)" -}} + {{- end -}} {{- end -}} {{- else if eq $brokerType "rabbitmq" -}} {{- if not .Values.broker.rabbitmq.url -}} @@ -352,4 +390,4 @@ Also validate that all file paths in adapterTaskConfig.files actually exist {{- end }} {{- end }} {{- end }} -{{- end }} \ No newline at end of file +{{- end }} diff --git a/cmd/adapter/main.go b/cmd/adapter/main.go index 8735f038..7f65ffda 100644 --- a/cmd/adapter/main.go +++ b/cmd/adapter/main.go @@ -4,6 +4,7 @@ import ( "context" "flag" "fmt" + "log/slog" "os" "os/signal" "strconv" @@ -16,14 +17,15 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/executor" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/hyperfleetapi" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/k8sclient" + "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/logctx" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/maestroclient" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/transportclient" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/health" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/metrics" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/telemetry" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/version" "github.com/openshift-hyperfleet/hyperfleet-broker/broker" + hfl "github.com/openshift-hyperfleet/hyperfleet-logger" "github.com/spf13/cobra" "github.com/spf13/pflag" sdktrace "go.opentelemetry.io/otel/sdk/trace" @@ -179,67 +181,96 @@ func isDryRun() bool { // Configuration loading (shared between serve and dry-run) // ----------------------------------------------------------------------------- -// buildLoggerConfig creates a logger configuration with the following priority +// buildLogOptions computes the log level/format/output strings with priority // (lowest to highest): config file < LOG_* env vars < --log-* CLI flags. -// Pass logCfg=nil for the bootstrap logger (before config is loaded). -func buildLoggerConfig(component string, logCfg *configloader.LogConfig) logger.Config { - cfg := logger.DefaultConfig() - - // Apply config file values (lowest priority) +// Pass logCfg=nil for the bootstrap handler (before config is loaded). +func buildLogOptions(logCfg *configloader.LogConfig) (level, format, output string) { if logCfg != nil { - if logCfg.Level != "" { - cfg.Level = logCfg.Level - } - if logCfg.Format != "" { - cfg.Format = logCfg.Format - } - if logCfg.Output != "" { - cfg.Output = logCfg.Output - } + level, format, output = logCfg.Level, logCfg.Format, logCfg.Output } // Apply environment variables (override config file) - if level := os.Getenv("LOG_LEVEL"); level != "" { - cfg.Level = strings.ToLower(level) + if v := os.Getenv("LOG_LEVEL"); v != "" { + level = strings.ToLower(v) } - if format := os.Getenv("LOG_FORMAT"); format != "" { - cfg.Format = strings.ToLower(format) + if v := os.Getenv("LOG_FORMAT"); v != "" { + format = strings.ToLower(v) } - if output := os.Getenv("LOG_OUTPUT"); output != "" { - cfg.Output = output + if v := os.Getenv("LOG_OUTPUT"); v != "" { + output = v } // Apply CLI flags (highest priority) if logLevel != "" { - cfg.Level = logLevel + level = logLevel } if logFormat != "" { - cfg.Format = logFormat + format = logFormat } if logOutput != "" { - cfg.Output = logOutput + output = logOutput } - cfg.Component = component - cfg.Version = version.Version + return level, format, output +} - return cfg +// buildDryRunLogOptions applies the normal level and format overrides while +// keeping logs on stderr so stdout remains machine-readable trace output. +func buildDryRunLogOptions() (level, format string) { + level, format, _ = buildLogOptions(&configloader.LogConfig{ + Level: "warn", + Format: "text", + }) + return level, format +} + +// initLogging builds the shared hyperfleet-logger handler and installs it as +// the process-wide slog default. Pass logCfg=nil for the bootstrap handler +// (before config is loaded). +func initLogging(component string, logCfg *configloader.LogConfig) error { + levelStr, formatStr, outputStr := buildLogOptions(logCfg) + return initLoggingWithOptions(component, levelStr, formatStr, outputStr) +} + +// initLoggingWithOptions builds the shared hyperfleet-logger handler and +// installs it as the process-wide slog default using explicit options. +func initLoggingWithOptions(component, levelStr, formatStr, outputStr string) error { + level, err := hfl.ParseLevel(levelStr) + if err != nil { + return fmt.Errorf("invalid log level: %w", err) + } + format, err := hfl.ParseFormat(formatStr) + if err != nil { + return fmt.Errorf("invalid log format: %w", err) + } + output, err := hfl.ParseOutput(outputStr) + if err != nil { + return fmt.Errorf("invalid log output: %w", err) + } + + handler := hfl.NewHandler(component, version.Version, + hfl.WithLevel(level), + hfl.WithFormat(format), + hfl.WithOutput(output), + hfl.WithContextFields(logctx.ContextFields()...), + hfl.WithStackTrace(logctx.StackTraceFilter), + ) + slog.SetDefault(slog.New(handler)) + return nil } // loadConfig loads the unified adapter configuration from both config files. -func loadConfig(ctx context.Context, log logger.Logger, flags *pflag.FlagSet) (*configloader.Config, error) { - log.Info(ctx, "Loading adapter configuration...") +func loadConfig(ctx context.Context, flags *pflag.FlagSet) (*configloader.Config, error) { + slog.InfoContext(ctx, "loading adapter configuration...") config, err := configloader.LoadConfig( configloader.WithAdapterConfigPath(configPath), configloader.WithTaskConfigPath(taskConfigPath), configloader.WithAdapterVersion(version.Version), configloader.WithFlags(flags), configloader.WithContext(ctx), - configloader.WithLogger(log), ) if err != nil { - errCtx := logger.WithErrorField(ctx, err) - log.Errorf(errCtx, "Failed to load adapter configuration") + slog.ErrorContext(ctx, "failed to load adapter configuration", "error", err) return nil, fmt.Errorf("failed to load adapter configuration: %w", err) } return config, nil @@ -250,7 +281,7 @@ func loadConfig(ctx context.Context, log logger.Logger, flags *pflag.FlagSet) (* // ----------------------------------------------------------------------------- // createAPIClient creates a HyperFleet API client from the config -func createAPIClient(apiConfig configloader.HyperfleetAPIConfig, log logger.Logger) (hyperfleetapi.Client, error) { +func createAPIClient(apiConfig configloader.HyperfleetAPIConfig) (hyperfleetapi.Client, error) { var opts []hyperfleetapi.ClientOption // Set base URL if configured (env fallback handled in NewClient) @@ -301,31 +332,30 @@ func createAPIClient(apiConfig configloader.HyperfleetAPIConfig, log logger.Logg opts = append(opts, hyperfleetapi.WithAuth(apiConfig.Auth)) } - return hyperfleetapi.NewClient(log, opts...) + return hyperfleetapi.NewClient(opts...) } // createTransportClient creates the appropriate transport client based on config. func createTransportClient( ctx context.Context, config *configloader.Config, - log logger.Logger, ) (transportclient.TransportClient, error) { if config.Clients.Maestro != nil { - log.Info(ctx, "Creating Maestro transport client...") - client, err := createMaestroClient(ctx, config.Clients.Maestro, log) + slog.InfoContext(ctx, "creating maestro transport client...") + client, err := createMaestroClient(ctx, config.Clients.Maestro) if err != nil { return nil, fmt.Errorf("failed to create Maestro client: %w", err) } - log.Info(ctx, "Maestro transport client created successfully") + slog.InfoContext(ctx, "maestro transport client created successfully") return client, nil } - log.Info(ctx, "Creating Kubernetes transport client...") - client, err := createK8sClient(ctx, config.Clients.Kubernetes, log) + slog.InfoContext(ctx, "creating kubernetes transport client...") + client, err := createK8sClient(ctx, config.Clients.Kubernetes) if err != nil { return nil, fmt.Errorf("failed to create Kubernetes client: %w", err) } - log.Info(ctx, "Kubernetes transport client created successfully") + slog.InfoContext(ctx, "kubernetes transport client created successfully") return client, nil } @@ -333,21 +363,19 @@ func createTransportClient( func createK8sClient( ctx context.Context, k8sConfig configloader.KubernetesConfig, - log logger.Logger, ) (*k8sclient.Client, error) { clientConfig := k8sclient.ClientConfig{ KubeConfigPath: k8sConfig.KubeConfigPath, QPS: k8sConfig.QPS, Burst: k8sConfig.Burst, } - return k8sclient.NewClient(ctx, clientConfig, log) + return k8sclient.NewClient(ctx, clientConfig) } // createMaestroClient creates a Maestro client from the config func createMaestroClient( ctx context.Context, maestroConfig *configloader.MaestroClientConfig, - log logger.Logger, ) (*maestroclient.Client, error) { config := &maestroclient.Config{ MaestroServerAddr: maestroConfig.HTTPServerAddress, @@ -383,7 +411,7 @@ func createMaestroClient( config.HTTPCAFile = maestroConfig.Auth.TLSConfig.HTTPCAFile } - return maestroclient.NewMaestroClient(ctx, config, log) + return maestroclient.NewMaestroClient(ctx, config) } // buildExecutor creates the executor with the given clients. @@ -391,14 +419,12 @@ func buildExecutor( config *configloader.Config, apiClient hyperfleetapi.Client, tc transportclient.TransportClient, - log logger.Logger, metricsRecorder *metrics.Recorder, ) (*executor.Executor, error) { return executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). WithTransportClient(tc). - WithLogger(log). WithMetricsRecorder(metricsRecorder). Build() } @@ -413,40 +439,38 @@ func runServe(flags *pflag.FlagSet) error { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - // Create bootstrap logger (before config is loaded) - log, err := logger.NewLogger(buildLoggerConfig("hyperfleet-adapter", nil)) - if err != nil { - return fmt.Errorf("failed to create logger: %w", err) + // Create bootstrap logging (before config is loaded) + if err := initLogging("hyperfleet-adapter", nil); err != nil { + return fmt.Errorf("failed to initialize logging: %w", err) } - log.Infof(ctx, "Starting Hyperfleet Adapter version=%s commit=%s built=%s", - version.Version, version.Commit, version.BuildDate) + slog.InfoContext(ctx, "starting hyperfleet adapter", + "version", version.Version, "commit", version.Commit, "built", version.BuildDate) // Load unified configuration (deployment + task configs) - config, err := loadConfig(ctx, log, flags) + config, err := loadConfig(ctx, flags) if err != nil { return err } - // Recreate logger with component name and log settings from config - log, err = logger.NewLogger(buildLoggerConfig(config.Adapter.Name, &config.Log)) - if err != nil { - return fmt.Errorf("failed to create logger with adapter config: %w", err) + // Reinitialize logging with component name and log settings from config + if err = initLogging(config.Adapter.Name, &config.Log); err != nil { + return fmt.Errorf("failed to initialize logging with adapter config: %w", err) } - log.Infof(ctx, "Adapter configuration loaded successfully: name=%s ", config.Adapter.Name) - log.Infof(ctx, "HyperFleet API client configured: timeout=%s retry_attempts=%d", - config.Clients.HyperfleetAPI.Timeout.String(), config.Clients.HyperfleetAPI.RetryAttempts) + slog.InfoContext(ctx, "adapter configuration loaded successfully", "name", config.Adapter.Name) + slog.InfoContext(ctx, "hyperfleet api client configured", + "timeout", config.Clients.HyperfleetAPI.Timeout.String(), + "retry_attempts", config.Clients.HyperfleetAPI.RetryAttempts) var redactedConfigBytes []byte if config.DebugConfig { var data []byte data, err = yaml.Marshal(config.Redacted()) if err != nil { - errCtx := logger.WithErrorField(ctx, err) - log.Warnf(errCtx, "Failed to marshal adapter configuration for logging") + slog.WarnContext(ctx, "failed to marshal adapter configuration for logging", "error", err) } else { redactedConfigBytes = data - log.Infof(ctx, "Loaded adapter configuration:\n%s", string(redactedConfigBytes)) + slog.InfoContext(ctx, "loaded adapter configuration", "config", string(redactedConfigBytes)) } } @@ -457,7 +481,7 @@ func runServe(flags *pflag.FlagSet) error { if enabled, err = strconv.ParseBool(tracingEnv); err == nil { tracingEnabled = enabled } else { - log.Warnf(ctx, "Invalid HYPERFLEET_TRACING_ENABLED value %q, defaulting to true", tracingEnv) + slog.WarnContext(ctx, "invalid HYPERFLEET_TRACING_ENABLED value, defaulting to true", "value", tracingEnv) } } @@ -469,33 +493,31 @@ func runServe(flags *pflag.FlagSet) error { } var traceProvider *sdktrace.TracerProvider - traceProvider, err = telemetry.InitTraceProvider(ctx, log, serviceName, version.Version) + traceProvider, err = telemetry.InitTraceProvider(ctx, serviceName, version.Version) if err != nil { - errCtx := logger.WithErrorField(ctx, err) - log.Errorf(errCtx, "Failed to initialize OpenTelemetry") + slog.ErrorContext(ctx, "failed to initialize opentelemetry", "error", err) return fmt.Errorf("failed to initialize OpenTelemetry: %w", err) } tp = traceProvider - log.Infof(ctx, "OpenTelemetry initialized: service_name=%s", serviceName) + slog.InfoContext(ctx, "opentelemetry initialized", "service_name", serviceName) } else { - log.Infof(ctx, "OpenTelemetry tracing disabled") + slog.InfoContext(ctx, "opentelemetry tracing disabled") } defer func() { if tp != nil { shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), OTelShutdownTimeout) defer shutdownCancel() if shutdownErr := tp.Shutdown(shutdownCtx); shutdownErr != nil { - log.Warnf(ctx, "Failed to shutdown OpenTelemetry: %v", shutdownErr) + slog.WarnContext(ctx, "failed to shutdown opentelemetry", "error", shutdownErr) } } }() // Start health server - healthServer := health.NewServer(log, HealthServerPort, config.Adapter.Name) + healthServer := health.NewServer(HealthServerPort, config.Adapter.Name) err = healthServer.Start(ctx) if err != nil { - errCtx := logger.WithErrorField(ctx, err) - log.Errorf(errCtx, "Failed to start health server") + slog.ErrorContext(ctx, "failed to start health server", "error", err) return fmt.Errorf("failed to start health server: %w", err) } healthServer.SetConfigLoaded() @@ -506,29 +528,26 @@ func runServe(flags *pflag.FlagSet) error { shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), HealthServerShutdownTimeout) defer shutdownCancel() if shutdownErr := healthServer.Shutdown(shutdownCtx); shutdownErr != nil { - errCtx := logger.WithErrorField(shutdownCtx, shutdownErr) - log.Warnf(errCtx, "Failed to shutdown health server") + slog.WarnContext(shutdownCtx, "failed to shutdown health server", "error", shutdownErr) } }() // Start metrics server - metricsServer := health.NewMetricsServer(log, MetricsServerPort, health.MetricsConfig{ + metricsServer := health.NewMetricsServer(MetricsServerPort, health.MetricsConfig{ Component: config.Adapter.Name, Version: version.Version, Commit: version.Commit, }) err = metricsServer.Start(ctx) if err != nil { - errCtx := logger.WithErrorField(ctx, err) - log.Errorf(errCtx, "Failed to start metrics server") + slog.ErrorContext(ctx, "failed to start metrics server", "error", err) return fmt.Errorf("failed to start metrics server: %w", err) } defer func() { shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), HealthServerShutdownTimeout) defer shutdownCancel() if shutdownErr := metricsServer.Shutdown(shutdownCtx); shutdownErr != nil { - errCtx := logger.WithErrorField(shutdownCtx, shutdownErr) - log.Warnf(errCtx, "Failed to shutdown metrics server") + slog.WarnContext(shutdownCtx, "failed to shutdown metrics server", "error", shutdownErr) } }() @@ -537,46 +556,43 @@ func runServe(flags *pflag.FlagSet) error { metricsRecorder := metrics.NewRecorder(config.Adapter.Name, version.Version, adapterName, nil) // Create real clients - log.Info(ctx, "Creating HyperFleet API client...") - apiClient, err := createAPIClient(config.Clients.HyperfleetAPI, log) + slog.InfoContext(ctx, "creating hyperfleet api client...") + apiClient, err := createAPIClient(config.Clients.HyperfleetAPI) if err != nil { - errCtx := logger.WithErrorField(ctx, err) - log.Errorf(errCtx, "Failed to create HyperFleet API client") + slog.ErrorContext(ctx, "failed to create hyperfleet api client", "error", err) return fmt.Errorf("failed to create HyperFleet API client: %w", err) } - tc, err := createTransportClient(ctx, config, log) + tc, err := createTransportClient(ctx, config) if err != nil { - errCtx := logger.WithErrorField(ctx, err) - log.Errorf(errCtx, "Failed to create transport client") + slog.ErrorContext(ctx, "failed to create transport client", "error", err) return err } // Build executor - log.Info(ctx, "Creating event executor...") - exec, err := buildExecutor(config, apiClient, tc, log, metricsRecorder) + slog.InfoContext(ctx, "creating event executor...") + exec, err := buildExecutor(config, apiClient, tc, metricsRecorder) if err != nil { - errCtx := logger.WithErrorField(ctx, err) - log.Errorf(errCtx, "Failed to create executor") + slog.ErrorContext(ctx, "failed to create executor", "error", err) return fmt.Errorf("failed to create executor: %w", err) } // Create the event handler and subscribe to broker - handler := executor.AlwaysAck(executor.WithMetrics(exec.CreateHandler(), metricsRecorder, log), log) + handler := executor.AlwaysAck(executor.WithMetrics(exec.CreateHandler(), metricsRecorder)) // Handle signals for graceful shutdown sigCh := make(chan os.Signal, 1) signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) go func() { sig := <-sigCh - log.Infof(ctx, "Received signal %s, initiating graceful shutdown...", sig) - log.Info(ctx, "Shutdown initiated, marking not ready") + slog.InfoContext(ctx, "received signal, initiating graceful shutdown...", "signal", sig.String()) + slog.InfoContext(ctx, "shutdown initiated, marking not ready") healthServer.SetShuttingDown(true) cancel() // Second signal forces immediate exit sig = <-sigCh - log.Infof(ctx, "Received second signal %s, forcing immediate exit", sig) + slog.InfoContext(ctx, "received second signal, forcing immediate exit", "signal", sig.String()) os.Exit(1) }() @@ -584,16 +600,14 @@ func runServe(flags *pflag.FlagSet) error { subscriptionID := config.Clients.Broker.SubscriptionID if subscriptionID == "" { err = fmt.Errorf("clients.broker.subscription_id is required") - errCtx := logger.WithErrorField(ctx, err) - log.Errorf(errCtx, "Missing required broker configuration") + slog.ErrorContext(ctx, "missing required broker configuration", "error", err) return err } topic := config.Clients.Broker.Topic if topic == "" { err = fmt.Errorf("clients.broker.topic is required") - errCtx := logger.WithErrorField(ctx, err) - log.Errorf(errCtx, "Missing required broker configuration") + slog.ErrorContext(ctx, "missing required broker configuration", "error", err) return err } @@ -601,34 +615,31 @@ func runServe(flags *pflag.FlagSet) error { brokerMetrics := broker.NewMetricsRecorder(config.Adapter.Name, version.Version, nil) // Create broker subscriber and subscribe - log.Info(ctx, "Creating broker subscriber...") - subscriber, err := broker.NewSubscriber(log, subscriptionID, brokerMetrics) + slog.InfoContext(ctx, "creating broker subscriber...") + subscriber, err := broker.NewSubscriber(slog.Default(), subscriptionID, brokerMetrics) if err != nil { - errCtx := logger.WithErrorField(ctx, err) - log.Errorf(errCtx, "Failed to create subscriber") + slog.ErrorContext(ctx, "failed to create subscriber", "error", err) return fmt.Errorf("failed to create subscriber: %w", err) } - log.Info(ctx, "Broker subscriber created successfully") + slog.InfoContext(ctx, "broker subscriber created successfully") - log.Info(ctx, "Subscribing to broker topic...") + slog.InfoContext(ctx, "subscribing to broker topic...") err = subscriber.Subscribe(ctx, topic, handler) if err != nil { - errCtx := logger.WithErrorField(ctx, err) - log.Errorf(errCtx, "Failed to subscribe to topic") + slog.ErrorContext(ctx, "failed to subscribe to topic", "error", err) return fmt.Errorf("failed to subscribe to topic: %w", err) } - log.Info(ctx, "Successfully subscribed to broker topic") + slog.InfoContext(ctx, "successfully subscribed to broker topic") // Mark as ready healthServer.SetBrokerReady(true) - log.Info(ctx, "Adapter is ready to process events") + slog.InfoContext(ctx, "adapter is ready to process events") // Monitor subscription errors fatalErrCh := make(chan error, 1) go func() { for subErr := range subscriber.Errors() { - errCtx := logger.WithErrorField(ctx, subErr) - log.Errorf(errCtx, "Subscription error") + slog.ErrorContext(ctx, "subscription error", "error", subErr) select { case fatalErrCh <- subErr: default: @@ -636,21 +647,20 @@ func runServe(flags *pflag.FlagSet) error { } }() - log.Info(ctx, "Adapter started, waiting for events...") + slog.InfoContext(ctx, "adapter started, waiting for events...") // Wait for shutdown signal or fatal subscription error select { case <-ctx.Done(): - log.Info(ctx, "Context canceled, shutting down...") + slog.InfoContext(ctx, "context canceled, shutting down...") case err := <-fatalErrCh: - errCtx := logger.WithErrorField(ctx, err) - log.Errorf(errCtx, "Fatal subscription error, shutting down") + slog.ErrorContext(ctx, "fatal subscription error, shutting down", "error", err) healthServer.SetShuttingDown(true) cancel() } // Close subscriber gracefully - log.Info(ctx, "Closing broker subscriber...") + slog.InfoContext(ctx, "closing broker subscriber...") shutdownCtx, shutdownCancel := context.WithTimeout( context.Background(), 30*time.Second, ) @@ -664,18 +674,16 @@ func runServe(flags *pflag.FlagSet) error { select { case err := <-closeDone: if err != nil { - errCtx := logger.WithErrorField(ctx, err) - log.Errorf(errCtx, "Error closing subscriber") + slog.ErrorContext(ctx, "error closing subscriber", "error", err) } else { - log.Info(ctx, "Subscriber closed successfully") + slog.InfoContext(ctx, "subscriber closed successfully") } case <-shutdownCtx.Done(): err := fmt.Errorf("subscriber close timed out after 30 seconds") - errCtx := logger.WithErrorField(ctx, err) - log.Error(errCtx, "Subscriber close timed out") + slog.ErrorContext(ctx, "subscriber close timed out", "error", err) } - log.Info(ctx, "Adapter shutdown complete") + slog.InfoContext(ctx, "adapter shutdown complete") return nil } @@ -688,19 +696,14 @@ func runServe(flags *pflag.FlagSet) error { func runDryRun(flags *pflag.FlagSet) error { ctx := context.Background() - // Create logger on stderr so stdout is reserved for trace output - log, err := logger.NewLogger(logger.Config{ - Level: "warn", - Format: "text", - Output: "stderr", - Component: "dry-run", - }) - if err != nil { - return fmt.Errorf("failed to create logger: %w", err) + // Log on stderr so stdout is reserved for trace output + level, format := buildDryRunLogOptions() + if err := initLoggingWithOptions("dry-run", level, format, "stderr"); err != nil { + return fmt.Errorf("failed to initialize logging: %w", err) } // Load config (same path as serve) - config, err := loadConfig(ctx, log, flags) + config, err := loadConfig(ctx, flags) if err != nil { return err } @@ -741,7 +744,7 @@ func runDryRun(flags *pflag.FlagSet) error { } // Build executor with mock clients (same builder as serve, no metrics in dry-run) - exec, err := buildExecutor(config, dryrunAPI, dryrunClient, log, nil) + exec, err := buildExecutor(config, dryrunAPI, dryrunClient, nil) if err != nil { return fmt.Errorf("failed to create executor: %w", err) } @@ -787,12 +790,14 @@ func runDryRun(flags *pflag.FlagSet) error { // Sensitive fields are redacted. Exits 0 on success. func runConfigDump(flags *pflag.FlagSet) error { ctx := context.Background() - log, err := logger.NewLogger(buildLoggerConfig("config-dump", nil)) - if err != nil { - return fmt.Errorf("failed to create logger: %w", err) + + // Log on stderr so stdout remains reserved for the machine-readable YAML output + level, format, _ := buildLogOptions(nil) + if err := initLoggingWithOptions("config-dump", level, format, "stderr"); err != nil { + return fmt.Errorf("failed to initialize logging: %w", err) } - config, err := loadConfig(ctx, log, flags) + config, err := loadConfig(ctx, flags) if err != nil { return err } diff --git a/cmd/adapter/main_test.go b/cmd/adapter/main_test.go new file mode 100644 index 00000000..eeb8fac8 --- /dev/null +++ b/cmd/adapter/main_test.go @@ -0,0 +1,22 @@ +package main + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestDryRunLogOptionsDefaults(t *testing.T) { + level, format := buildDryRunLogOptions() + + require.Equal(t, "warn", level) + require.Equal(t, "text", format) +} + +func TestDryRunLogOptionsHonorsLevelOverride(t *testing.T) { + t.Setenv("LOG_LEVEL", "debug") + + level, _ := buildDryRunLogOptions() + + require.Equal(t, "debug", level) +} diff --git a/docs/conventions/logging.md b/docs/conventions/logging.md index 4fd4bf7b..3d217961 100644 --- a/docs/conventions/logging.md +++ b/docs/conventions/logging.md @@ -2,29 +2,71 @@ > **Audience:** Framework developers contributing to the adapter codebase. -Uses `log/slog` wrapped in `pkg/logger`. Every log call takes `context.Context` as first parameter. +Uses stdlib `log/slog` directly, configured via the shared handler from `github.com/openshift-hyperfleet/hyperfleet-logger` (import alias `hfl`). No custom Logger wrapper — every package logs through `slog.Default()` / the package-level `slog.XContext` functions. ## Patterns ```go // Basic -logger.Info(ctx, "message") +slog.InfoContext(ctx, "message") -// Error logging — dominant pattern: attach error to context -errCtx := logger.WithErrorField(ctx, err) -logger.Errorf(errCtx, "Operation failed") +// Error logging — inline attr, no context wrapping +slog.ErrorContext(ctx, "operation failed", "error", err) // Structured fields on context (carried through call chain) -ctx = logger.WithLogField(ctx, "cluster_id", clusterID) +ctx = hfl.Set(ctx, logctx.ManifestWorkKey, name) + +// Built-in fields (registered by hyperfleet-logger itself) +ctx = hfl.WithResourceType(ctx, "Cluster") +ctx = hfl.Set(ctx, hfl.ResourceIDKey, clusterID) ``` -## Additional API +Messages are lowercase. Formatted `%s`/`%v`-style messages become structured `key, value` attrs instead — `fmt.Sprintf("failed for %s: %v", name, err)` becomes `"failed", "name", name, "error", err`. + +## Handler setup + +The handler is built once, at process startup (`cmd/adapter/main.go`'s `initLogging`), and installed via `slog.SetDefault(slog.New(handler))`: + +```go +handler := hfl.NewHandler(component, version.Version, + hfl.WithLevel(level), + hfl.WithFormat(format), + hfl.WithOutput(output), + hfl.WithContextFields(logctx.ContextFields()...), + hfl.WithStackTrace(logctx.StackTraceFilter), +) +``` + +Never construct a second handler mid-request or thread a logger through function parameters — everything reads the process-global default. + +## Context fields + +Adapter-specific typed context keys live in `internal/logctx/` (`hfl.NewKey[T](name)`), registered via `logctx.ContextFields()` at handler construction. Values set with `hfl.Set(ctx, key, val)` are automatically attached to every log record made with that context, at any depth in the call chain — this is how a field set once (e.g. `logctx.ManifestWorkKey` in `CreateManifestWork`) reaches a log call several frames down (`retryOnTransientGRPC`) without being passed explicitly. + +Use a registered `logctx` key only for fields read by more than one log call, or that must propagate across a call chain. A field consumed by exactly one downstream log call is an inline attr instead — don't register a key for it. + +OTel trace/span IDs use the shared `logctx.WithOTelTraceContext(ctx)` helper, which wraps `hfl.WithTraceID`/`hfl.WithSpanID` (already registered as default context fields by `hyperfleet-logger`). + +## Stack traces + +`hfl.WithStackTrace(filter)` attaches a stack trace to a record only when `r.Level >= slog.LevelError` AND the filter returns `true`. The adapter's filter, `logctx.StackTraceFilter`, reproduces the classification `pkg/logger` used to apply: it extracts the `"error"` attr from the record and skips capture for expected operational errors (context cancellation, network errors, expected 4xx/5xx API errors, expected K8s API/resource-data errors), capturing only for unexpected ones. + +## Test support + +```go +// Silence logs in a test +slog.SetDefault(slog.New(slog.DiscardHandler)) + +// Capture and assert on log output +var buf bytes.Buffer +slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))) +t.Cleanup(func() { slog.SetDefault(slog.New(slog.DiscardHandler)) }) +``` -- `logger.WithLogFields(ctx, logger.LogFields{...})` — multiple fields at once -- `logger.With("key", val)` — returns new logger with field (not context-based) -- `logger.Without("key")` — returns new logger with field removed +`slog.SetDefault` is process-global — a test using it to capture output must not run under `t.Parallel()` alongside another test that also touches `slog.Default()`. ## Reference -- Logger interface: `pkg/logger/logger.go` -- Context helpers: `pkg/logger/context.go` +- Handler construction: `cmd/adapter/main.go` (`initLogging`) +- Adapter-specific context keys and stack-trace filter: `internal/logctx/` +- Shared handler package: `github.com/openshift-hyperfleet/hyperfleet-logger` diff --git a/go.mod b/go.mod index 93b1805d..93ae6227 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,8 @@ require ( github.com/go-viper/mapstructure/v2 v2.5.0 github.com/google/cel-go v0.31.0 github.com/mitchellh/copystructure v1.2.0 - github.com/openshift-hyperfleet/hyperfleet-broker v1.1.1 + github.com/openshift-hyperfleet/hyperfleet-broker v1.1.2-0.20260805201321-4ebbef72d0d2 + github.com/openshift-hyperfleet/hyperfleet-logger v0.0.0-20260811173525-c9f9e282d029 github.com/openshift-online/maestro v0.0.0-20260202062555-48b47506a254 github.com/openshift-online/ocm-sdk-go v0.1.509 github.com/prometheus/client_golang v1.24.1 diff --git a/go.sum b/go.sum index 4f8b5050..2e2fcb97 100644 --- a/go.sum +++ b/go.sum @@ -271,8 +271,10 @@ github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8 github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM= github.com/opencontainers/image-spec v1.1.1 h1:y0fUlFfIZhPF1W537XOLg0/fcx6zcHCJwooC2xJA040= github.com/opencontainers/image-spec v1.1.1/go.mod h1:qpqAh3Dmcf36wStyyWU+kCeDgrGnAve2nCC8+7h8Q0M= -github.com/openshift-hyperfleet/hyperfleet-broker v1.1.1 h1:3zbpNuFL+OEvKl6a/KJAlHFcYR4QqQBoGkf35higypU= -github.com/openshift-hyperfleet/hyperfleet-broker v1.1.1/go.mod h1:E7Br4NnsaTTfWR2fEqHAtvFXUAgzFpksF+G5qTBMmy0= +github.com/openshift-hyperfleet/hyperfleet-broker v1.1.2-0.20260805201321-4ebbef72d0d2 h1:c5qyV3DjaBOMaxa/Aq2Ch3i4fh3kAXh2bo4z+EJq+Fs= +github.com/openshift-hyperfleet/hyperfleet-broker v1.1.2-0.20260805201321-4ebbef72d0d2/go.mod h1:KKePw5NRbxNxdUK9KL4Cnhi9YI9TnKJUEkdaggyDRA4= +github.com/openshift-hyperfleet/hyperfleet-logger v0.0.0-20260811173525-c9f9e282d029 h1:c3GdD3EUdR9lRjNot+aBIxVbAXbOSTtiQpWVTHnFHaU= +github.com/openshift-hyperfleet/hyperfleet-logger v0.0.0-20260811173525-c9f9e282d029/go.mod h1:5Nh2IMS2MouehZ4UiRXFMjhuwdys2DX26J4vArmXe6Y= github.com/openshift-online/maestro v0.0.0-20260202062555-48b47506a254 h1:v/jYqdzZpzB/bscVpajlbcKgCNeV4tx4fkm5m2JR8Ug= github.com/openshift-online/maestro v0.0.0-20260202062555-48b47506a254/go.mod h1:cyeif610uObNrbcyn5s1fZg7OWseVjaMAqgrEDA2Aec= github.com/openshift-online/ocm-sdk-go v0.1.509 h1:JcmyaaJQslBqe4vIFugV0rQElGLojJSP/lO11moI/Ho= diff --git a/internal/configloader/loader.go b/internal/configloader/loader.go index e874a737..54d28f2e 100644 --- a/internal/configloader/loader.go +++ b/internal/configloader/loader.go @@ -3,10 +3,10 @@ package configloader import ( "context" "fmt" + "log/slog" "os" "path/filepath" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/utils" "gopkg.in/yaml.v3" ) @@ -34,7 +34,6 @@ type LoadOption func(*loadOptions) type loadOptions struct { flags interface{} // *pflag.FlagSet ctx context.Context - logger logger.Logger adapterConfigPath string taskConfigPath string adapterVersion string @@ -83,13 +82,6 @@ func WithContext(ctx context.Context) LoadOption { } } -// WithLogger sets the logger for config loading -func WithLogger(l logger.Logger) LoadOption { - return func(o *loadOptions) { - o.logger = l - } -} - // ----------------------------------------------------------------------------- // Public API // ----------------------------------------------------------------------------- @@ -105,13 +97,6 @@ func LoadConfig(opts ...LoadOption) (*Config, error) { if o.ctx == nil { o.ctx = context.Background() } - if o.logger == nil { - var logErr error - o.logger, logErr = logger.NewLogger(logger.DefaultConfig()) - if logErr != nil { - return nil, fmt.Errorf("failed to create logger: %w", logErr) - } - } // 1. Load AdapterConfig with Viper (env/CLI overrides) // resolvedAdapterConfigPath is the actual path used (may come from standardConfigPaths fallback) @@ -134,7 +119,7 @@ func LoadConfig(opts ...LoadOption) (*Config, error) { // Validate adapter version if specified if o.adapterVersion != "" { - if err = ValidateAdapterVersion(o.ctx, o.logger, adapterCfg, o.adapterVersion); err != nil { + if err = ValidateAdapterVersion(o.ctx, adapterCfg, o.adapterVersion); err != nil { return nil, fmt.Errorf("adapter version validation failed: %w", err) } } @@ -182,7 +167,7 @@ func LoadConfig(opts ...LoadOption) (*Config, error) { return nil, fmt.Errorf("task config semantic validation failed: %w", err) } for _, w := range taskValidator.Warnings() { - o.logger.Warn(o.ctx, w) + slog.WarnContext(o.ctx, w) } } diff --git a/internal/configloader/loader_test.go b/internal/configloader/loader_test.go index fc5e330b..cb8618ca 100644 --- a/internal/configloader/loader_test.go +++ b/internal/configloader/loader_test.go @@ -3,6 +3,7 @@ package configloader import ( "bytes" "context" + "log/slog" "os" "path/filepath" "testing" @@ -11,8 +12,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/yaml.v3" - - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" ) const testAdapterConfigYAML = ` @@ -526,7 +525,6 @@ func TestGetPreconditionByName(t *testing.T) { func TestValidateAdapterVersion(t *testing.T) { ctx := context.Background() - log := newTestLogger(nil) config := &AdapterConfig{ Adapter: AdapterInfo{ @@ -536,35 +534,35 @@ func TestValidateAdapterVersion(t *testing.T) { } // Exact match - err := ValidateAdapterVersion(ctx, log, config, "1.0.0") + err := ValidateAdapterVersion(ctx, config, "1.0.0") assert.NoError(t, err) // Patch version differs - should pass (bug fix release) - err = ValidateAdapterVersion(ctx, log, config, "1.0.5") + err = ValidateAdapterVersion(ctx, config, "1.0.5") assert.NoError(t, err) // Minor version differs - should fail - err = ValidateAdapterVersion(ctx, log, config, "1.1.0") + err = ValidateAdapterVersion(ctx, config, "1.1.0") assert.Error(t, err) assert.Contains(t, err.Error(), "adapter version mismatch") // Major version differs - should fail - err = ValidateAdapterVersion(ctx, log, config, "2.0.0") + err = ValidateAdapterVersion(ctx, config, "2.0.0") assert.Error(t, err) assert.Contains(t, err.Error(), "adapter version mismatch") // Empty expected version (skip validation) - err = ValidateAdapterVersion(ctx, log, config, "") + err = ValidateAdapterVersion(ctx, config, "") assert.NoError(t, err) // Dev build versions (0.0.0-* skip validation) - err = ValidateAdapterVersion(ctx, log, config, "0.0.0-dev") + err = ValidateAdapterVersion(ctx, config, "0.0.0-dev") assert.NoError(t, err) - err = ValidateAdapterVersion(ctx, log, config, "0.0.0-master") + err = ValidateAdapterVersion(ctx, config, "0.0.0-master") assert.NoError(t, err) - err = ValidateAdapterVersion(ctx, log, config, "v0.0.0-dev") + err = ValidateAdapterVersion(ctx, config, "v0.0.0-dev") assert.NoError(t, err) // Empty config version (not provided in adapter config - skip validation) @@ -573,11 +571,11 @@ func TestValidateAdapterVersion(t *testing.T) { Name: "test-adapter", }, } - err = ValidateAdapterVersion(ctx, log, noVersionConfig, "1.0.0") + err = ValidateAdapterVersion(ctx, noVersionConfig, "1.0.0") assert.NoError(t, err) // Pre-release version with same major.minor - should pass - err = ValidateAdapterVersion(ctx, log, config, "1.0.1-rc.1") + err = ValidateAdapterVersion(ctx, config, "1.0.1-rc.1") assert.NoError(t, err) // Non-semver config version - should warn and skip validation @@ -587,26 +585,25 @@ func TestValidateAdapterVersion(t *testing.T) { Version: "not-a-version", }, } - var buf bytes.Buffer - logWithCapture := newTestLogger(&buf) + buf := captureSlogOutput(t) - err = ValidateAdapterVersion(ctx, logWithCapture, invalidConfig, "1.0.0") + err = ValidateAdapterVersion(ctx, invalidConfig, "1.0.0") assert.NoError(t, err) - assert.Contains(t, buf.String(), "Skipping adapter version validation") + assert.Contains(t, buf.String(), "skipping adapter version validation") assert.Contains(t, buf.String(), "config version is not valid semver") // Non-semver binary version - should warn and skip validation buf.Reset() - err = ValidateAdapterVersion(ctx, logWithCapture, config, "not-a-version") + err = ValidateAdapterVersion(ctx, config, "not-a-version") assert.NoError(t, err) - assert.Contains(t, buf.String(), "Skipping adapter version validation") + assert.Contains(t, buf.String(), "skipping adapter version validation") assert.Contains(t, buf.String(), "binary version is not valid semver") // Non-semver binary version "dev" (Konflux pipeline case) buf.Reset() - err = ValidateAdapterVersion(ctx, logWithCapture, config, "dev") + err = ValidateAdapterVersion(ctx, config, "dev") assert.NoError(t, err) - assert.Contains(t, buf.String(), "Skipping adapter version validation") + assert.Contains(t, buf.String(), "skipping adapter version validation") assert.Contains(t, buf.String(), "binary version is not valid semver") // Non-semver config version with valid binary version @@ -617,27 +614,30 @@ func TestValidateAdapterVersion(t *testing.T) { }, } buf.Reset() - err = ValidateAdapterVersion(ctx, logWithCapture, devConfig, "1.0.0") + err = ValidateAdapterVersion(ctx, devConfig, "1.0.0") assert.NoError(t, err) - assert.Contains(t, buf.String(), "Skipping adapter version validation") + assert.Contains(t, buf.String(), "skipping adapter version validation") assert.Contains(t, buf.String(), "config version is not valid semver") // Both versions non-semver (config parse fails first, so only config warning emitted) buf.Reset() - err = ValidateAdapterVersion(ctx, logWithCapture, devConfig, "latest") + err = ValidateAdapterVersion(ctx, devConfig, "latest") assert.NoError(t, err) - assert.Contains(t, buf.String(), "Skipping adapter version validation") + assert.Contains(t, buf.String(), "skipping adapter version validation") assert.Contains(t, buf.String(), "config version is not valid semver") } -func newTestLogger(buf *bytes.Buffer) logger.Logger { - cfg := logger.DefaultConfig() - cfg.Format = "text" - if buf != nil { - cfg.Writer = buf - } - l, _ := logger.NewLogger(cfg) - return l +// captureSlogOutput installs a text-handler slog logger as the process default +// for the duration of the test and returns the buffer it writes to, so tests +// can assert on log output produced via slog.WarnContext/slog.ErrorContext. +// The previous default logger is restored automatically via t.Cleanup. +func captureSlogOutput(t *testing.T) *bytes.Buffer { + t.Helper() + var buf bytes.Buffer + prev := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&buf, nil))) + t.Cleanup(func() { slog.SetDefault(prev) }) + return &buf } func TestValidateFileReferencesInTaskConfig(t *testing.T) { diff --git a/internal/configloader/validator.go b/internal/configloader/validator.go index 355120fa..13d41187 100644 --- a/internal/configloader/validator.go +++ b/internal/configloader/validator.go @@ -3,6 +3,7 @@ package configloader import ( "context" "fmt" + "log/slog" "os" "path/filepath" "reflect" @@ -14,7 +15,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/criteria" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/manifest" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" ) // templateVarRegex matches Go template variables like {{ .varName }} or {{ .nested.var }} @@ -816,7 +816,7 @@ func isSliceOrArray(value interface{}) bool { // patch version differences are allowed (patch releases are bug fixes only). // For example, config "1.2.0" is compatible with adapter "1.2.3". func ValidateAdapterVersion( - ctx context.Context, log logger.Logger, config *AdapterConfig, expectedVersion string, + ctx context.Context, config *AdapterConfig, expectedVersion string, ) error { if expectedVersion == "" { return nil @@ -829,17 +829,15 @@ func ValidateAdapterVersion( configSemver, err := semver.NewVersion(configVersion) if err != nil { - ctx = logger.WithLogField(ctx, "version", configVersion) - ctx = logger.WithErrorField(ctx, err) - log.Warn(ctx, "Skipping adapter version validation: config version is not valid semver") + slog.WarnContext(ctx, "skipping adapter version validation: config version is not valid semver", + "version", configVersion, "error", err) return nil } expectedSemver, err := semver.NewVersion(expectedVersion) if err != nil { - ctx = logger.WithLogField(ctx, "version", expectedVersion) - ctx = logger.WithErrorField(ctx, err) - log.Warn(ctx, "Skipping adapter version validation: binary version is not valid semver") + slog.WarnContext(ctx, "skipping adapter version validation: binary version is not valid semver", + "version", expectedVersion, "error", err) return nil } diff --git a/internal/criteria/README.md b/internal/criteria/README.md index ecc55f03..5a31f58a 100644 --- a/internal/criteria/README.md +++ b/internal/criteria/README.md @@ -42,7 +42,7 @@ ctx.Set("provider", "aws") ctx.Set("nodeCount", 5) // Create evaluator -evaluator, _ := criteria.NewEvaluator(context.Background(), ctx, log) +evaluator, _ := criteria.NewEvaluator(context.Background(), ctx) // Evaluate a single condition result, err := evaluator.EvaluateCondition( @@ -226,7 +226,7 @@ ctx.Set("cloudProvider", "aws") ctx.Set("vpcId", "vpc-12345") // Evaluate precondition conditions -evaluator, _ := criteria.NewEvaluator(context.Background(), ctx, log) +evaluator, _ := criteria.NewEvaluator(context.Background(), ctx) conditions := make([]criteria.ConditionDef, len(precond.Conditions)) for i, cond := range precond.Conditions { conditions[i] = criteria.ConditionDef{ @@ -256,7 +256,7 @@ The package provides descriptive error messages: ctx := criteria.NewEvaluationContext() ctx.Set("count", "not a number") -evaluator, _ := criteria.NewEvaluator(context.Background(), ctx, log) +evaluator, _ := criteria.NewEvaluator(context.Background(), ctx) result, err := evaluator.EvaluateCondition( "count", criteria.OperatorGreaterThan, diff --git a/internal/criteria/cel_evaluator_test.go b/internal/criteria/cel_evaluator_test.go index 12f38ad1..89130c5d 100644 --- a/internal/criteria/cel_evaluator_test.go +++ b/internal/criteria/cel_evaluator_test.go @@ -7,7 +7,6 @@ import ( "github.com/google/cel-go/common/types" "github.com/google/cel-go/common/types/ref" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -291,7 +290,7 @@ func TestEvaluatorCELIntegration(t *testing.T) { ctx.Set("replicas", 3) ctx.Set("provider", "aws") - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) // Test EvaluateCEL result, err := evaluator.EvaluateCEL(`status == "Ready" && replicas > 1`) diff --git a/internal/criteria/evaluator.go b/internal/criteria/evaluator.go index da0b620c..836af65c 100644 --- a/internal/criteria/evaluator.go +++ b/internal/criteria/evaluator.go @@ -3,11 +3,10 @@ package criteria import ( "context" "fmt" + "log/slog" "reflect" "strings" "sync" - - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" ) // EvaluationResult contains the result of evaluating a condition @@ -39,7 +38,6 @@ type ConditionsResult struct { // Evaluator evaluates criteria against an evaluation context type Evaluator struct { evalCtx *EvaluationContext - log logger.Logger ctx context.Context // Lazily cached CEL evaluator for repeated CEL evaluations @@ -52,19 +50,15 @@ type Evaluator struct { // NewEvaluator creates a new criteria evaluator. // All parameters are required - ctx for logging correlation, evalCtx for CEL data. // Returns an error if any required parameter is nil. -func NewEvaluator(ctx context.Context, evalCtx *EvaluationContext, log logger.Logger) (*Evaluator, error) { +func NewEvaluator(ctx context.Context, evalCtx *EvaluationContext) (*Evaluator, error) { if ctx == nil { return nil, fmt.Errorf("ctx is required for Evaluator") } if evalCtx == nil { return nil, fmt.Errorf("evalCtx is required for Evaluator") } - if log == nil { - return nil, fmt.Errorf("log is required for Evaluator") - } return &Evaluator{ evalCtx: evalCtx, - log: log, ctx: ctx, }, nil } @@ -206,7 +200,7 @@ func (e *Evaluator) ExtractValue(field, expression string) (*ExtractValueResult, // Caller should handle logging based on CELResult.Error // This is NOT a parse error, so we don't return error - caller can use default // Only caught field missing or empty value as warn log - e.log.Warnf(e.ctx, "CEL evaluation failed for %q: %v", expression, celResult.Error) + slog.WarnContext(e.ctx, "cel evaluation failed", "expression", expression, "error", celResult.Error) } result.Value = celResult.Value result.Source = expression diff --git a/internal/criteria/evaluator_scenarios_test.go b/internal/criteria/evaluator_scenarios_test.go index fcbe3a77..b2292810 100644 --- a/internal/criteria/evaluator_scenarios_test.go +++ b/internal/criteria/evaluator_scenarios_test.go @@ -4,7 +4,6 @@ import ( "context" "testing" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -44,7 +43,7 @@ func TestRealWorldScenario(t *testing.T) { ctx.Set("vpcId", "vpc-12345") ctx.Set("nodeCount", 5) - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) // Test precondition conditions from the template @@ -119,7 +118,7 @@ func TestResourceStatusEvaluation(t *testing.T) { ctx.Set("resources", resources) - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) t.Run("namespace is active", func(t *testing.T) { @@ -137,7 +136,7 @@ func TestResourceStatusEvaluation(t *testing.T) { localCtx := NewEvaluationContext() localCtx.Set("replicas", 3) localCtx.Set("readyReplicas", 3) - localEvaluator, err := NewEvaluator(context.Background(), localCtx, logger.NewTestLogger()) + localEvaluator, err := NewEvaluator(context.Background(), localCtx) require.NoError(t, err) result, err := localEvaluator.EvaluateCondition( "replicas", @@ -170,7 +169,7 @@ func TestComplexNestedConditions(t *testing.T) { }, }) - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) t.Run("adapter execution successful", func(t *testing.T) { @@ -211,7 +210,7 @@ func TestMapKeyContainment(t *testing.T) { "owner": "team-a", }) - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) t.Run("map contains key - found", func(t *testing.T) { @@ -275,7 +274,7 @@ func TestContainsOperatorEdgeCases(t *testing.T) { // Nil value ctx.Set("nilField", nil) - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) t.Run("string contains substring - found", func(t *testing.T) { @@ -351,7 +350,7 @@ func TestTerminatingClusterScenario(t *testing.T) { ctx.Set("cloudProvider", "aws") ctx.Set("vpcId", "vpc-12345") - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) t.Run("terminating cluster fails preconditions", func(t *testing.T) { @@ -432,7 +431,7 @@ func TestNodeCountValidation(t *testing.T) { ctx.Set("minNodes", tt.minNodes) ctx.Set("maxNodes", tt.maxNodes) - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) // Check if nodeCount >= minNodes diff --git a/internal/criteria/evaluator_test.go b/internal/criteria/evaluator_test.go index 004e12f5..1e061e6d 100644 --- a/internal/criteria/evaluator_test.go +++ b/internal/criteria/evaluator_test.go @@ -4,7 +4,6 @@ import ( "context" "testing" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -524,7 +523,7 @@ func TestEvaluatorEvaluateCondition(t *testing.T) { "phase": "Active", }) - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) tests := []struct { @@ -612,7 +611,7 @@ func TestEvaluatorEvaluateConditions(t *testing.T) { ctx.Set("cloudProvider", "aws") ctx.Set("vpcId", "vpc-12345") - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) tests := []struct { @@ -902,20 +901,15 @@ func TestEvaluationError(t *testing.T) { func TestNewEvaluatorErrorsWithNilParams(t *testing.T) { t.Run("errors with nil ctx", func(t *testing.T) { //nolint:staticcheck // intentionally testing nil ctx - _, err := NewEvaluator(nil, NewEvaluationContext(), logger.NewTestLogger()) + _, err := NewEvaluator(nil, NewEvaluationContext()) assert.Error(t, err) assert.Contains(t, err.Error(), "ctx is required") }) t.Run("errors with nil evalCtx", func(t *testing.T) { - _, err := NewEvaluator(context.Background(), nil, logger.NewTestLogger()) + _, err := NewEvaluator(context.Background(), nil) assert.Error(t, err) assert.Contains(t, err.Error(), "evalCtx is required") }) - t.Run("errors with nil log", func(t *testing.T) { - _, err := NewEvaluator(context.Background(), NewEvaluationContext(), nil) - assert.Error(t, err) - assert.Contains(t, err.Error(), "log is required") - }) } func TestExtractValue(t *testing.T) { @@ -929,7 +923,7 @@ func TestExtractValue(t *testing.T) { }, }) - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) // Get existing field @@ -955,7 +949,7 @@ func TestEvaluateCondition(t *testing.T) { ctx.Set("replicas", 3) ctx.Set("provider", "aws") - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) // Test equals - matched @@ -992,7 +986,7 @@ func TestEvaluateConditions(t *testing.T) { ctx.Set("replicas", 3) ctx.Set("provider", "aws") - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) // All conditions pass @@ -1061,7 +1055,7 @@ func TestNullHandling(t *testing.T) { }, }) - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) t.Run("access field on null parent returns nil value", func(t *testing.T) { @@ -1092,7 +1086,7 @@ func TestDeepNullPath(t *testing.T) { }, }) - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) // a.b.c is null, so a.b.c.d.e.f should return nil value (not error) diff --git a/internal/criteria/evaluator_version_test.go b/internal/criteria/evaluator_version_test.go index d43da98b..46ede644 100644 --- a/internal/criteria/evaluator_version_test.go +++ b/internal/criteria/evaluator_version_test.go @@ -4,7 +4,6 @@ import ( "context" "testing" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -15,7 +14,7 @@ func TestContextVersionTracking(t *testing.T) { ctx := NewEvaluationContext() ctx.Set("status", "Ready") - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) // First CEL evaluation - creates CEL env with only "status" @@ -51,7 +50,7 @@ func TestSetVariablesFromMapVersionTracking(t *testing.T) { "env": "production", }) - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) // First evaluation @@ -79,7 +78,7 @@ func TestMergeVersionTracking(t *testing.T) { ctx1 := NewEvaluationContext() ctx1.Set("a", 1) - evaluator, err := NewEvaluator(context.Background(), ctx1, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx1) require.NoError(t, err) // First evaluation @@ -147,7 +146,7 @@ func TestNoVersionChangeNoRecreate(t *testing.T) { ctx := NewEvaluationContext() ctx.Set("status", "Ready") - evaluator, err := NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := NewEvaluator(context.Background(), ctx) require.NoError(t, err) // First evaluation diff --git a/internal/executor/executor.go b/internal/executor/executor.go index 2f548b6e..8fa2e2b6 100644 --- a/internal/executor/executor.go +++ b/internal/executor/executor.go @@ -4,17 +4,19 @@ import ( "context" "encoding/json" "fmt" + "log/slog" "reflect" "strings" "github.com/cloudevents/sdk-go/v2/event" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/configloader" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/hyperfleetapi" + "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/logctx" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/transportclient" apierrors "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/errors" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/metrics" pkgotel "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/telemetry" + hfl "github.com/openshift-hyperfleet/hyperfleet-logger" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/trace" ) @@ -33,7 +35,6 @@ func NewExecutor(config *ExecutorConfig) (*Executor, error) { precondExecutor: newPreconditionExecutor(config), resourceExecutor: newResourceExecutor(config), postActionExecutor: newPostActionExecutor(config), - log: config.Logger, }, nil } @@ -48,7 +49,6 @@ func validateExecutorConfig(config *ExecutorConfig) error { requiredFields := []string{ "APIClient", - "Logger", "TransportClient"} for _, field := range requiredFields { @@ -62,7 +62,7 @@ func validateExecutorConfig(config *ExecutorConfig) error { // Execute processes event data according to the adapter configuration // The caller is responsible for: -// - Adding event ID to context for logging correlation using logger.WithEventID() +// - Adding event ID to context for logging correlation using hfl.Set(ctx, logctx.EventIDKey, id) func (e *Executor) Execute(ctx context.Context, data interface{}) *ExecutionResult { // Start OTel span and add trace context to logs ctx, span := e.startTracedExecution(ctx) @@ -72,8 +72,7 @@ func (e *Executor) Execute(ctx context.Context, data interface{}) *ExecutionResu eventData, rawData, err := ParseEventData(data) if err != nil { parseErr := fmt.Errorf("failed to parse event data: %w", err) - errCtx := logger.WithErrorField(ctx, parseErr) - e.log.Errorf(errCtx, "Failed to parse event data") + slog.ErrorContext(ctx, "failed to parse event data", "error", parseErr) return &ExecutionResult{ Status: StatusFailed, CurrentPhase: PhaseParamExtraction, @@ -86,11 +85,12 @@ func (e *Executor) Execute(ctx context.Context, data interface{}) *ExecutionResu // the logger will set the cluster_id=owner_id, nodepool_id=resource_id, resource_type=nodepool // but when a resource is cluster type, it will just record cluster_id=resource_id if eventData.OwnerReferences != nil { - ctx = logger.WithResourceType(ctx, eventData.Kind) - ctx = logger.WithDynamicResourceID(ctx, eventData.Kind, eventData.ID) - ctx = logger.WithDynamicResourceID(ctx, eventData.OwnerReferences.Kind, eventData.OwnerReferences.ID) + ctx = hfl.WithResourceType(ctx, eventData.Kind) + ctx = hfl.WithResourceID(ctx, eventData.ID) + ctx = hfl.Set(ctx, logctx.OwnerResourceTypeKey, eventData.OwnerReferences.Kind) + ctx = hfl.Set(ctx, logctx.OwnerResourceIDKey, eventData.OwnerReferences.ID) } else { - ctx = logger.WithDynamicResourceID(ctx, eventData.Kind, eventData.ID) + ctx = hfl.WithResourceID(ctx, eventData.ID) } execCtx := NewExecutionContext(ctx, rawData, e.config.Config) @@ -103,28 +103,27 @@ func (e *Executor) Execute(ctx context.Context, data interface{}) *ExecutionResu CurrentPhase: PhaseParamExtraction, } - e.log.Info(ctx, "Processing event") + slog.InfoContext(ctx, "processing event") // Phase 1: Parameter Extraction - e.log.Infof(ctx, "Phase %s: RUNNING", result.CurrentPhase) + slog.InfoContext(ctx, "phase running", "phase", result.CurrentPhase) if paramErr := e.executeParamExtraction(execCtx); paramErr != nil { result.Status = StatusFailed result.Errors[PhaseParamExtraction] = paramErr execCtx.SetError("ParameterExtractionFailed", paramErr.Error()) resErr := fmt.Errorf("parameter extraction failed: %w", paramErr) - errCtx := logger.WithErrorField(ctx, resErr) - e.log.Errorf(errCtx, "Phase %s: FAILED", PhaseParamExtraction) + slog.ErrorContext(ctx, "phase failed", "phase", PhaseParamExtraction, "error", resErr) result.ExecutionContext = execCtx result.Params = execCtx.Params return result } result.Params = execCtx.Params - e.log.Debugf(ctx, "Parameter extraction completed: extracted %d params", len(execCtx.Params)) + slog.DebugContext(ctx, "parameter extraction completed", "param_count", len(execCtx.Params)) // Phase 2: Preconditions result.CurrentPhase = PhasePreconditions preconditions := e.config.Config.Preconditions - e.log.Infof(ctx, "Phase %s: RUNNING - %d configured", result.CurrentPhase, len(preconditions)) + slog.InfoContext(ctx, "phase running", "phase", result.CurrentPhase, "configured_count", len(preconditions)) precondOutcome := e.precondExecutor.ExecuteAll(ctx, preconditions, execCtx) result.PreconditionResults = precondOutcome.Results @@ -132,8 +131,8 @@ func (e *Executor) Execute(ctx context.Context, data interface{}) *ExecutionResu case precondOutcome.Error != nil && apierrors.IsResourceNotFoundError(precondOutcome.Error): // Resource no longer exists (e.g. deleted externally, wrong ID in event). // Stop processing gracefully. - e.log.Infof(ctx, "Phase %s: resource not found, stopping processing gracefully", - result.CurrentPhase) + slog.InfoContext(ctx, "phase: resource not found, stopping processing gracefully", + "phase", result.CurrentPhase) result.ResourcesSkipped = true result.SkipReason = ResourceNotFoundReason execCtx.SetSkipped(ResourceNotFoundReason, "") @@ -147,8 +146,7 @@ func (e *Executor) Execute(ctx context.Context, data interface{}) *ExecutionResu precondErr := fmt.Errorf("precondition evaluation failed: error=%w", precondOutcome.Error) result.Errors[result.CurrentPhase] = precondErr execCtx.SetError("PreconditionFailed", precondOutcome.Error.Error()) - errCtx := logger.WithErrorField(ctx, precondOutcome.Error) - e.log.Errorf(errCtx, "Phase %s: FAILED", result.CurrentPhase) + slog.ErrorContext(ctx, "phase failed", "phase", result.CurrentPhase, "error", precondOutcome.Error) result.ResourcesSkipped = true result.SkipReason = "PreconditionFailed" // Set skip metadata on adapter context without overwriting the failed execution status @@ -162,16 +160,16 @@ func (e *Executor) Execute(ctx context.Context, data interface{}) *ExecutionResu result.ResourcesSkipped = true result.SkipReason = precondOutcome.NotMetReason execCtx.SetSkipped("PreconditionNotMet", precondOutcome.NotMetReason) - e.log.Infof(ctx, "Phase %s: SUCCESS - NOT_MET - %s", result.CurrentPhase, precondOutcome.NotMetReason) + slog.InfoContext(ctx, "phase success: not met", "phase", result.CurrentPhase, "reason", precondOutcome.NotMetReason) default: // All preconditions matched - e.log.Infof(ctx, "Phase %s: SUCCESS - MET - %d passed", result.CurrentPhase, len(precondOutcome.Results)) + slog.InfoContext(ctx, "phase success: met", "phase", result.CurrentPhase, "passed_count", len(precondOutcome.Results)) } // Phase 3: Resources (skip if preconditions not met or previous error) result.CurrentPhase = PhaseResources resources := e.config.Config.Resources - e.log.Infof(ctx, "Phase %s: RUNNING - %d configured", result.CurrentPhase, len(resources)) + slog.InfoContext(ctx, "phase running", "phase", result.CurrentPhase, "configured_count", len(resources)) if !result.ResourcesSkipped { resourceResults, resourceErr := e.resourceExecutor.ExecuteAll(ctx, resources, execCtx) result.ResourceResults = resourceResults @@ -181,14 +179,13 @@ func (e *Executor) Execute(ctx context.Context, data interface{}) *ExecutionResu resErr := fmt.Errorf("resource execution failed: %w", resourceErr) result.Errors[result.CurrentPhase] = resErr execCtx.SetError("ResourceFailed", resourceErr.Error()) - errCtx := logger.WithErrorField(ctx, resourceErr) - e.log.Errorf(errCtx, "Phase %s: FAILED", result.CurrentPhase) + slog.ErrorContext(ctx, "phase failed", "phase", result.CurrentPhase, "error", resourceErr) // Continue to post actions for error reporting } else { - e.log.Infof(ctx, "Phase %s: SUCCESS - %d processed", result.CurrentPhase, len(resourceResults)) + slog.InfoContext(ctx, "phase success", "phase", result.CurrentPhase, "processed_count", len(resourceResults)) } } else { - e.log.Infof(ctx, "Phase %s: SKIPPED - %s", result.CurrentPhase, result.SkipReason) + slog.InfoContext(ctx, "phase skipped", "phase", result.CurrentPhase, "reason", result.SkipReason) } // Phase 4: Post Actions (always execute for error reporting) @@ -198,15 +195,15 @@ func (e *Executor) Execute(ctx context.Context, data interface{}) *ExecutionResu if postConfig != nil { postActionCount = len(postConfig.PostActions) } - e.log.Infof(ctx, "Phase %s: RUNNING - %d configured", result.CurrentPhase, postActionCount) + slog.InfoContext(ctx, "phase running", "phase", result.CurrentPhase, "configured_count", postActionCount) postResults, err := e.postActionExecutor.ExecuteAll(ctx, postConfig, execCtx) result.PostActionResults = postResults if err != nil { if apierrors.IsResourceNotFoundError(err) { // Resource no longer exists. Log and continue, don't fail. - e.log.Infof(ctx, "Phase %s: resource not found, skipping remaining post-actions", - result.CurrentPhase) + slog.InfoContext(ctx, "phase: resource not found, skipping remaining post-actions", + "phase", result.CurrentPhase) result.ResourcesSkipped = true // ResourceNotFound takes precedence: the resource no longer exists, // making the original skip reason moot. @@ -229,20 +226,18 @@ func (e *Executor) Execute(ctx context.Context, data interface{}) *ExecutionResu result.Status = StatusFailed postErr := fmt.Errorf("post action execution failed: %w", err) result.Errors[result.CurrentPhase] = postErr - errCtx := logger.WithErrorField(ctx, err) - e.log.Errorf(errCtx, "Phase %s: FAILED", result.CurrentPhase) + slog.ErrorContext(ctx, "phase failed", "phase", result.CurrentPhase, "error", err) } } else { - e.log.Infof(ctx, "Phase %s: SUCCESS - %d executed", result.CurrentPhase, len(postResults)) + slog.InfoContext(ctx, "phase success", "phase", result.CurrentPhase, "executed_count", len(postResults)) } // Finalize result.ExecutionContext = execCtx if result.Status == StatusSuccess { - e.log.Infof(ctx, - "Event execution finished: event_execution_status=success resources_skipped=%t reason=%s", - result.ResourcesSkipped, result.SkipReason) + slog.InfoContext(ctx, "event execution finished", + "execution_status", "success", "resources_skipped", result.ResourcesSkipped, "reason", result.SkipReason) } else { // Combine all errors into a single error for logging var errMsgs []string @@ -250,8 +245,7 @@ func (e *Executor) Execute(ctx context.Context, data interface{}) *ExecutionResu errMsgs = append(errMsgs, fmt.Sprintf("%s: %v", phase, err)) } combinedErr := fmt.Errorf("execution failed: %s", strings.Join(errMsgs, "; ")) - errCtx := logger.WithErrorField(ctx, combinedErr) - e.log.Errorf(errCtx, "Event execution finished: event_execution_status=failed") + slog.ErrorContext(ctx, "event execution finished", "execution_status", "failed", "error", combinedErr) } return result } @@ -274,7 +268,7 @@ func (e *Executor) executeParamExtraction(execCtx *ExecutionContext) error { // config.* param sources resolve against the real (unredacted) config so that // sensitive fields like cert paths can still be explicitly extracted when needed. - return extractConfigParams(execCtx.Ctx, e.config.Config, execCtx, configMap, e.config.APIClient, e.log) + return extractConfigParams(execCtx.Ctx, e.config.Config, execCtx, configMap, e.config.APIClient) } // startTracedExecution creates an OTel span and adds trace context to logs. @@ -289,7 +283,7 @@ func (e *Executor) startTracedExecution(ctx context.Context) (context.Context, t ctx, span := otel.Tracer(componentName).Start(ctx, "Execute") // Add trace_id and span_id to logger context for log correlation - ctx = logger.WithOTelTraceContext(ctx) + ctx = logctx.WithOTelTraceContext(ctx) return ctx, span } @@ -298,7 +292,7 @@ func (e *Executor) startTracedExecution(ctx context.Context) (context.Context, t func (e *Executor) CreateHandler() HandlerFunc { return func(ctx context.Context, evt *event.Event) (*ExecutionResult, error) { // Add event ID to context for logging correlation - ctx = logger.WithEventID(ctx, evt.ID()) + ctx = hfl.Set(ctx, logctx.EventIDKey, evt.ID()) // Extract W3C trace context from CloudEvent extensions (if present) // This enables distributed tracing when upstream services (e.g., Sentinel) @@ -306,13 +300,13 @@ func (e *Executor) CreateHandler() HandlerFunc { ctx = pkgotel.ExtractTraceContextFromCloudEvent(ctx, evt) // Log event metadata - e.log.Infof(ctx, "Event received: id=%s type=%s source=%s time=%s", - evt.ID(), evt.Type(), evt.Source(), evt.Time()) + slog.InfoContext(ctx, "event received", + "event_id", evt.ID(), "event_type", evt.Type(), "event_source", evt.Source(), "event_time", evt.Time()) result := e.Execute(ctx, evt.Data()) - e.log.Infof(ctx, "Event processed: type=%s source=%s time=%s", - evt.Type(), evt.Source(), evt.Time()) + slog.InfoContext(ctx, "event processed", + "event_type", evt.Type(), "event_source", evt.Source(), "event_time", evt.Time()) return result, nil } @@ -394,12 +388,6 @@ func (b *ExecutorBuilder) WithTransportClient(client transportclient.TransportCl return b } -// WithLogger sets the logger -func (b *ExecutorBuilder) WithLogger(log logger.Logger) *ExecutorBuilder { - b.config.Logger = log - return b -} - // WithMetricsRecorder sets the optional Prometheus metrics recorder func (b *ExecutorBuilder) WithMetricsRecorder(recorder *metrics.Recorder) *ExecutorBuilder { b.config.MetricsRecorder = recorder diff --git a/internal/executor/executor_test.go b/internal/executor/executor_test.go index 96d0bbb4..839c7b90 100644 --- a/internal/executor/executor_test.go +++ b/internal/executor/executor_test.go @@ -19,9 +19,10 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/criteria" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/hyperfleetapi" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/k8sclient" + "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/logctx" apierrors "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/errors" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/metrics" + hfl "github.com/openshift-hyperfleet/hyperfleet-logger" ) // newMockAPIClient creates a new mock API client for convenience @@ -91,7 +92,6 @@ func build404TestExecutor(t *testing.T, config *configloader.Config, mockClient WithConfig(config). WithAPIClient(mockClient). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) return exec @@ -113,7 +113,6 @@ func TestNewExecutor(t *testing.T) { name: "missing adapter config", config: &ExecutorConfig{ APIClient: newMockAPIClient(), - Logger: logger.NewTestLogger(), }, expectError: true, }, @@ -121,12 +120,11 @@ func TestNewExecutor(t *testing.T) { name: "missing API client", config: &ExecutorConfig{ Config: &configloader.Config{}, - Logger: logger.NewTestLogger(), }, expectError: true, }, { - name: "missing logger", + name: "missing transport client", config: &ExecutorConfig{ Config: &configloader.Config{}, APIClient: newMockAPIClient(), @@ -139,7 +137,6 @@ func TestNewExecutor(t *testing.T) { Config: &configloader.Config{}, APIClient: newMockAPIClient(), TransportClient: k8sclient.NewMockK8sClient(), - Logger: logger.NewTestLogger(), }, expectError: false, }, @@ -169,7 +166,6 @@ func TestExecutorBuilder(t *testing.T) { WithConfig(config). WithAPIClient(newMockAPIClient()). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) @@ -336,7 +332,6 @@ func TestExecute_ParamExtraction(t *testing.T) { WithConfig(config). WithAPIClient(newMockAPIClient()). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() if err != nil { t.Fatalf("unexpected error creating executor: %v", err) @@ -348,7 +343,7 @@ func TestExecute_ParamExtraction(t *testing.T) { } // Execute with event ID in context - ctx := logger.WithEventID(context.Background(), "test-event-123") + ctx := hfl.Set(context.Background(), logctx.EventIDKey, "test-event-123") result := exec.Execute(ctx, eventData) // Check result @@ -433,7 +428,6 @@ func TestExecute_ParamsAPICallSource(t *testing.T) { WithConfig(config). WithAPIClient(mockClient). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) @@ -566,7 +560,7 @@ func TestParamExtractor(t *testing.T) { // Extract params using pure function configMap, err := configToMap(config) require.NoError(t, err) - err = extractConfigParams(context.Background(), config, execCtx, configMap, nil, logger.NewTestLogger()) + err = extractConfigParams(context.Background(), config, execCtx, configMap, nil) if tt.expectError { assert.Error(t, err) @@ -594,7 +588,7 @@ func runParamExtraction( configMap, err := configToMap(config) require.NoError(t, err) addAdapterParams(config, execCtx, configMap) - err = extractConfigParams(context.Background(), config, execCtx, configMap, mockClient, logger.NewTestLogger()) + err = extractConfigParams(context.Background(), config, execCtx, configMap, mockClient) return execCtx, err } @@ -1013,13 +1007,12 @@ func TestSequentialExecution_Preconditions(t *testing.T) { WithConfig(config). WithAPIClient(newMockAPIClient()). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() if err != nil { t.Fatalf("unexpected error creating executor: %v", err) } - ctx := logger.WithEventID(context.Background(), "test-event-seq") + ctx := hfl.Set(context.Background(), logctx.EventIDKey, "test-event-seq") result := exec.Execute(ctx, map[string]interface{}{}) // Verify number of precondition results @@ -1086,11 +1079,10 @@ func TestPrecondition_CustomCELFunctions(t *testing.T) { WithConfig(config). WithAPIClient(newMockAPIClient()). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err, "failed to create executor") - ctx := logger.WithEventID(context.Background(), "test-custom-cel") + ctx := hfl.Set(context.Background(), logctx.EventIDKey, "test-custom-cel") result := exec.Execute(ctx, map[string]interface{}{}) // Verify precondition executed @@ -1173,13 +1165,12 @@ func TestSequentialExecution_Resources(t *testing.T) { WithConfig(config). WithAPIClient(newMockAPIClient()). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() if err != nil { t.Fatalf("unexpected error creating executor: %v", err) } - ctx := logger.WithEventID(context.Background(), "test-event-resources") + ctx := hfl.Set(context.Background(), logctx.EventIDKey, "test-event-resources") result := exec.Execute(ctx, map[string]interface{}{}) // Verify sequential stop-on-failure: number of results should match expected @@ -1242,13 +1233,12 @@ func TestSequentialExecution_PostActions(t *testing.T) { WithConfig(config). WithAPIClient(mockClient). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() if err != nil { t.Fatalf("unexpected error creating executor: %v", err) } - ctx := logger.WithEventID(context.Background(), "test-event-post") + ctx := hfl.Set(context.Background(), logctx.EventIDKey, "test-event-post") result := exec.Execute(ctx, map[string]interface{}{}) // Verify number of post action results @@ -1310,13 +1300,12 @@ func TestSequentialExecution_SkipReasonCapture(t *testing.T) { WithConfig(config). WithAPIClient(newMockAPIClient()). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() if err != nil { t.Fatalf("unexpected error creating executor: %v", err) } - ctx := logger.WithEventID(context.Background(), "test-event-skip") + ctx := hfl.Set(context.Background(), logctx.EventIDKey, "test-event-skip") result := exec.Execute(ctx, map[string]interface{}{}) // Verify execution status is success (adapter executed successfully) @@ -1373,11 +1362,10 @@ func TestCreateHandler_MetricsRecording(t *testing.T) { WithConfig(config). WithAPIClient(newMockAPIClient()). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) - handler := AlwaysAck(WithMetrics(exec.CreateHandler(), recorder, logger.NewTestLogger()), logger.NewTestLogger()) + handler := AlwaysAck(WithMetrics(exec.CreateHandler(), recorder)) evt := event.New() evt.SetID("test-event-1") @@ -1422,11 +1410,10 @@ func TestCreateHandler_MetricsRecording_Failed(t *testing.T) { WithConfig(config). WithAPIClient(newMockAPIClient()). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) - handler := AlwaysAck(WithMetrics(exec.CreateHandler(), recorder, logger.NewTestLogger()), logger.NewTestLogger()) + handler := AlwaysAck(WithMetrics(exec.CreateHandler(), recorder)) evt := event.New() evt.SetID("test-event-fail") @@ -1461,11 +1448,10 @@ func TestCreateHandler_NilMetricsRecorder(t *testing.T) { WithConfig(config). WithAPIClient(newMockAPIClient()). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) - handler := AlwaysAck(WithMetrics(exec.CreateHandler(), nil, logger.NewTestLogger()), logger.NewTestLogger()) + handler := AlwaysAck(WithMetrics(exec.CreateHandler(), nil)) evt := event.New() evt.SetID("test-event-nil") @@ -1520,7 +1506,7 @@ func TestWithMetrics_RecordsMetrics(t *testing.T) { inner := HandlerFunc(func(_ context.Context, _ *event.Event) (*ExecutionResult, error) { return tt.result, nil }) - handler := WithMetrics(inner, recorder, logger.NewTestLogger()) + handler := WithMetrics(inner, recorder) evt := event.New() evt.SetID("test-metrics-" + tt.name) @@ -1561,7 +1547,7 @@ func TestWithMetrics_HandlerPanicPropagates(t *testing.T) { registry := prometheus.NewRegistry() recorder := metrics.NewRecorder("test-adapter", "v0.1.0", "test", registry) - handler := WithMetrics(inner, recorder, logger.NewTestLogger()) + handler := WithMetrics(inner, recorder) evt := event.New() evt.SetID("test-handler-panic") @@ -1583,7 +1569,7 @@ func TestWithMetrics_MetricsPanicRecovered(t *testing.T) { // new(metrics.Recorder) bypasses the nil receiver guard but has nil internal // fields, causing panic inside recordMetrics panicRecorder := new(metrics.Recorder) - handler := WithMetrics(inner, panicRecorder, logger.NewTestLogger()) + handler := WithMetrics(inner, panicRecorder) evt := event.New() evt.SetID("test-metrics-panic") @@ -1624,7 +1610,7 @@ func TestAlwaysAck_AlwaysReturnsNil(t *testing.T) { return tt.result, tt.err }) - handler := AlwaysAck(inner, logger.NewTestLogger()) + handler := AlwaysAck(inner) evt := event.New() evt.SetID("test-ack") @@ -1679,11 +1665,10 @@ func TestPreconditionAPIFailure_ExecutionStatusRemainsFailed(t *testing.T) { WithConfig(config). WithAPIClient(mockClient). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) - ctx := logger.WithEventID(context.Background(), "test-precond-fail") + ctx := hfl.Set(context.Background(), logctx.EventIDKey, "test-precond-fail") result := exec.Execute(ctx, map[string]interface{}{"id": "cluster-123"}) // Verify overall result status is failed @@ -1793,11 +1778,10 @@ func TestPreconditionCapture_NamedMapVariable(t *testing.T) { WithConfig(config). WithAPIClient(mockClient). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) - ctx := logger.WithEventID(context.Background(), "test-named-map") + ctx := hfl.Set(context.Background(), logctx.EventIDKey, "test-named-map") result := exec.Execute(ctx, map[string]interface{}{}) require.Equal(t, StatusSuccess, result.Status) @@ -1906,11 +1890,10 @@ func TestPreconditionCapture_FieldDefault(t *testing.T) { WithConfig(config). WithAPIClient(mockClient). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) - ctx := logger.WithEventID(context.Background(), "test-field-default") + ctx := hfl.Set(context.Background(), logctx.EventIDKey, "test-field-default") result := exec.Execute(ctx, map[string]interface{}{}) require.Equal(t, StatusSuccess, result.Status) @@ -1993,7 +1976,7 @@ func TestPrecondition404_GracefulStop(t *testing.T) { config := new404PreconditionConfig() exec := build404TestExecutor(t, config, mockClient) - ctx := logger.WithEventID(context.Background(), "test-precond-404") + ctx := hfl.Set(context.Background(), logctx.EventIDKey, "test-precond-404") result := exec.Execute(ctx, map[string]interface{}{"id": "cluster-gone"}) assert.Equal(t, StatusSuccess, result.Status, @@ -2025,7 +2008,7 @@ func TestPostAction404_GracefulHandling(t *testing.T) { config := new404PostActionConfig() exec := build404TestExecutor(t, config, mockClient) - ctx := logger.WithEventID(context.Background(), "test-postaction-404") + ctx := hfl.Set(context.Background(), logctx.EventIDKey, "test-postaction-404") result := exec.Execute(ctx, map[string]interface{}{"id": "cluster-gone"}) // Status should be success; 404 in post-actions is gracefully handled @@ -2057,7 +2040,7 @@ func TestPrecondition404_SkipsPostActions(t *testing.T) { config := new404PostActionConfig() exec := build404TestExecutor(t, config, mockClient) - ctx := logger.WithEventID(context.Background(), "test-precond-404-skips-post") + ctx := hfl.Set(context.Background(), logctx.EventIDKey, "test-precond-404-skips-post") _ = exec.Execute(ctx, map[string]interface{}{"id": "cluster-gone"}) // Only the precondition GET should have been made; no PUT for post-actions @@ -2087,7 +2070,7 @@ func TestPreconditionFail_PostAction404(t *testing.T) { config := new404PostActionConfig() exec := build404TestExecutor(t, config, mockClient) - ctx := logger.WithEventID(context.Background(), "test-precond-fail-post-404") + ctx := hfl.Set(context.Background(), logctx.EventIDKey, "test-precond-fail-post-404") result := exec.Execute(ctx, map[string]interface{}{"id": "cluster-gone"}) // Status stays failed (precondition error is the primary failure) @@ -2118,7 +2101,7 @@ func TestPreconditionBrokenURL404_ReportsError(t *testing.T) { config := new404PreconditionConfig() exec := build404TestExecutor(t, config, mockClient) - ctx := logger.WithEventID(context.Background(), "test-broken-url-404") + ctx := hfl.Set(context.Background(), logctx.EventIDKey, "test-broken-url-404") result := exec.Execute(ctx, map[string]interface{}{"id": "cls-123"}) assert.Equal(t, StatusFailed, result.Status, @@ -2148,7 +2131,7 @@ func TestPostActionBrokenURL404_ReportsError(t *testing.T) { config := new404PostActionConfig() exec := build404TestExecutor(t, config, mockClient) - ctx := logger.WithEventID(context.Background(), "test-post-broken-url-404") + ctx := hfl.Set(context.Background(), logctx.EventIDKey, "test-post-broken-url-404") result := exec.Execute(ctx, map[string]interface{}{"id": "cls-123"}) assert.Equal(t, StatusFailed, result.Status, @@ -2208,7 +2191,6 @@ func TestCELExpression_EnvVariable(t *testing.T) { WithConfig(config). WithAPIClient(newMockAPIClient()). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) @@ -2235,7 +2217,6 @@ func TestCELExpression_EventVariable(t *testing.T) { WithConfig(config). WithAPIClient(newMockAPIClient()). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) @@ -2280,7 +2261,6 @@ func TestCELExpression_EnvInPostActionWhen(t *testing.T) { WithConfig(config). WithAPIClient(mockClient). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) @@ -2321,7 +2301,6 @@ func TestCELExpression_EventInPostActionWhen(t *testing.T) { WithConfig(config). WithAPIClient(mockClient). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) @@ -2353,7 +2332,6 @@ func TestCELExpression_EnvInParamExpression(t *testing.T) { WithConfig(config). WithAPIClient(newMockAPIClient()). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) @@ -2376,7 +2354,6 @@ func TestCELExpression_EventInParamExpression(t *testing.T) { WithConfig(config). WithAPIClient(newMockAPIClient()). WithTransportClient(k8sclient.NewMockK8sClient()). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) @@ -2412,7 +2389,6 @@ func TestGoTemplate_EnvInManifest(t *testing.T) { WithConfig(config). WithAPIClient(newMockAPIClient()). WithTransportClient(mockClient). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) @@ -2449,7 +2425,6 @@ func TestGoTemplate_EventInManifest(t *testing.T) { WithConfig(config). WithAPIClient(newMockAPIClient()). WithTransportClient(mockClient). - WithLogger(logger.NewTestLogger()). Build() require.NoError(t, err) diff --git a/internal/executor/handler.go b/internal/executor/handler.go index a915248b..9ef604b6 100644 --- a/internal/executor/handler.go +++ b/internal/executor/handler.go @@ -2,22 +2,22 @@ package executor import ( "context" + "log/slog" "time" "github.com/cloudevents/sdk-go/v2/event" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/metrics" ) // HandlerFunc is a composable event handler. Build a broker-compatible handler with: // -// handler := AlwaysAck(WithMetrics(exec.CreateHandler(), metricsRecorder, log), log) +// handler := AlwaysAck(WithMetrics(exec.CreateHandler(), metricsRecorder)) type HandlerFunc func(ctx context.Context, evt *event.Event) (*ExecutionResult, error) // WithMetrics wraps a HandlerFunc to record Prometheus metrics after execution. // A panic in metrics recording is recovered to prevent crashing the handler. // If recorder is nil, the handler is returned unwrapped. -func WithMetrics(h HandlerFunc, recorder *metrics.Recorder, log logger.Logger) HandlerFunc { +func WithMetrics(h HandlerFunc, recorder *metrics.Recorder) HandlerFunc { if recorder == nil { return h } @@ -34,7 +34,7 @@ func WithMetrics(h HandlerFunc, recorder *metrics.Recorder, log logger.Logger) H func() { defer func() { if r := recover(); r != nil { - log.Errorf(ctx, "panic in metrics recording (recovered): %v", r) + slog.ErrorContext(ctx, "panic in metrics recording (recovered)", "panic", r) } }() recordMetrics(recorder, resultForMetrics, duration) @@ -47,23 +47,19 @@ func WithMetrics(h HandlerFunc, recorder *metrics.Recorder, log logger.Logger) H // AlwaysAck wraps a HandlerFunc into a broker compatible handler that always returns nil, // preventing infinite retry loops for non-recoverable errors. // Errors are logged at warn level before being discarded. -func AlwaysAck(h HandlerFunc, log logger.Logger) func(ctx context.Context, evt *event.Event) error { +func AlwaysAck(h HandlerFunc) func(ctx context.Context, evt *event.Event) error { return func(ctx context.Context, evt *event.Event) error { result, err := h(ctx, evt) - errCtx := logger.WithLogFields(ctx, logger.LogFields{ - "event_id": evt.ID(), - "event_type": evt.Type(), - }) if err != nil { - errCtx = logger.WithErrorField(errCtx, err) - log.Warn(errCtx, "event handler error (acked)") + slog.WarnContext(ctx, "event handler error (acked)", + "event_id", evt.ID(), "event_type", evt.Type(), "error", err) } else if result != nil && result.Status == StatusFailed { phases := make([]string, 0, len(result.Errors)) for phase := range result.Errors { phases = append(phases, string(phase)) } - errCtx = logger.WithLogField(errCtx, "failed_phases", phases) - log.Warn(errCtx, "event handler failed (acked)") + slog.WarnContext(ctx, "event handler failed (acked)", + "event_id", evt.ID(), "event_type", evt.Type(), "failed_phases", phases) } return nil } diff --git a/internal/executor/param_extractor.go b/internal/executor/param_extractor.go index d5e9819b..ea2f725e 100644 --- a/internal/executor/param_extractor.go +++ b/internal/executor/param_extractor.go @@ -12,7 +12,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/configloader" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/criteria" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/hyperfleetapi" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/utils" ) @@ -23,10 +22,9 @@ func extractConfigParams( execCtx *ExecutionContext, configMap map[string]interface{}, apiClient hyperfleetapi.Client, - log logger.Logger, ) error { for _, param := range config.Params { - value, err := extractParam(ctx, param, execCtx, configMap, apiClient, log) + value, err := extractParam(ctx, param, execCtx, configMap, apiClient) if err != nil { if param.Required { return NewExecutorError(PhaseParamExtraction, param.Name, @@ -78,13 +76,12 @@ func extractParam( execCtx *ExecutionContext, configMap map[string]interface{}, apiClient hyperfleetapi.Client, - log logger.Logger, ) (interface{}, error) { switch { case param.Source.IsAPICall(): - return extractFromAPICall(ctx, param, execCtx, apiClient, log) + return extractFromAPICall(ctx, param, execCtx, apiClient) case param.Source.IsExpression(): - return extractFromCELExpression(ctx, param, execCtx, log) + return extractFromCELExpression(ctx, param, execCtx) case param.Source.IsFile(): return extractFromFile(param) case param.Source.IsString(): @@ -136,13 +133,12 @@ func extractFromAPICall( param configloader.Parameter, execCtx *ExecutionContext, apiClient hyperfleetapi.Client, - log logger.Logger, ) (interface{}, error) { ac := param.Source.APICall if ac == nil { return nil, fmt.Errorf("param %q: api_call source has nil configuration", param.Name) } - resp, renderedURL, err := ExecuteAPICall(ctx, ac, execCtx, apiClient, log) + resp, renderedURL, err := ExecuteAPICall(ctx, ac, execCtx, apiClient) if validationErr := ValidateAPIResponse(resp, err, ac.Method, renderedURL); validationErr != nil { return nil, validationErr } @@ -158,11 +154,10 @@ func extractFromCELExpression( ctx context.Context, param configloader.Parameter, execCtx *ExecutionContext, - log logger.Logger, ) (interface{}, error) { evalCtx := criteria.NewEvaluationContext() evalCtx.SetVariablesFromMap(execCtx.GetCELVariables()) - evaluator, err := criteria.NewEvaluator(ctx, evalCtx, log) + evaluator, err := criteria.NewEvaluator(ctx, evalCtx) if err != nil { return nil, fmt.Errorf("param %q: failed to create CEL evaluator: %w", param.Name, err) } diff --git a/internal/executor/post_action_executor.go b/internal/executor/post_action_executor.go index 638df702..e9d54b51 100644 --- a/internal/executor/post_action_executor.go +++ b/internal/executor/post_action_executor.go @@ -4,19 +4,18 @@ import ( "context" "encoding/json" "fmt" + "log/slog" "text/template/parse" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/configloader" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/criteria" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/hyperfleetapi" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/utils" ) // PostActionExecutor executes post-processing actions type PostActionExecutor struct { apiClient hyperfleetapi.Client - log logger.Logger } // newPostActionExecutor creates a new post-action executor @@ -24,7 +23,6 @@ type PostActionExecutor struct { func newPostActionExecutor(config *ExecutorConfig) *PostActionExecutor { return &PostActionExecutor{ apiClient: config.APIClient, - log: config.Logger, } } @@ -42,12 +40,11 @@ func (pae *PostActionExecutor) ExecuteAll( // Step 1: Build post payloads (like clusterStatusPayload) var skippedPayloads map[string]bool if len(postConfig.Payloads) > 0 { - pae.log.Infof(ctx, "Building %d post payloads", len(postConfig.Payloads)) + slog.InfoContext(ctx, "building post payloads", "payload_count", len(postConfig.Payloads)) var err error skippedPayloads, err = pae.buildPostPayloads(ctx, postConfig.Payloads, execCtx) if err != nil { - errCtx := logger.WithErrorField(ctx, err) - pae.log.Errorf(errCtx, "Failed to build post payloads") + slog.ErrorContext(ctx, "failed to build post payloads", "error", err) execCtx.Adapter.ExecutionError = &ExecutionError{ Phase: string(PhasePostActions), Step: "build_payloads", @@ -58,7 +55,7 @@ func (pae *PostActionExecutor) ExecuteAll( } for _, payload := range postConfig.Payloads { if !skippedPayloads[payload.Name] { - pae.log.Debugf(ctx, "payload[%s] built successfully", payload.Name) + slog.DebugContext(ctx, "payload built successfully", "payload", payload.Name) } } } @@ -70,8 +67,7 @@ func (pae *PostActionExecutor) ExecuteAll( results = append(results, result) if err != nil { - errCtx := logger.WithErrorField(ctx, err) - pae.log.Errorf(errCtx, "PostAction[%s] processed: FAILED", action.Name) + slog.ErrorContext(ctx, "post action processed: failed", "post_action", action.Name, "error", err) // Set ExecutionError for failed post action execCtx.Adapter.ExecutionError = &ExecutionError{ @@ -84,9 +80,9 @@ func (pae *PostActionExecutor) ExecuteAll( return results, err } if result.Skipped { - pae.log.Infof(ctx, "PostAction[%s] processed: SKIPPED - reason=%s", action.Name, result.SkipReason) + slog.InfoContext(ctx, "post action processed: skipped", "post_action", action.Name, "reason", result.SkipReason) } else { - pae.log.Infof(ctx, "PostAction[%s] processed: SUCCESS - status=%s", action.Name, result.Status) + slog.InfoContext(ctx, "post action processed: success", "post_action", action.Name, "status", result.Status) } } @@ -106,7 +102,7 @@ func (pae *PostActionExecutor) buildPostPayloads( evalCtx := criteria.NewEvaluationContext() evalCtx.SetVariablesFromMap(execCtx.GetCELVariables()) - evaluator, err := criteria.NewEvaluator(ctx, evalCtx, pae.log) + evaluator, err := criteria.NewEvaluator(ctx, evalCtx) if err != nil { return nil, fmt.Errorf("failed to create evaluator: %w", err) } @@ -122,7 +118,7 @@ func (pae *PostActionExecutor) buildPostPayloads( return nil, fmt.Errorf("when condition evaluation error for payload '%s': %w", payload.Name, celResult.Error) } if !celResult.Matched { - pae.log.Infof(ctx, "Payload '%s' skipped: when condition is false", payload.Name) + slog.InfoContext(ctx, "payload skipped: when condition is false", "payload", payload.Name) skippedPayloads[payload.Name] = true continue } @@ -224,9 +220,9 @@ func (pae *PostActionExecutor) processValue( // If value is nil (field not found or empty), use default if result.Value == nil { if result.Error != nil && valueDef.Default == nil { - pae.log.Warnf(ctx, "Field '%s' not found in payload: %v", result.Source, result.Error) + slog.WarnContext(ctx, "field not found in payload", "field", result.Source, "error", result.Error) } else if valueDef.Default != nil { - pae.log.Debugf(ctx, "Using default value for '%s': %v", result.Source, valueDef.Default) + slog.DebugContext(ctx, "using default value", "field", result.Source, "default", valueDef.Default) } return valueDef.Default, nil } @@ -278,7 +274,7 @@ func (pae *PostActionExecutor) executePostAction( result.Skipped = true result.Status = StatusSkipped result.SkipReason = fmt.Sprintf("referenced payload '%s' was skipped", payloadName) - pae.log.Infof(ctx, "PostAction[%s] skipped: payload '%s' was not built", action.Name, payloadName) + slog.InfoContext(ctx, "post action skipped: payload not built", "post_action", action.Name, "payload", payloadName) return result, nil } } @@ -288,7 +284,7 @@ func (pae *PostActionExecutor) executePostAction( if action.When != nil { evalCtx := criteria.NewEvaluationContext() evalCtx.SetVariablesFromMap(execCtx.GetCELVariables()) - evaluator, err := criteria.NewEvaluator(ctx, evalCtx, pae.log) + evaluator, err := criteria.NewEvaluator(ctx, evalCtx) if err != nil { execErr := NewExecutorError(PhasePostActions, action.Name, "failed to create evaluator for when condition", err) result.Status = StatusFailed @@ -312,14 +308,14 @@ func (pae *PostActionExecutor) executePostAction( result.Skipped = true result.Status = StatusSkipped result.SkipReason = fmt.Sprintf("when condition evaluated to false: %s", action.When.Expression) - pae.log.Infof(ctx, "PostAction[%s] skipped: when condition is false", action.Name) + slog.InfoContext(ctx, "post action skipped: when condition is false", "post_action", action.Name) return result, nil } } // Execute log action if configured if action.Log != nil { - ExecuteLogAction(ctx, action.Log, execCtx, pae.log) + ExecuteLogAction(ctx, action.Log, execCtx) } // Execute API call if configured @@ -339,7 +335,7 @@ func (pae *PostActionExecutor) executeAPICall( execCtx *ExecutionContext, result *PostActionResult, ) error { - resp, url, err := ExecuteAPICall(ctx, apiCall, execCtx, pae.apiClient, pae.log) + resp, url, err := ExecuteAPICall(ctx, apiCall, execCtx, pae.apiClient) result.APICallMade = true // Capture response details if available (even if err != nil) diff --git a/internal/executor/post_action_executor_test.go b/internal/executor/post_action_executor_test.go index fac3c61b..819b86d4 100644 --- a/internal/executor/post_action_executor_test.go +++ b/internal/executor/post_action_executor_test.go @@ -10,7 +10,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/configloader" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/criteria" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/hyperfleetapi" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -19,7 +18,6 @@ import ( // testPAE creates a PostActionExecutor for tests func testPAE() *PostActionExecutor { return newPostActionExecutor(&ExecutorConfig{ - Logger: logger.NewTestLogger(), APIClient: newMockAPIClient(), }) } @@ -97,7 +95,7 @@ func TestBuildPayload(t *testing.T) { for k, v := range tt.params { evalCtx.Set(k, v) } - evaluator, err := criteria.NewEvaluator(context.Background(), evalCtx, pae.log) + evaluator, err := criteria.NewEvaluator(context.Background(), evalCtx) assert.NoError(t, err) result, err := pae.buildPayload(context.Background(), tt.build, evaluator, tt.params) @@ -177,7 +175,7 @@ func TestBuildMapPayload(t *testing.T) { for k, v := range tt.params { evalCtx.Set(k, v) } - evaluator, err := criteria.NewEvaluator(context.Background(), evalCtx, pae.log) + evaluator, err := criteria.NewEvaluator(context.Background(), evalCtx) require.NoError(t, err) result, err := pae.buildMapPayload(context.Background(), tt.input, evaluator, tt.params) @@ -287,7 +285,7 @@ func TestProcessValue(t *testing.T) { for k, v := range tt.evalCtxData { evalCtx.Set(k, v) } - evaluator, err := criteria.NewEvaluator(context.Background(), evalCtx, pae.log) + evaluator, err := criteria.NewEvaluator(context.Background(), evalCtx) require.NoError(t, err) result, err := pae.processValue(context.Background(), tt.value, evaluator, tt.params) @@ -392,7 +390,6 @@ func TestPostActionExecutor_ExecuteAll(t *testing.T) { pae := newPostActionExecutor(&ExecutorConfig{ APIClient: mockClient, - Logger: logger.NewTestLogger(), }) evt := event.New() @@ -635,7 +632,6 @@ func TestExecuteAPICall(t *testing.T) { tt.apiCall, execCtx, mockClient, - logger.NewTestLogger(), ) if tt.expectError { @@ -711,7 +707,6 @@ func TestPostActionWhenCondition(t *testing.T) { pae := newPostActionExecutor(&ExecutorConfig{ APIClient: mockClient, - Logger: logger.NewTestLogger(), }) action := configloader.PostAction{ @@ -828,7 +823,6 @@ func TestPostActionSkippedWhenReferencedPayloadSkipped(t *testing.T) { pae := newPostActionExecutor(&ExecutorConfig{ APIClient: mockClient, - Logger: logger.NewTestLogger(), }) action := configloader.PostAction{ @@ -864,7 +858,6 @@ func TestPostActionNotSkippedWhenPayloadBuilt(t *testing.T) { pae := newPostActionExecutor(&ExecutorConfig{ APIClient: mockClient, - Logger: logger.NewTestLogger(), }) action := configloader.PostAction{ diff --git a/internal/executor/precondition_executor.go b/internal/executor/precondition_executor.go index 6757a8b0..dfe9e74d 100644 --- a/internal/executor/precondition_executor.go +++ b/internal/executor/precondition_executor.go @@ -4,18 +4,17 @@ import ( "context" "encoding/json" "fmt" + "log/slog" "strings" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/configloader" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/criteria" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/hyperfleetapi" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" ) // PreconditionExecutor evaluates preconditions type PreconditionExecutor struct { apiClient hyperfleetapi.Client - log logger.Logger } // newPreconditionExecutor creates a new precondition executor @@ -23,7 +22,6 @@ type PreconditionExecutor struct { func newPreconditionExecutor(config *ExecutorConfig) *PreconditionExecutor { return &PreconditionExecutor{ apiClient: config.APIClient, - log: config.Logger, } } @@ -42,8 +40,7 @@ func (pe *PreconditionExecutor) ExecuteAll( if err != nil { // Execution error (API call failed, parse error, etc.) - errCtx := logger.WithErrorField(ctx, err) - pe.log.Errorf(errCtx, "Precondition[%s] evaluated: FAILED", precond.Name) + slog.ErrorContext(ctx, "precondition evaluated: failed", "precondition", precond.Name, "error", err) return &PreconditionsOutcome{ AllMatched: false, Results: results, @@ -53,7 +50,8 @@ func (pe *PreconditionExecutor) ExecuteAll( if !result.Matched { // Business outcome: precondition not satisfied - pe.log.Infof(ctx, "Precondition[%s] evaluated: NOT_MET - %s", precond.Name, formatConditionDetails(result)) + slog.InfoContext(ctx, "precondition evaluated: not met", + "precondition", precond.Name, "details", formatConditionDetails(result)) return &PreconditionsOutcome{ AllMatched: false, Results: results, @@ -62,7 +60,7 @@ func (pe *PreconditionExecutor) ExecuteAll( } } - pe.log.Infof(ctx, "Precondition[%s] evaluated: MET", precond.Name) + slog.InfoContext(ctx, "precondition evaluated: met", "precondition", precond.Name) } // All preconditions matched @@ -87,7 +85,7 @@ func (pe *PreconditionExecutor) executePrecondition( // Step 1: Execute log action if configured if precond.Log != nil { - ExecuteLogAction(ctx, precond.Log, execCtx, pe.log) + ExecuteLogAction(ctx, precond.Log, execCtx) } // Step 2: Make API call if configured @@ -131,7 +129,7 @@ func (pe *PreconditionExecutor) executePrecondition( // Capture fields from response if len(precond.Capture) > 0 { - pe.log.Debugf(ctx, "Capturing %d fields from API response", len(precond.Capture)) + slog.DebugContext(ctx, "capturing fields from api response", "field_count", len(precond.Capture)) // Create evaluator with response data only. // Both field (JSONPath) and expression (CEL) work on the same source. @@ -143,9 +141,9 @@ func (pe *PreconditionExecutor) executePrecondition( // has(checkClusterState.deleted_time) captureCtx.Set(precond.Name, responseData) - captureEvaluator, evalErr := criteria.NewEvaluator(ctx, captureCtx, pe.log) + captureEvaluator, evalErr := criteria.NewEvaluator(ctx, captureCtx) if evalErr != nil { - pe.log.Warnf(ctx, "Failed to create capture evaluator: %v", evalErr) + slog.WarnContext(ctx, "failed to create capture evaluator", "error", evalErr) } else { for _, capture := range precond.Capture { extractResult, err := captureEvaluator.ExtractValue(capture.Field, capture.Expression) @@ -161,16 +159,18 @@ func (pe *PreconditionExecutor) executePrecondition( // Expression captures are unaffected — their errors surface as-is. if capture.Field != "" && extractResult.Error != nil { if capture.Default != nil { - pe.log.Debugf(ctx, "Field '%s' absent from response, using default: %v", capture.Name, capture.Default) + slog.DebugContext(ctx, "field absent from response, using default", + "field", capture.Name, "default", capture.Default) value = capture.Default } else { - pe.log.Warnf(ctx, "Failed to capture '%s': %v", capture.Name, extractResult.Error) + slog.WarnContext(ctx, "failed to capture field", "field", capture.Name, "error", extractResult.Error) } } result.CapturedFields[capture.Name] = value execCtx.Params[capture.Name] = value - pe.log.Debugf(ctx, "Captured %s = %v (from %s)", capture.Name, value, extractResult.Source) + slog.DebugContext(ctx, "captured field", + "field", capture.Name, "value", value, "source", extractResult.Source) } } } @@ -182,7 +182,7 @@ func (pe *PreconditionExecutor) executePrecondition( evalCtx := criteria.NewEvaluationContext() evalCtx.SetVariablesFromMap(execCtx.GetCELVariables()) - evaluator, err := criteria.NewEvaluator(ctx, evalCtx, pe.log) + evaluator, err := criteria.NewEvaluator(ctx, evalCtx) if err != nil { result.Status = StatusFailed result.Error = err @@ -192,7 +192,7 @@ func (pe *PreconditionExecutor) executePrecondition( // Evaluate using structured conditions or CEL expression switch { case len(precond.Conditions) > 0: - pe.log.Debugf(ctx, "Evaluating %d structured conditions", len(precond.Conditions)) + slog.DebugContext(ctx, "evaluating structured conditions", "condition_count", len(precond.Conditions)) condDefs := ToConditionDefs(precond.Conditions) condResult, err := evaluator.EvaluateConditions(condDefs) @@ -207,11 +207,9 @@ func (pe *PreconditionExecutor) executePrecondition( // Log individual condition results for _, cr := range condResult.Results { - if cr.Matched { - pe.log.Debugf(ctx, "Condition: %s %s %v = %v (matched)", cr.Field, cr.Operator, cr.ExpectedValue, cr.FieldValue) - } else { - pe.log.Debugf(ctx, "Condition: %s %s %v = %v (not matched)", cr.Field, cr.Operator, cr.ExpectedValue, cr.FieldValue) - } + slog.DebugContext(ctx, "condition evaluated", + "field", cr.Field, "operator", cr.Operator, "expected", cr.ExpectedValue, + "actual", cr.FieldValue, "matched", cr.Matched) } // Record evaluation in execution context - reuse criteria.EvaluationResult directly @@ -222,7 +220,7 @@ func (pe *PreconditionExecutor) executePrecondition( execCtx.AddConditionsEvaluation(PhasePreconditions, precond.Name, condResult.Matched, fieldResults) case precond.Expression != "": // Evaluate CEL expression - pe.log.Debugf(ctx, "Evaluating CEL expression: %s", strings.TrimSpace(precond.Expression)) + slog.DebugContext(ctx, "evaluating cel expression", "expression", strings.TrimSpace(precond.Expression)) celResult, err := evaluator.EvaluateCEL(strings.TrimSpace(precond.Expression)) if err != nil { result.Status = StatusFailed @@ -232,13 +230,13 @@ func (pe *PreconditionExecutor) executePrecondition( result.Matched = celResult.Matched result.CELResult = celResult - pe.log.Debugf(ctx, "CEL result: matched=%v value=%v", celResult.Matched, celResult.Value) + slog.DebugContext(ctx, "cel result", "matched", celResult.Matched, "value", celResult.Value) // Record CEL evaluation in execution context execCtx.AddCELEvaluation(PhasePreconditions, precond.Name, precond.Expression, celResult.Matched) default: // No conditions specified - consider it matched - pe.log.Debugf(ctx, "No conditions specified, auto-matched") + slog.DebugContext(ctx, "no conditions specified, auto-matched") result.Matched = true } @@ -251,7 +249,7 @@ func (pe *PreconditionExecutor) executeAPICall( apiCall *configloader.APICall, execCtx *ExecutionContext, ) ([]byte, error) { - resp, url, err := ExecuteAPICall(ctx, apiCall, execCtx, pe.apiClient, pe.log) + resp, url, err := ExecuteAPICall(ctx, apiCall, execCtx, pe.apiClient) // Validate response - returns APIError with full metadata if validation fails if validationErr := ValidateAPIResponse(resp, err, apiCall.Method, url); validationErr != nil { diff --git a/internal/executor/resource_executor.go b/internal/executor/resource_executor.go index cc2ba043..1f484d9f 100644 --- a/internal/executor/resource_executor.go +++ b/internal/executor/resource_executor.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "time" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/configloader" @@ -12,7 +13,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/maestroclient" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/manifest" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/transportclient" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/metrics" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/utils" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -23,7 +23,6 @@ import ( // ResourceExecutor creates and updates Kubernetes resources type ResourceExecutor struct { client transportclient.TransportClient - log logger.Logger metrics *metrics.Recorder } @@ -32,7 +31,6 @@ type ResourceExecutor struct { func newResourceExecutor(config *ExecutorConfig) *ResourceExecutor { return &ResourceExecutor{ client: config.TransportClient, - log: config.Logger, metrics: config.MetricsRecorder, } } @@ -158,12 +156,14 @@ func (re *ResourceExecutor) executeResource( execCtx.Adapter.SkipReason = fmt.Sprintf("%s: %s", resource.Name, result.OperationReason) } - re.log.Infof(ctx, "Resource[%s] skipped: create.when condition is false", resource.Name) + slog.InfoContext(ctx, "resource skipped: create.when condition is false", "resource", resource.Name) return result, nil } - re.log.Debugf(ctx, "Resource[%s] lifecycle.create.when evaluated to true, creating resource", resource.Name) + slog.DebugContext(ctx, "resource lifecycle.create.when evaluated to true, creating resource", + "resource", resource.Name) } else { - re.log.Debugf(ctx, "Resource[%s] already exists: ignoring lifecycle.create.when, applying normally", resource.Name) + slog.DebugContext(ctx, "resource already exists: ignoring lifecycle.create.when, applying normally", + "resource", resource.Name) } } @@ -184,11 +184,12 @@ func (re *ResourceExecutor) executeResource( return re.executeResourceDelete(ctx, resource, execCtx, transportTarget) } // when-expression is false → fall through to normal apply flow - re.log.Debugf(ctx, "Resource[%s] lifecycle.delete.when evaluated to false, applying normally", resource.Name) + slog.DebugContext(ctx, "resource lifecycle.delete.when evaluated to false, applying normally", + "resource", resource.Name) } // Step 3: Render the manifest/manifestWork to bytes - re.log.Debugf(ctx, "Rendering manifest template for resource %s", resource.Name) + slog.DebugContext(ctx, "rendering manifest template", "resource", resource.Name) renderedBytes, err := re.renderToBytes(resource, execCtx) if err != nil { result.Status = StatusFailed @@ -220,9 +221,8 @@ func (re *ResourceExecutor) executeResource( Step: resource.Name, Message: err.Error(), } - errCtx := logger.WithK8sResult(ctx, "FAILED") - errCtx = logger.WithErrorField(errCtx, err) - re.log.Errorf(errCtx, "Resource[%s] processed: FAILED", resource.Name) + slog.ErrorContext(ctx, "resource processed: failed", + "resource", resource.Name, "k8s_result", "FAILED", "error", err) return result, NewExecutorError(PhaseResources, resource.Name, "failed to apply resource", err) } @@ -230,9 +230,8 @@ func (re *ResourceExecutor) executeResource( result.Operation = applyResult.Operation result.OperationReason = applyResult.Reason - successCtx := logger.WithK8sResult(ctx, "SUCCESS") - re.log.Infof(successCtx, "Resource[%s] processed: operation=%s reason=%s", - resource.Name, result.Operation, result.OperationReason) + slog.InfoContext(ctx, "resource processed", + "resource", resource.Name, "k8s_result", "SUCCESS", "operation", result.Operation, "reason", result.OperationReason) // Step 7: Post-apply discovery — find the applied resource and store in execCtx for CEL evaluation if resource.Discovery != nil { @@ -245,9 +244,8 @@ func (re *ResourceExecutor) executeResource( Step: resource.Name, Message: discoverErr.Error(), } - errCtx := logger.WithK8sResult(ctx, "FAILED") - errCtx = logger.WithErrorField(errCtx, discoverErr) - re.log.Errorf(errCtx, "Resource[%s] discovery after apply failed: %v", resource.Name, discoverErr) + slog.ErrorContext(ctx, "resource discovery after apply failed", + "resource", resource.Name, "k8s_result", "FAILED", "error", discoverErr) return result, NewExecutorError( PhaseResources, resource.Name, "failed to discover resource after apply", discoverErr) } @@ -255,16 +253,16 @@ func (re *ResourceExecutor) executeResource( // Always store the discovered top-level resource by resource name. // Nested discoveries are added as independent entries keyed by nested name. execCtx.Resources[resource.Name] = discovered - re.log.Debugf(ctx, "Resource[%s] discovered and stored in context", resource.Name) + slog.DebugContext(ctx, "resource discovered and stored in context", "resource", resource.Name) // Step 8: Nested discoveries — find sub-resources within the discovered parent (e.g., ManifestWork) if len(resource.NestedDiscoveries) > 0 { nestedResults := re.discoverNestedResources(ctx, resource, execCtx, discovered) for nestedName, nestedObj := range nestedResults { if nestedName == resource.Name { - re.log.Warnf(ctx, - "Nested discovery %q has the same name as parent resource; skipping to avoid overwriting parent", - nestedName) + slog.WarnContext(ctx, + "nested discovery has same name as parent resource; skipping to avoid overwriting parent", + "nested_discovery", nestedName) continue } if nestedObj == nil { @@ -290,8 +288,8 @@ func (re *ResourceExecutor) executeResource( } execCtx.Resources[nestedName] = nestedObj } - re.log.Debugf(ctx, "Resource[%s] discovered with %d nested resources added to context", - resource.Name, len(nestedResults)) + slog.DebugContext(ctx, "resource discovered with nested resources added to context", + "resource", resource.Name, "nested_count", len(nestedResults)) } } } @@ -410,22 +408,22 @@ func (re *ResourceExecutor) discoverNestedResources( // Build discovery config with rendered templates discoveryConfig, err := re.buildNestedDiscoveryConfig(nd.Discovery, execCtx.Params) if err != nil { - re.log.Warnf(ctx, "Resource[%s] nested discovery[%s] failed to build config: %v", - resource.Name, nd.Name, err) + slog.WarnContext(ctx, "resource nested discovery failed to build config", + "resource", resource.Name, "nested_discovery", nd.Name, "error", err) continue } // Search within the parent resource list, err := manifest.DiscoverNestedManifest(parent, discoveryConfig) if err != nil { - re.log.Warnf(ctx, "Resource[%s] nested discovery[%s] failed: %v", - resource.Name, nd.Name, err) + slog.WarnContext(ctx, "resource nested discovery failed", + "resource", resource.Name, "nested_discovery", nd.Name, "error", err) continue } if len(list.Items) == 0 { - re.log.Debugf(ctx, "Resource[%s] nested discovery[%s] found no matches", - resource.Name, nd.Name) + slog.DebugContext(ctx, "resource nested discovery found no matches", + "resource", resource.Name, "nested_discovery", nd.Name) continue } @@ -434,8 +432,8 @@ func (re *ResourceExecutor) discoverNestedResources( if best != nil { manifest.EnrichWithResourceStatus(parent, best) nestedResults[nd.Name] = best - re.log.Debugf(ctx, "Resource[%s] nested discovery[%s] found: %s/%s", - resource.Name, nd.Name, best.GetKind(), best.GetName()) + slog.DebugContext(ctx, "resource nested discovery found", + "resource", resource.Name, "nested_discovery", nd.Name, "kind", best.GetKind(), "name", best.GetName()) } } @@ -553,8 +551,8 @@ func (re *ResourceExecutor) preDiscoverAll( if resource.IsMaestroTransport() && resource.Transport.Maestro != nil { targetCluster, err := utils.RenderTemplate(resource.Transport.Maestro.TargetCluster, execCtx.Params) if err != nil { - re.log.Warnf(ctx, "Resource[%s] pre-discovery: failed to render targetCluster: %v", - resource.Name, err) + slog.WarnContext(ctx, "resource pre-discovery: failed to render targetCluster", + "resource", resource.Name, "error", err) return NewExecutorError(PhaseResources, resource.Name, "failed to render targetCluster", err) } transportTarget = &maestroclient.TransportContext{ConsumerName: targetCluster} @@ -568,12 +566,12 @@ func (re *ResourceExecutor) preDiscoverAll( } // Transient error (RBAC, network, API server): propagate so the reconciliation // fails visibly rather than treating the resource as absent. - re.log.Warnf(ctx, "Resource[%s] pre-discovery failed: %v", resource.Name, err) + slog.WarnContext(ctx, "resource pre-discovery failed", "resource", resource.Name, "error", err) return NewExecutorError(PhaseResources, resource.Name, "pre-discovery failed", err) } if discovered != nil { execCtx.Resources[resource.Name] = discovered - re.log.Debugf(ctx, "Resource[%s] pre-discovered and stored in context", resource.Name) + slog.DebugContext(ctx, "resource pre-discovered and stored in context", "resource", resource.Name) } } return nil @@ -595,7 +593,7 @@ func (re *ResourceExecutor) evaluateLifecycleWhen( evalCtx := criteria.NewEvaluationContext() evalCtx.SetVariablesFromMap(execCtx.GetCELVariables()) - evaluator, err := criteria.NewEvaluator(ctx, evalCtx, re.log) + evaluator, err := criteria.NewEvaluator(ctx, evalCtx) if err != nil { return false, fmt.Errorf("failed to create CEL evaluator: %w", err) } @@ -607,7 +605,8 @@ func (re *ResourceExecutor) evaluateLifecycleWhen( } execCtx.AddCELEvaluation(PhaseResources, resource.Name+"/"+kind, expression, celResult.Matched) - re.log.Debugf(ctx, "Resource[%s] %s=%q → matched=%v", resource.Name, kind, expression, celResult.Matched) + slog.DebugContext(ctx, "resource lifecycle when evaluated", + "resource", resource.Name, "kind", kind, "expression", expression, "matched", celResult.Matched) return celResult.Matched, nil } @@ -670,7 +669,7 @@ func (re *ResourceExecutor) executeResourceDelete( // !resources.?X.hasValue() evaluates to true in this reconciliation. execCtx.Resources[resource.Name] = nil result.OperationReason = "resource already deleted or never existed" - re.log.Infof(ctx, "Resource[%s] delete: already deleted or never existed", resource.Name) + slog.InfoContext(ctx, "resource delete: already deleted or never existed", "resource", resource.Name) re.metrics.RecordDeletion(resourceType, metrics.DeletionStatusSuccess) re.metrics.ObserveDeletionDuration(resourceType, time.Since(startTime)) return result, nil @@ -704,9 +703,8 @@ func (re *ResourceExecutor) executeResourceDelete( result.Status = StatusFailed result.Error = err re.recordResourceError(execCtx, resource, err) - errCtx := logger.WithK8sResult(ctx, "FAILED") - errCtx = logger.WithErrorField(errCtx, err) - re.log.Errorf(errCtx, "Resource[%s] delete: FAILED", resource.Name) + slog.ErrorContext(ctx, "resource delete: failed", + "resource", resource.Name, "k8s_result", "FAILED", "error", err) re.metrics.RecordDeletion(resourceType, metrics.DeletionStatusError) re.metrics.ObserveDeletionDuration(resourceType, time.Since(startTime)) return result, NewExecutorError(PhaseResources, resource.Name, "failed to delete resource", err) @@ -722,23 +720,24 @@ func (re *ResourceExecutor) executeResourceDelete( switch { case postDiscoverErr != nil && !postIsNotFound: // Non-fatal: log the error but don't fail the delete — the delete itself succeeded. - re.log.Debugf(ctx, "Resource[%s] post-delete discovery error (non-fatal): %v", resource.Name, postDiscoverErr) + slog.DebugContext(ctx, "resource post-delete discovery error (non-fatal)", + "resource", resource.Name, "error", postDiscoverErr) execCtx.Resources[resource.Name] = discovered case postDeleteDiscovered == nil || postIsNotFound: // Resource is confirmed gone: dependent resources can proceed in this reconciliation. execCtx.Resources[resource.Name] = nil - re.log.Debugf(ctx, "Resource[%s] confirmed deleted (post-delete discovery: not found)", resource.Name) + slog.DebugContext(ctx, "resource confirmed deleted (post-delete discovery: not found)", "resource", resource.Name) default: // Resource still present (finalizers or async deletion): dependents wait for next reconciliation. execCtx.Resources[resource.Name] = postDeleteDiscovered - re.log.Debugf(ctx, "Resource[%s] still present after delete (finalizers or async): dependents wait", resource.Name) + slog.DebugContext(ctx, "resource still present after delete (finalizers or async): dependents wait", + "resource", resource.Name) } result.OperationReason = "lifecycle.delete.when evaluated to true" - re.log.Infof(logger.WithK8sResult(ctx, "SUCCESS"), - "Resource[%s] delete: operation=delete propagationPolicy=%s", - resource.Name, propagationPolicy) + slog.InfoContext(ctx, "resource delete", + "resource", resource.Name, "k8s_result", "SUCCESS", "operation", "delete", "propagation_policy", propagationPolicy) re.metrics.RecordDeletion(resourceType, metrics.DeletionStatusSuccess) re.metrics.ObserveDeletionDuration(resourceType, time.Since(startTime)) diff --git a/internal/executor/resource_executor_test.go b/internal/executor/resource_executor_test.go index a52626d7..d8751830 100644 --- a/internal/executor/resource_executor_test.go +++ b/internal/executor/resource_executor_test.go @@ -9,7 +9,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/k8sclient" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/manifest" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/transportclient" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -37,7 +36,6 @@ func TestResourceExecutor_ExecuteAll_DiscoveryFailure(t *testing.T) { config := &ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), } re := newResourceExecutor(config) @@ -140,7 +138,6 @@ func TestResourceExecutor_ExecuteAll_StoresNestedDiscoveriesByName(t *testing.T) re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := configloader.Resource{ @@ -201,9 +198,7 @@ func TestResourceExecutor_ExecuteAll_StoresNestedDiscoveriesByName(t *testing.T) } func TestRenderToBytes_StringManifest(t *testing.T) { - re := newResourceExecutor(&ExecutorConfig{ - Logger: logger.NewTestLogger(), - }) + re := newResourceExecutor(&ExecutorConfig{}) tests := []struct { name string @@ -328,9 +323,7 @@ metadata: func TestRenderToBytes_StringManifestWithSubnetList(t *testing.T) { // Test the customer's original use case: generating a list of subnets - re := newResourceExecutor(&ExecutorConfig{ - Logger: logger.NewTestLogger(), - }) + re := newResourceExecutor(&ExecutorConfig{}) manifest := `apiVersion: v1 kind: ConfigMap @@ -361,9 +354,7 @@ data: } func TestRenderToBytes_StringManifestEdgeCases(t *testing.T) { - re := newResourceExecutor(&ExecutorConfig{ - Logger: logger.NewTestLogger(), - }) + re := newResourceExecutor(&ExecutorConfig{}) t.Run("plain YAML string without templates", func(t *testing.T) { // Backward compatibility: plain YAML ref files (no templates) still work @@ -485,7 +476,6 @@ func TestResourceExecutor_ExecuteAll_StringManifest(t *testing.T) { re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) // Use a string manifest with structural Go templates @@ -728,7 +718,6 @@ func TestResourceExecutor_LifecycleCreate_WhenTrue_ResourceNotFound_Applied(t *t re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := newResourceWithLifecycleCreate("shouldCreate") @@ -751,7 +740,6 @@ func TestResourceExecutor_LifecycleCreate_WhenFalse_ResourceNotFound_Skipped(t * re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := newResourceWithLifecycleCreate("shouldCreate") @@ -780,7 +768,6 @@ func TestResourceExecutor_LifecycleCreate_WhenCELError_ExecutionFails(t *testing re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) // Invalid CEL syntax — evaluateLifecycleWhen will error. @@ -818,7 +805,6 @@ func TestResourceExecutor_LifecycleCreate_ResourceAlreadyExists_IgnoresWhen(t *t re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := newResourceWithLifecycleCreate("shouldCreate") @@ -852,7 +838,6 @@ func TestResourceExecutor_LifecycleCreate_Absent_NormalApply(t *testing.T) { re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := configloader.Resource{ @@ -977,7 +962,6 @@ func TestResourceExecutor_LifecycleDelete_WhenTrue_ResourceFound_InstantDelete(t re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := newResourceWithLifecycle("deleted_time != null", "Background") @@ -1020,7 +1004,6 @@ func TestResourceExecutor_LifecycleDelete_WhenTrue_ResourceFound_WithFinalizers( re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := newResourceWithLifecycle("deleted_time != null", "Background") @@ -1051,7 +1034,6 @@ func TestResourceExecutor_LifecycleDelete_WhenTrue_ResourceNotFound(t *testing.T re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := newResourceWithLifecycle("deleted_time != null", "Background") @@ -1082,7 +1064,6 @@ func TestResourceExecutor_LifecycleDelete_WhenFalse_NormalApply(t *testing.T) { re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) // deleted_time is null → expression "deleted_time != null" is false @@ -1116,7 +1097,6 @@ func TestResourceExecutor_LifecycleDelete_NoLifecycle_NormalApply(t *testing.T) re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := configloader.Resource{ @@ -1148,7 +1128,6 @@ func TestResourceExecutor_LifecycleDelete_NoExpression_DefaultsFalse(t *testing. re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := newResourceWithLifecycle("", "Foreground") @@ -1188,7 +1167,6 @@ func TestResourceExecutor_LifecycleDelete_OrderingViaResources_InstantDelete(t * re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) clusterJob := configloader.Resource{ @@ -1265,7 +1243,6 @@ func TestResourceExecutor_LifecycleDelete_OrderingViaResources_WithFinalizers(t re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) clusterJob := configloader.Resource{ @@ -1340,7 +1317,6 @@ func TestResourceExecutor_LifecycleDelete_OrderingSecondReconciliation(t *testin re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock2, - Logger: logger.NewTestLogger(), }) clusterJob := configloader.Resource{ @@ -1411,7 +1387,6 @@ func TestResourceExecutor_LifecycleDelete_DeleteError(t *testing.T) { re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := newResourceWithLifecycle("deleted_time != null", "Background") @@ -1461,7 +1436,6 @@ func TestResourceExecutor_LifecycleDelete_InvalidCELExpression(t *testing.T) { re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) // "deleted_time != null &&" is a dangling logical-AND — invalid CEL syntax. @@ -1488,7 +1462,6 @@ func TestResourceExecutor_LifecycleDelete_CELUndeclaredVariable(t *testing.T) { re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) // "not_captured_var" is intentionally absent from execCtx.Params. @@ -1535,7 +1508,6 @@ func TestResourceExecutor_LifecycleDelete_PropagationPolicy(t *testing.T) { re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := newResourceWithLifecycle("deleted_time != null", tt.policy) @@ -1579,7 +1551,6 @@ func TestResourceExecutor_LifecycleDelete_PreDeleteDiscoveryError(t *testing.T) re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := newResourceWithLifecycle("deleted_time != null", "Background") @@ -1611,7 +1582,6 @@ func TestResourceExecutor_LifecycleDelete_DeleteConfigNil(t *testing.T) { re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := configloader.Resource{ @@ -1659,7 +1629,6 @@ func TestResourceExecutor_LifecycleDelete_Maestro_AsyncDeletion(t *testing.T) { re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := configloader.Resource{ @@ -1760,7 +1729,6 @@ func TestResourceExecutor_ExecuteAll_ContinuesAfterDeleteFailure(t *testing.T) { re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resourceA := newResourceWithLifecycle("deleted_time != null", "Background") @@ -1800,7 +1768,6 @@ func TestResourceExecutor_ExecuteAll_ContinuesAfterCELEvalError(t *testing.T) { re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) // resourceA has an invalid CEL expression — evaluateLifecycleDeleteWhen will error. @@ -1915,7 +1882,6 @@ func TestResourceExecutor_LifecycleDelete_BySelectors(t *testing.T) { re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, - Logger: logger.NewTestLogger(), }) resource := configloader.Resource{ diff --git a/internal/executor/types.go b/internal/executor/types.go index 31fafa54..896d85b7 100644 --- a/internal/executor/types.go +++ b/internal/executor/types.go @@ -10,7 +10,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/hyperfleetapi" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/manifest" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/transportclient" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/metrics" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) @@ -65,8 +64,6 @@ type ExecutorConfig struct { APIClient hyperfleetapi.Client // TransportClient is the transport client for applying resources (kubernetes or maestro) TransportClient transportclient.TransportClient - // Logger is the logger instance - Logger logger.Logger // MetricsRecorder is the optional Prometheus metrics recorder MetricsRecorder *metrics.Recorder } @@ -77,7 +74,6 @@ type Executor struct { precondExecutor *PreconditionExecutor resourceExecutor *ResourceExecutor postActionExecutor *PostActionExecutor - log logger.Logger } // ExecutionResult contains the result of processing an event diff --git a/internal/executor/utils.go b/internal/executor/utils.go index f042c773..dae8fb23 100644 --- a/internal/executor/utils.go +++ b/internal/executor/utils.go @@ -3,6 +3,7 @@ package executor import ( "context" "fmt" + "log/slog" "net/http" "net/url" "os" @@ -14,8 +15,8 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/criteria" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/hyperfleetapi" apierrors "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/errors" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/utils" + hfl "github.com/openshift-hyperfleet/hyperfleet-logger" ) // ToConditionDefs converts configloader.Condition slice to criteria.ConditionDef slice. @@ -39,7 +40,6 @@ func ExecuteLogAction( ctx context.Context, logAction *configloader.LogAction, execCtx *ExecutionContext, - log logger.Logger, ) { if logAction == nil || logAction.Message == "" { return @@ -48,30 +48,16 @@ func ExecuteLogAction( // Render the message template message, err := utils.RenderTemplate(logAction.Message, execCtx.Params) if err != nil { - errCtx := logger.WithErrorField(ctx, err) - log.Errorf(errCtx, "failed to render log message") + slog.ErrorContext(ctx, "failed to render log message", "error", err) return } - // Log at the specified level (default: info) - level := strings.ToLower(logAction.Level) - if level == "" { - level = "info" - } - - switch level { - case "debug": - log.Debugf(ctx, "[config] %s", message) - case "info": - log.Infof(ctx, "[config] %s", message) - case "warning", "warn": - log.Warnf(ctx, "[config] %s", message) - case "error": - log.Errorf(ctx, "[config] %s", message) - default: - log.Infof(ctx, "[config] %s", message) + // Log at the specified level; unknown levels fall back to info. + level, err := hfl.ParseLevel(logAction.Level) + if err != nil { + slog.ErrorContext(ctx, "invalid log level") } - + slog.Log(ctx, level, "[config] "+message) } // ExecuteAPICall executes an API call with the given configuration and returns the response and rendered URL @@ -83,7 +69,6 @@ func ExecuteAPICall( apiCall *configloader.APICall, execCtx *ExecutionContext, apiClient hyperfleetapi.Client, - log logger.Logger, ) (*hyperfleetapi.Response, string, error) { if apiCall == nil { return nil, "", fmt.Errorf("apiCall is nil") @@ -98,7 +83,7 @@ func ExecuteAPICall( // Then build the final URL - this handles absolute URLs vs relative paths url := buildHyperfleetAPICallURL(renderedURL, execCtx) - log.Infof(ctx, "Making API call: %s %s", apiCall.Method, url) + slog.InfoContext(ctx, "making api call", "method", apiCall.Method, "url", url) // Build request options opts := make([]hyperfleetapi.RequestOption, 0) @@ -122,7 +107,8 @@ func ExecuteAPICall( if timeoutErr == nil { opts = append(opts, hyperfleetapi.WithRequestTimeout(timeout)) } else { - log.Warnf(ctx, "failed to parse timeout '%s': %v, using default timeout", apiCall.Timeout, timeoutErr) + slog.WarnContext(ctx, "failed to parse timeout, using default timeout", + "timeout", apiCall.Timeout, "error", timeoutErr) } } @@ -148,7 +134,7 @@ func ExecuteAPICall( return nil, url, fmt.Errorf("failed to render body template: %w", err) } } - log.Debugf(ctx, "API call payload: %s %s payload=%s", apiCall.Method, url, string(body)) + slog.DebugContext(ctx, "api call payload", "method", apiCall.Method, "url", url, "payload", string(body)) resp, err = apiClient.Post(ctx, url, body, opts...) // Log error message on failure for debugging purposes if err != nil || (resp != nil && !resp.IsSuccess()) { @@ -158,8 +144,7 @@ func ExecuteAPICall( } else { logErr = fmt.Errorf("POST %s returned non-success status: %d", url, resp.StatusCode) } - errCtx := logger.WithErrorField(ctx, logErr) - log.Error(errCtx, "POST Request failed") + slog.ErrorContext(ctx, "post request failed", "error", logErr) } case http.MethodPut: body := []byte(apiCall.Body) @@ -169,7 +154,7 @@ func ExecuteAPICall( return nil, "", fmt.Errorf("failed to render body template: %w", err) } } - log.Debugf(ctx, "API call payload: %s %s payload=%s", apiCall.Method, url, string(body)) + slog.DebugContext(ctx, "api call payload", "method", apiCall.Method, "url", url, "payload", string(body)) resp, err = apiClient.Put(ctx, url, body, opts...) // Log error message on failure for debugging purposes if err != nil || (resp != nil && !resp.IsSuccess()) { @@ -179,8 +164,7 @@ func ExecuteAPICall( } else { logErr = fmt.Errorf("PUT %s returned non-success status: %d", url, resp.StatusCode) } - errCtx := logger.WithErrorField(ctx, logErr) - log.Error(errCtx, "PUT Request failed") + slog.ErrorContext(ctx, "put request failed", "error", logErr) } case http.MethodPatch: body := []byte(apiCall.Body) @@ -190,7 +174,7 @@ func ExecuteAPICall( return nil, "", fmt.Errorf("failed to render body template: %w", err) } } - log.Debugf(ctx, "API call payload: %s %s payload=%s", apiCall.Method, url, string(body)) + slog.DebugContext(ctx, "api call payload", "method", apiCall.Method, "url", url, "payload", string(body)) resp, err = apiClient.Patch(ctx, url, body, opts...) case http.MethodDelete: resp, err = apiClient.Delete(ctx, url, opts...) @@ -202,7 +186,7 @@ func ExecuteAPICall( // Return response AND error - response may contain useful details even on error // (e.g., HTTP status code, response body) if resp != nil { - log.Warnf(ctx, "API call failed: %d %s, error: %v", resp.StatusCode, resp.Status, err) + slog.WarnContext(ctx, "api call failed", "status_code", resp.StatusCode, "status", resp.Status, "error", err) // Wrap as APIError with full context apiErr := apierrors.NewAPIError( apiCall.Method, @@ -216,7 +200,7 @@ func ExecuteAPICall( ) return resp, url, apiErr } else { - log.Warnf(ctx, "API call failed: %v", err) + slog.WarnContext(ctx, "api call failed", "error", err) // No response - create APIError with minimal context apiErr := apierrors.NewAPIError( apiCall.Method, @@ -236,7 +220,7 @@ func ExecuteAPICall( return nil, url, apierrors.NewAPIError(apiCall.Method, url, 0, "", nil, 0, 0, nilErr) } - log.Infof(ctx, "API call completed: %d %s", resp.StatusCode, resp.Status) + slog.InfoContext(ctx, "api call completed", "status_code", resp.StatusCode, "status", resp.Status) return resp, url, nil } diff --git a/internal/executor/utils_test.go b/internal/executor/utils_test.go index 2b6dd8a8..a8705aad 100644 --- a/internal/executor/utils_test.go +++ b/internal/executor/utils_test.go @@ -11,7 +11,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/criteria" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/hyperfleetapi" apierrors "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/errors" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -722,11 +721,10 @@ func TestExecuteLogAction(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - log := logger.NewTestLogger() execCtx := &ExecutionContext{Params: tt.params} // This should not panic - ExecuteLogAction(context.Background(), tt.logAction, execCtx, log) + ExecuteLogAction(context.Background(), tt.logAction, execCtx) // We don't verify the exact log output, just that it doesn't error }) diff --git a/internal/hyperfleetapi/client.go b/internal/hyperfleetapi/client.go index 974d5c02..79e1e60a 100644 --- a/internal/hyperfleetapi/client.go +++ b/internal/hyperfleetapi/client.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "log/slog" "math" "math/big" "net/http" @@ -14,8 +15,8 @@ import ( "strings" "time" + "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/logctx" apierrors "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/errors" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/version" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" @@ -36,7 +37,6 @@ const ( type httpClient struct { client *http.Client config *ClientConfig - log logger.Logger tokenSource *fileTokenSource } @@ -128,10 +128,9 @@ func WithAuth(auth *AuthConfig) ClientOption { // This function owns the environment variable fallback logic; callers should // pass the configured value via WithBaseURL if available, otherwise NewClient // reads the env var as a last resort. -func NewClient(log logger.Logger, opts ...ClientOption) (Client, error) { +func NewClient(opts ...ClientOption) (Client, error) { c := &httpClient{ config: DefaultClientConfig(), - log: log, } // Apply options (including WithBaseURL if provided by caller) @@ -219,7 +218,8 @@ func (c *httpClient) Do(ctx context.Context, req *Request) (*Response, error) { resp, err := c.doRequest(ctx, req) if err != nil { lastErr = err - c.log.Warnf(ctx, "HyperFleet API request failed (attempt %d/%d): %v", attempt, retryAttempts, err) + slog.WarnContext(ctx, "hyperfleet api request failed", + "attempt", attempt, "max_attempts", retryAttempts, "error", err) } else { resp.Attempts = attempt resp.Duration = time.Since(startTime) @@ -231,14 +231,14 @@ func (c *httpClient) Do(ctx context.Context, req *Request) (*Response, error) { lastResp = resp lastErr = fmt.Errorf("HTTP %d: %s", resp.StatusCode, resp.Status) - c.log.Warnf(ctx, "HyperFleet API request returned retryable status %d (attempt %d/%d)", - resp.StatusCode, attempt, retryAttempts) + slog.WarnContext(ctx, "hyperfleet api request returned retryable status", + "status_code", resp.StatusCode, "attempt", attempt, "max_attempts", retryAttempts) } // Don't sleep after the last attempt if attempt < retryAttempts { delay := c.calculateBackoff(attempt, backoffStrategy) - c.log.Infof(ctx, "Retrying in %v...", delay) + slog.InfoContext(ctx, "retrying request", "delay", delay) select { case <-ctx.Done(): @@ -301,7 +301,7 @@ func (c *httpClient) doRequest(ctx context.Context, req *Request) (*Response, er defer span.End() // Update logger context with new span_id for this request - ctx = logger.WithOTelTraceContext(ctx) + ctx = logctx.WithOTelTraceContext(ctx) // Determine timeout timeout := c.config.Timeout @@ -358,14 +358,14 @@ func (c *httpClient) doRequest(ctx context.Context, req *Request) (*Response, er otel.GetTextMapPropagator().Inject(reqCtx, propagation.HeaderCarrier(httpReq.Header)) // Execute request - c.log.Debugf(ctx, "HyperFleet API request: %s %s", req.Method, req.URL) + slog.DebugContext(ctx, "hyperfleet api request", "method", req.Method, "url", req.URL) httpResp, err := c.client.Do(httpReq) if err != nil { return nil, fmt.Errorf("HTTP request failed: %w", err) } defer func() { if closeErr := httpResp.Body.Close(); closeErr != nil { - c.log.Warnf(ctx, "Failed to close response body: %v", closeErr) + slog.WarnContext(ctx, "failed to close response body", "error", closeErr) } }() @@ -382,7 +382,7 @@ func (c *httpClient) doRequest(ctx context.Context, req *Request) (*Response, er Body: respBody, } - c.log.Debugf(ctx, "HyperFleet API response: %d %s", response.StatusCode, response.Status) + slog.DebugContext(ctx, "hyperfleet api response", "status_code", response.StatusCode, "status", response.Status) return response, nil } diff --git a/internal/hyperfleetapi/client_test.go b/internal/hyperfleetapi/client_test.go index a49f5937..9f072101 100644 --- a/internal/hyperfleetapi/client_test.go +++ b/internal/hyperfleetapi/client_test.go @@ -9,37 +9,18 @@ import ( "net/http/httptest" "os" "path/filepath" - "sync" "sync/atomic" "testing" "time" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/errors" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -var ( - sharedTestLogger logger.Logger - loggerOnce sync.Once -) - -func testLog() logger.Logger { - loggerOnce.Do(func() { - var err error - cfg := logger.Config{Level: "error", Format: "text", Output: "stdout", Component: "test", Version: "test"} - sharedTestLogger, err = logger.NewLogger(cfg) - if err != nil { - panic(err) - } - }) - return sharedTestLogger -} - func TestNewClient(t *testing.T) { // NewClient requires base URL - test with explicit base URL - client, err := NewClient(testLog(), WithBaseURL("http://localhost:8080")) + client, err := NewClient(WithBaseURL("http://localhost:8080")) require.NoError(t, err) require.NotNil(t, client) } @@ -62,7 +43,7 @@ func TestNewClientMissingBaseURL(t *testing.T) { }) } - _, err := NewClient(testLog()) + _, err := NewClient() require.Error(t, err) assert.Contains(t, err.Error(), "base URL") } @@ -136,7 +117,7 @@ func TestNewClientWithOptions(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - client, err := NewClient(testLog(), tt.opts...) + client, err := NewClient(tt.opts...) if err != nil { t.Errorf("NewClient returned error: %v", err) } @@ -161,7 +142,7 @@ func TestClientGet(t *testing.T) { defer server.Close() // Use server URL as base URL for testing - client, err := NewClient(testLog(), WithBaseURL(server.URL)) + client, err := NewClient(WithBaseURL(server.URL)) require.NoError(t, err, "failed to create client") ctx := context.Background() @@ -194,7 +175,7 @@ func TestClientPost(t *testing.T) { })) defer server.Close() - client, err := NewClient(testLog(), WithBaseURL(server.URL)) + client, err := NewClient(WithBaseURL(server.URL)) require.NoError(t, err, "failed to create client") ctx := context.Background() body := []byte(`{"key":"value"}`) @@ -226,7 +207,7 @@ func TestClientWithHeaders(t *testing.T) { })) defer server.Close() - client, err := NewClient(testLog(), WithBaseURL(server.URL), + client, err := NewClient(WithBaseURL(server.URL), WithDefaultHeader("Authorization", "Bearer default-token")) require.NoError(t, err, "failed to create client") ctx := context.Background() @@ -266,7 +247,7 @@ func TestClientRetry(t *testing.T) { config.RetryAttempts = 3 config.BaseDelay = 10 * time.Millisecond // Short delay for tests - client, err := NewClient(testLog(), WithConfig(config)) + client, err := NewClient(WithConfig(config)) require.NoError(t, err, "failed to create client") ctx := context.Background() @@ -301,7 +282,7 @@ func TestClientRetryExhausted(t *testing.T) { config.RetryAttempts = 3 config.BaseDelay = 10 * time.Millisecond - client, err := NewClient(testLog(), WithConfig(config)) + client, err := NewClient(WithConfig(config)) require.NoError(t, err, "failed to create client") ctx := context.Background() @@ -332,7 +313,7 @@ func TestClientNoRetryOn4xx(t *testing.T) { config.BaseURL = server.URL config.RetryAttempts = 3 - client, err := NewClient(testLog(), WithConfig(config)) + client, err := NewClient(WithConfig(config)) require.NoError(t, err, "failed to create client") ctx := context.Background() @@ -362,7 +343,7 @@ func TestClientTimeout(t *testing.T) { config.Timeout = 100 * time.Millisecond config.RetryAttempts = 1 - client, err := NewClient(testLog(), WithConfig(config)) + client, err := NewClient(WithConfig(config)) require.NoError(t, err, "failed to create client") ctx := context.Background() @@ -377,7 +358,7 @@ func TestClientContextCancellation(t *testing.T) { })) defer server.Close() - client, err := NewClient(testLog(), WithBaseURL(server.URL)) + client, err := NewClient(WithBaseURL(server.URL)) require.NoError(t, err, "failed to create client") ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) defer cancel() @@ -480,7 +461,7 @@ func TestClientPut(t *testing.T) { })) defer server.Close() - client, err := NewClient(testLog(), WithBaseURL(server.URL)) + client, err := NewClient(WithBaseURL(server.URL)) require.NoError(t, err, "failed to create client") ctx := context.Background() @@ -501,7 +482,7 @@ func TestClientPatch(t *testing.T) { })) defer server.Close() - client, err := NewClient(testLog(), WithBaseURL(server.URL)) + client, err := NewClient(WithBaseURL(server.URL)) require.NoError(t, err, "failed to create client") ctx := context.Background() @@ -522,7 +503,7 @@ func TestClientDelete(t *testing.T) { })) defer server.Close() - client, err := NewClient(testLog(), WithBaseURL(server.URL)) + client, err := NewClient(WithBaseURL(server.URL)) require.NoError(t, err, "failed to create client") ctx := context.Background() @@ -657,7 +638,7 @@ func TestAPIErrorInRetryExhausted(t *testing.T) { config.RetryAttempts = 2 config.BaseDelay = 10 * time.Millisecond - client, err := NewClient(testLog(), WithConfig(config)) + client, err := NewClient(WithConfig(config)) require.NoError(t, err, "failed to create client") ctx := context.Background() @@ -700,7 +681,7 @@ func TestClientBearerTokenAuth(t *testing.T) { })) defer server.Close() - client, err := NewClient(testLog(), + client, err := NewClient( WithBaseURL(server.URL), WithAuth(&AuthConfig{TokenPath: tokenFile, TokenCacheTTL: 0}), ) @@ -722,7 +703,7 @@ func TestClientNoAuthHeader_WhenNoAuth(t *testing.T) { })) defer server.Close() - client, err := NewClient(testLog(), WithBaseURL(server.URL)) + client, err := NewClient(WithBaseURL(server.URL)) require.NoError(t, err) _, err = client.Get(context.Background(), "/test") diff --git a/internal/k8sclient/apply.go b/internal/k8sclient/apply.go index 7131a8e3..db90d48e 100644 --- a/internal/k8sclient/apply.go +++ b/internal/k8sclient/apply.go @@ -4,10 +4,13 @@ import ( "context" "encoding/json" "fmt" + "log/slog" "time" + "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/logctx" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/manifest" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/transportclient" + hfl "github.com/openshift-hyperfleet/hyperfleet-logger" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" @@ -97,9 +100,11 @@ func (c *Client) ApplyManifest( gvk := newManifest.GroupVersionKind() name := newManifest.GetName() + ctx = hfl.Set(ctx, logctx.K8sKindKey, gvk.Kind) + ctx = hfl.Set(ctx, logctx.K8sNameKey, name) + ctx = hfl.Set(ctx, logctx.K8sNamespaceKey, newManifest.GetNamespace()) - c.log.Debugf(ctx, "ApplyManifest %s/%s: operation=%s reason=%s", - gvk.Kind, name, result.Operation, result.Reason) + slog.DebugContext(ctx, "apply manifest", "operation", result.Operation, "reason", result.Reason) // Execute the operation var applyErr error @@ -109,7 +114,7 @@ func (c *Client) ApplyManifest( if applyErr != nil && apierrors.IsAlreadyExists(applyErr) { // Resource was created by a concurrent process between our Get and Create. // Treat as a successful no-op rather than an error. - c.log.Debugf(ctx, "Resource %s/%s already exists (concurrent create), treating as skip", gvk.Kind, name) + slog.DebugContext(ctx, "resource already exists (concurrent create), treating as skip") result.Operation = manifest.OperationSkip result.Reason = "already exists (concurrent create)" applyErr = nil @@ -150,21 +155,24 @@ func (c *Client) recreateResource( gvk := existing.GroupVersionKind() namespace := existing.GetNamespace() name := existing.GetName() + ctx = hfl.Set(ctx, logctx.K8sKindKey, gvk.Kind) + ctx = hfl.Set(ctx, logctx.K8sNameKey, name) + ctx = hfl.Set(ctx, logctx.K8sNamespaceKey, namespace) // Delete the existing resource - c.log.Debugf(ctx, "Deleting resource for recreation: %s/%s", gvk.Kind, name) + slog.DebugContext(ctx, "deleting resource for recreation") if err := c.deleteResource(ctx, gvk, namespace, name); err != nil { return nil, fmt.Errorf("failed to delete resource for recreation: %w", err) } // Wait for the resource to be fully deleted - c.log.Debugf(ctx, "Waiting for resource deletion to complete: %s/%s", gvk.Kind, name) + slog.DebugContext(ctx, "waiting for resource deletion to complete") if err := c.waitForDeletion(ctx, gvk, namespace, name); err != nil { return nil, fmt.Errorf("failed waiting for resource deletion: %w", err) } // Create the new resource - c.log.Debugf(ctx, "Creating new resource after deletion confirmed: %s/%s", gvk.Kind, name) + slog.DebugContext(ctx, "creating new resource after deletion confirmed") return c.CreateResource(ctx, newManifest) } @@ -183,22 +191,22 @@ func (c *Client) waitForDeletion( for { select { case <-ctx.Done(): - c.log.Warnf(ctx, "Context canceled/timed out while waiting for deletion of %s/%s", gvk.Kind, name) + slog.WarnContext(ctx, "context canceled/timed out while waiting for deletion") return fmt.Errorf("context canceled while waiting for resource deletion: %w", ctx.Err()) case <-ticker.C: _, err := c.GetResource(ctx, gvk, namespace, name, nil) if err != nil { // NotFound means the resource is deleted - this is success if apierrors.IsNotFound(err) { - c.log.Debugf(ctx, "Resource deletion confirmed: %s/%s", gvk.Kind, name) + slog.DebugContext(ctx, "resource deletion confirmed") return nil } // Any other error is unexpected - c.log.Errorf(ctx, "Error checking deletion status for %s/%s: %v", gvk.Kind, name, err) + slog.ErrorContext(ctx, "error checking deletion status", "error", err) return fmt.Errorf("error checking deletion status: %w", err) } // Resource still exists, continue polling - c.log.Debugf(ctx, "Resource %s/%s still exists, waiting for deletion...", gvk.Kind, name) + slog.DebugContext(ctx, "resource still exists, waiting for deletion") } } } diff --git a/internal/k8sclient/apply_test.go b/internal/k8sclient/apply_test.go index 9702590e..5f9752a1 100644 --- a/internal/k8sclient/apply_test.go +++ b/internal/k8sclient/apply_test.go @@ -6,7 +6,6 @@ import ( "testing" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/manifest" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -17,10 +16,8 @@ import ( func newTestClient() *Client { scheme := runtime.NewScheme() builder := fake.NewClientBuilder().WithScheme(scheme) - log, _ := logger.NewLogger(logger.Config{Level: "error", Output: "stdout", Format: "json"}) return &Client{ client: builder.Build(), - log: log, } } diff --git a/internal/k8sclient/client.go b/internal/k8sclient/client.go index 158e8ce3..f8350467 100644 --- a/internal/k8sclient/client.go +++ b/internal/k8sclient/client.go @@ -3,10 +3,10 @@ package k8sclient import ( "context" "encoding/json" + "log/slog" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/transportclient" apperrors "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/errors" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -20,7 +20,6 @@ import ( // Client is the Kubernetes client for managing resources using controller-runtime type Client struct { client client.Client - log logger.Logger } // ClientConfig holds configuration for creating a Kubernetes client @@ -48,12 +47,12 @@ type ClientConfig struct { // // // For production deployment in K8s cluster (uses ServiceAccount) // config := ClientConfig{QPS: 100.0, Burst: 200} -// client, err := NewClient(ctx, config, log) +// client, err := NewClient(ctx, config) // // // For local development (uses explicit kubeconfig path) // config := ClientConfig{KubeConfigPath: "/home/user/.kube/config"} -// client, err := NewClient(ctx, config, log) -func NewClient(ctx context.Context, config ClientConfig, log logger.Logger) (*Client, error) { +// client, err := NewClient(ctx, config) +func NewClient(ctx context.Context, config ClientConfig) (*Client, error) { var restConfig *rest.Config var err error @@ -65,14 +64,14 @@ func NewClient(ctx context.Context, config ClientConfig, log logger.Logger) (*Cl if err != nil { return nil, apperrors.KubernetesError("failed to load kubeconfig from %s: %v", kubeConfigPath, err) } - log.Infof(ctx, "Using kubeconfig from: %s", kubeConfigPath) + slog.InfoContext(ctx, "using kubeconfig", "path", kubeConfigPath) } else { // Use in-cluster config with ServiceAccount restConfig, err = rest.InClusterConfig() if err != nil { return nil, apperrors.KubernetesError("failed to create in-cluster config: %v", err) } - log.Info(ctx, "Using in-cluster Kubernetes configuration (ServiceAccount)") + slog.InfoContext(ctx, "using in-cluster kubernetes configuration (service account)") } // Set rate limits @@ -96,13 +95,12 @@ func NewClient(ctx context.Context, config ClientConfig, log logger.Logger) (*Cl return &Client{ client: k8sClient, - log: log, }, nil } // NewClientFromConfig creates a client from an existing rest.Config // This is useful for testing with envtest -func NewClientFromConfig(ctx context.Context, restConfig *rest.Config, log logger.Logger) (*Client, error) { +func NewClientFromConfig(ctx context.Context, restConfig *rest.Config) (*Client, error) { k8sClient, err := client.New(restConfig, client.Options{}) if err != nil { return nil, apperrors.KubernetesError("failed to create kubernetes client: %v", err) @@ -110,7 +108,6 @@ func NewClientFromConfig(ctx context.Context, restConfig *rest.Config, log logge return &Client{ client: k8sClient, - log: log, }, nil } diff --git a/internal/k8sclient/discovery.go b/internal/k8sclient/discovery.go index aefa893d..39dd9eed 100644 --- a/internal/k8sclient/discovery.go +++ b/internal/k8sclient/discovery.go @@ -2,6 +2,7 @@ package k8sclient import ( "context" + "log/slog" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/manifest" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/transportclient" @@ -44,8 +45,8 @@ func (c *Client) DiscoverResources( if discovery.IsSingleResource() { // Single resource by name - c.log.Infof(ctx, "Discovering single resource: %s/%s (namespace: %s)", - gvk.Kind, discovery.GetName(), discovery.GetNamespace()) + slog.InfoContext(ctx, "discovering single resource", + "kind", gvk.Kind, "name", discovery.GetName(), "namespace", discovery.GetNamespace()) obj, err := c.GetResource(ctx, gvk, discovery.GetNamespace(), discovery.GetName(), nil) if err != nil { diff --git a/internal/logctx/logctx.go b/internal/logctx/logctx.go new file mode 100644 index 00000000..4037b5c0 --- /dev/null +++ b/internal/logctx/logctx.go @@ -0,0 +1,61 @@ +// Package logctx defines adapter-specific typed context keys and helpers +// used to enrich log records via the shared hyperfleet-logger context field +// mechanism. +package logctx + +import ( + "context" + "log/slog" + + hfl "github.com/openshift-hyperfleet/hyperfleet-logger" + "go.opentelemetry.io/otel/trace" +) + +// Adapter-specific typed context keys. +var ( + EventIDKey = hfl.NewKey[string]("event_id") + K8sKindKey = hfl.NewKey[string]("k8s_kind") + K8sNameKey = hfl.NewKey[string]("k8s_name") + K8sNamespaceKey = hfl.NewKey[string]("k8s_namespace") + ObservedGenerationKey = hfl.NewKey[int64]("observed_generation") + MaestroConsumerKey = hfl.NewKey[string]("maestro_consumer") + ManifestWorkKey = hfl.NewKey[string]("manifestwork") + OwnerResourceTypeKey = hfl.NewKey[string]("owner_resource_type") + OwnerResourceIDKey = hfl.NewKey[string]("owner_resource_id") +) + +// ContextFields returns the adapter-specific context fields to register with +// the shared logger handler via hfl.WithContextFields. +func ContextFields() []hfl.ContextField { + return []hfl.ContextField{ + hfl.StringField(EventIDKey), + hfl.StringField(K8sKindKey), + hfl.StringField(K8sNameKey), + hfl.StringField(K8sNamespaceKey), + hfl.FieldFromKey(ObservedGenerationKey, slog.Int64Value), + hfl.StringField(MaestroConsumerKey), + hfl.StringField(ManifestWorkKey), + hfl.StringField(OwnerResourceTypeKey), + hfl.StringField(OwnerResourceIDKey), + } +} + +// WithOTelTraceContext extracts OpenTelemetry trace context (trace_id, span_id) +// from the context and adds them via the shared logger's trace/span context +// helpers for distributed tracing correlation. +// If no active span exists, returns the context unchanged. +func WithOTelTraceContext(ctx context.Context) context.Context { + spanCtx := trace.SpanContextFromContext(ctx) + if !spanCtx.IsValid() { + return ctx + } + + if spanCtx.HasTraceID() { + ctx = hfl.WithTraceID(ctx, spanCtx.TraceID().String()) + } + if spanCtx.HasSpanID() { + ctx = hfl.WithSpanID(ctx, spanCtx.SpanID().String()) + } + + return ctx +} diff --git a/internal/logctx/logctx_test.go b/internal/logctx/logctx_test.go new file mode 100644 index 00000000..6973598c --- /dev/null +++ b/internal/logctx/logctx_test.go @@ -0,0 +1,397 @@ +package logctx + +import ( + "context" + "errors" + "io" + "log/slog" + "syscall" + "testing" + "time" + + apperrors "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/errors" + hfl "github.com/openshift-hyperfleet/hyperfleet-logger" + oteltrace "go.opentelemetry.io/otel/trace" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +// ----------------------------------------------------------------------------- +// ContextFields() +// ----------------------------------------------------------------------------- + +func TestContextFields(t *testing.T) { + fields := ContextFields() + + wantNames := []string{ + "event_id", + "k8s_kind", + "k8s_name", + "k8s_namespace", + "observed_generation", + "maestro_consumer", + "manifestwork", + "owner_resource_type", + "owner_resource_id", + } + + if len(fields) != len(wantNames) { + t.Fatalf("expected %d context fields, got %d", len(wantNames), len(fields)) + } + for i, name := range wantNames { + if fields[i].Name != name { + t.Errorf("field %d: expected name %q, got %q", i, name, fields[i].Name) + } + } +} + +func TestContextFieldRoundTrip(t *testing.T) { + ctx := context.Background() + ctx = hfl.Set(ctx, EventIDKey, "evt-1") + ctx = hfl.Set(ctx, K8sKindKey, "Deployment") + ctx = hfl.Set(ctx, K8sNameKey, "my-app") + ctx = hfl.Set(ctx, K8sNamespaceKey, "default") + ctx = hfl.Set(ctx, ObservedGenerationKey, int64(42)) + ctx = hfl.Set(ctx, MaestroConsumerKey, "consumer-1") + ctx = hfl.Set(ctx, ManifestWorkKey, "mw-1") + ctx = hfl.Set(ctx, OwnerResourceTypeKey, "Cluster") + ctx = hfl.Set(ctx, OwnerResourceIDKey, "cluster-1") + + if v, ok := hfl.Get(ctx, EventIDKey); !ok || v != "evt-1" { + t.Errorf("EventIDKey: got %q, ok=%v", v, ok) + } + if v, ok := hfl.Get(ctx, K8sKindKey); !ok || v != "Deployment" { + t.Errorf("K8sKindKey: got %q, ok=%v", v, ok) + } + if v, ok := hfl.Get(ctx, K8sNameKey); !ok || v != "my-app" { + t.Errorf("K8sNameKey: got %q, ok=%v", v, ok) + } + if v, ok := hfl.Get(ctx, K8sNamespaceKey); !ok || v != "default" { + t.Errorf("K8sNamespaceKey: got %q, ok=%v", v, ok) + } + if v, ok := hfl.Get(ctx, ObservedGenerationKey); !ok || v != int64(42) { + t.Errorf("ObservedGenerationKey: got %d, ok=%v", v, ok) + } + if v, ok := hfl.Get(ctx, MaestroConsumerKey); !ok || v != "consumer-1" { + t.Errorf("MaestroConsumerKey: got %q, ok=%v", v, ok) + } + if v, ok := hfl.Get(ctx, ManifestWorkKey); !ok || v != "mw-1" { + t.Errorf("ManifestWorkKey: got %q, ok=%v", v, ok) + } + if v, ok := hfl.Get(ctx, OwnerResourceTypeKey); !ok || v != "Cluster" { + t.Errorf("OwnerResourceTypeKey: got %q, ok=%v", v, ok) + } + if v, ok := hfl.Get(ctx, OwnerResourceIDKey); !ok || v != "cluster-1" { + t.Errorf("OwnerResourceIDKey: got %q, ok=%v", v, ok) + } +} + +// ----------------------------------------------------------------------------- +// WithOTelTraceContext() +// ----------------------------------------------------------------------------- + +func TestWithOTelTraceContextNoOpWithoutActiveSpan(t *testing.T) { + ctx := context.Background() + + got := WithOTelTraceContext(ctx) + + if _, ok := hfl.TraceIDFromContext(got); ok { + t.Errorf("expected no trace_id to be set on a context with no active span") + } + if _, ok := hfl.SpanIDFromContext(got); ok { + t.Errorf("expected no span_id to be set on a context with no active span") + } +} + +func TestWithOTelTraceContextSetsTraceAndSpanIDFromValidSpan(t *testing.T) { + traceID, err := oteltrace.TraceIDFromHex("4bf92f3577b34da6a3ce929d0e0e4736") + if err != nil { + t.Fatalf("failed to build trace ID: %v", err) + } + spanID, err := oteltrace.SpanIDFromHex("00f067aa0ba902b7") + if err != nil { + t.Fatalf("failed to build span ID: %v", err) + } + + spanCtx := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ + TraceID: traceID, + SpanID: spanID, + TraceFlags: oteltrace.FlagsSampled, + }) + if !spanCtx.IsValid() { + t.Fatalf("test setup: expected constructed span context to be valid") + } + + ctx := oteltrace.ContextWithSpanContext(context.Background(), spanCtx) + + got := WithOTelTraceContext(ctx) + + gotTraceID, ok := hfl.TraceIDFromContext(got) + if !ok { + t.Fatalf("expected trace_id to be set") + } + if gotTraceID != traceID.String() { + t.Errorf("expected trace_id %q, got %q", traceID.String(), gotTraceID) + } + + gotSpanID, ok := hfl.SpanIDFromContext(got) + if !ok { + t.Fatalf("expected span_id to be set") + } + if gotSpanID != spanID.String() { + t.Errorf("expected span_id %q, got %q", spanID.String(), gotSpanID) + } +} + +// ----------------------------------------------------------------------------- +// StackTraceFilter() +// ----------------------------------------------------------------------------- + +// recordWithError builds an slog.Record carrying err under the "error" attribute +// key, mirroring how the runtime calls slog.ErrorContext(ctx, "msg", "error", err). +func recordWithError(err error) slog.Record { + r := slog.NewRecord(time.Now(), slog.LevelError, "msg", 0) + r.AddAttrs(slog.Any("error", err)) + return r +} + +func TestStackTraceFilter(t *testing.T) { + groupResource := schema.GroupResource{Group: "", Resource: "pods"} + + tests := []struct { + record func() slog.Record + name string + expected bool + }{ + { + name: "no error attr present", + record: func() slog.Record { + return slog.NewRecord(time.Now(), slog.LevelError, "msg", 0) + }, + expected: false, + }, + { + name: "error attr holding a non-error value", + record: func() slog.Record { + r := slog.NewRecord(time.Now(), slog.LevelError, "msg", 0) + r.AddAttrs(slog.Any("error", "not an error")) + return r + }, + expected: false, + }, + { + name: "context.Canceled", + record: func() slog.Record { return recordWithError(context.Canceled) }, + expected: false, + }, + { + name: "wrapped context.Canceled", + record: func() slog.Record { return recordWithError(errors.Join(context.Canceled)) }, + expected: false, + }, + { + name: "context.DeadlineExceeded", + record: func() slog.Record { return recordWithError(context.DeadlineExceeded) }, + expected: false, + }, + { + name: "io.EOF", + record: func() slog.Record { return recordWithError(io.EOF) }, + expected: false, + }, + { + name: "network error (ECONNREFUSED)", + record: func() slog.Record { return recordWithError(syscall.ECONNREFUSED) }, + expected: false, + }, + { + name: "k8s NotFound", + record: func() slog.Record { + return recordWithError(apierrors.NewNotFound(groupResource, "my-pod")) + }, + expected: false, + }, + { + name: "k8s Conflict", + record: func() slog.Record { + return recordWithError(apierrors.NewConflict(groupResource, "my-pod", errors.New("conflict"))) + }, + expected: false, + }, + { + name: "k8s AlreadyExists", + record: func() slog.Record { + return recordWithError(apierrors.NewAlreadyExists(groupResource, "my-pod")) + }, + expected: false, + }, + { + name: "k8s Forbidden", + record: func() slog.Record { + return recordWithError(apierrors.NewForbidden(groupResource, "my-pod", errors.New("forbidden"))) + }, + expected: false, + }, + { + name: "k8s Unauthorized", + record: func() slog.Record { + return recordWithError(apierrors.NewUnauthorized("unauthorized")) + }, + expected: false, + }, + { + name: "k8s BadRequest", + record: func() slog.Record { + return recordWithError(apierrors.NewBadRequest("bad request")) + }, + expected: false, + }, + { + name: "k8s Gone", + record: func() slog.Record { + // Intentionally exercises the deprecated IsGone path, distinct from IsResourceExpired below. + return recordWithError(apierrors.NewGone("gone")) //nolint:staticcheck + }, + expected: false, + }, + { + name: "k8s ResourceExpired", + record: func() slog.Record { + return recordWithError(apierrors.NewResourceExpired("expired")) + }, + expected: false, + }, + { + name: "k8s ServiceUnavailable", + record: func() slog.Record { + return recordWithError(apierrors.NewServiceUnavailable("unavailable")) + }, + expected: false, + }, + { + name: "k8s Timeout", + record: func() slog.Record { + return recordWithError(apierrors.NewTimeoutError("timeout", 5)) + }, + expected: false, + }, + { + name: "k8s TooManyRequests", + record: func() slog.Record { + return recordWithError(apierrors.NewTooManyRequests("too many requests", 5)) + }, + expected: false, + }, + { + name: "APIError NotFound (404)", + record: func() slog.Record { + return recordWithError(apperrors.NewAPIError( + "GET", "http://x", 404, "404 Not Found", nil, 1, 0, errors.New("boom"))) + }, + expected: false, + }, + { + name: "APIError Unauthorized (401)", + record: func() slog.Record { + return recordWithError(apperrors.NewAPIError( + "GET", "http://x", 401, "401 Unauthorized", nil, 1, 0, errors.New("boom"))) + }, + expected: false, + }, + { + name: "APIError Forbidden (403)", + record: func() slog.Record { + return recordWithError(apperrors.NewAPIError( + "GET", "http://x", 403, "403 Forbidden", nil, 1, 0, errors.New("boom"))) + }, + expected: false, + }, + { + name: "APIError BadRequest (400)", + record: func() slog.Record { + return recordWithError(apperrors.NewAPIError( + "GET", "http://x", 400, "400 Bad Request", nil, 1, 0, errors.New("boom"))) + }, + expected: false, + }, + { + name: "APIError Conflict (409)", + record: func() slog.Record { + return recordWithError(apperrors.NewAPIError("GET", "http://x", 409, "409 Conflict", nil, 1, 0, errors.New("boom"))) + }, + expected: false, + }, + { + name: "APIError RateLimited (429)", + record: func() slog.Record { + return recordWithError(apperrors.NewAPIError( + "GET", "http://x", 429, "429 Too Many Requests", nil, 1, 0, errors.New("boom"))) + }, + expected: false, + }, + { + name: "APIError Timeout (408)", + record: func() slog.Record { + return recordWithError(apperrors.NewAPIError( + "GET", "http://x", 408, "408 Request Timeout", nil, 1, 0, errors.New("boom"))) + }, + expected: false, + }, + { + // Surprising but intentional: the old isExpectedAPIError classifies any + // 5xx as an expected/skip-worthy error via apiErr.IsServerError(). + name: "APIError ServerError (500) is treated as expected", + record: func() slog.Record { + return recordWithError(apperrors.NewAPIError( + "GET", "http://x", 500, "500 Internal Server Error", nil, 1, 0, errors.New("boom"))) + }, + expected: false, + }, + { + // An unclassified status code (not one of the specific 4xx codes handled, + // and not a 5xx) is the only escape hatch that still captures a trace. + name: "APIError with unclassified status code still captures trace", + record: func() slog.Record { + return recordWithError(apperrors.NewAPIError( + "GET", "http://x", 300, "300 Multiple Choices", nil, 1, 0, errors.New("boom"))) + }, + expected: true, + }, + { + name: "K8sResourceKeyNotFoundError", + record: func() slog.Record { + return recordWithError(apperrors.NewK8sResourceKeyNotFoundError("Secret", "ns", "name", "key")) + }, + expected: false, + }, + { + name: "K8sInvalidPathError", + record: func() slog.Record { + return recordWithError(apperrors.NewK8sInvalidPathError("Secret", "bad/path", "ns/name/key")) + }, + expected: false, + }, + { + name: "K8sResourceDataError", + record: func() slog.Record { + return recordWithError(apperrors.NewK8sResourceDataError( + "Secret", "ns", "name", "malformed data", errors.New("boom"))) + }, + expected: false, + }, + { + name: "unclassified plain error captures trace", + record: func() slog.Record { return recordWithError(errors.New("boom")) }, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := StackTraceFilter(context.Background(), tt.record()) + if got != tt.expected { + t.Errorf("expected %v, got %v", tt.expected, got) + } + }) + } +} diff --git a/pkg/logger/stack_trace.go b/internal/logctx/stack_trace.go similarity index 62% rename from pkg/logger/stack_trace.go rename to internal/logctx/stack_trace.go index b3a13f76..2af9b5f7 100644 --- a/pkg/logger/stack_trace.go +++ b/internal/logctx/stack_trace.go @@ -1,18 +1,17 @@ -package logger +package logctx import ( "context" "errors" - "fmt" "io" - "runtime" + "log/slog" apperrors "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" ) // ----------------------------------------------------------------------------- -// Stack Trace Capture +// Stack Trace Capture Filter // ----------------------------------------------------------------------------- // skipStackTraceCheckers is a list of functions that check if an error should skip stack trace capture. @@ -78,11 +77,26 @@ func isK8sResourceDataError(err error) bool { return errors.As(err, &k8sDataErr) } -// shouldCaptureStackTrace determines if a stack trace should be captured for the given error. -// Returns false for expected operational errors (high frequency, known causes) to avoid -// performance overhead during error storms. Returns true for unexpected errors that -// indicate bugs or require investigation. -func shouldCaptureStackTrace(err error) bool { +// StackTraceFilter determines whether a stack trace should be attached to the +// given log record. It is intended to be passed to hfl.WithStackTrace +// at handler construction time. +// +// It extracts the error from the record's "error" attribute (if any) and +// returns false for expected operational errors (high frequency, known +// causes) to avoid performance overhead during error storms. Returns true for +// unexpected errors that indicate bugs or require investigation. +func StackTraceFilter(_ context.Context, r slog.Record) bool { + var err error + r.Attrs(func(a slog.Attr) bool { + if a.Key == "error" { + if e, ok := a.Value.Any().(error); ok { + err = e + } + return false + } + return true + }) + if err == nil { return false } @@ -97,38 +111,3 @@ func shouldCaptureStackTrace(err error) bool { // Capture stack trace for unexpected/internal errors return true } - -// withStackTraceField returns a context with the stack trace set. -// If frames is nil or empty, returns the context unchanged. -func withStackTraceField(ctx context.Context, frames []string) context.Context { - if len(frames) == 0 { - return ctx - } - return WithLogField(ctx, StackTraceKey, frames) -} - -// CaptureStackTrace captures the current call stack and returns it as a slice of strings. -// Each string contains the file path, line number, and function name. -// The skip parameter specifies how many stack frames to skip: -// - skip=0 starts from the caller of CaptureStackTrace -// - skip=1 skips one additional level, etc. -func CaptureStackTrace(skip int) []string { - const maxFrames = 32 - pcs := make([]uintptr, maxFrames) - // +2 to skip runtime.Callers and CaptureStackTrace itself - n := runtime.Callers(skip+2, pcs) - if n == 0 { - return nil - } - - frames := runtime.CallersFrames(pcs[:n]) - var stack []string - for { - frame, more := frames.Next() - stack = append(stack, fmt.Sprintf("%s:%d %s", frame.File, frame.Line, frame.Function)) - if !more { - break - } - } - return stack -} diff --git a/internal/maestroclient/client.go b/internal/maestroclient/client.go index ab6d95fb..b2fac0bd 100644 --- a/internal/maestroclient/client.go +++ b/internal/maestroclient/client.go @@ -6,6 +6,7 @@ import ( "crypto/x509" "encoding/json" "fmt" + "log/slog" "net/http" "net/url" "os" @@ -13,12 +14,13 @@ import ( "strings" "time" + "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/logctx" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/manifest" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/transportclient" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/constants" apperrors "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/errors" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/version" + hfl "github.com/openshift-hyperfleet/hyperfleet-logger" "github.com/openshift-online/maestro/pkg/api/openapi" "github.com/openshift-online/maestro/pkg/client/cloudevents/grpcsource" "google.golang.org/grpc" @@ -45,7 +47,6 @@ type Client struct { workClient workv1client.WorkV1Interface maestroAPIClient *openapi.APIClient config *Config - log logger.Logger grpcOptions *grpcopts.GRPCOptions } @@ -112,8 +113,8 @@ type Config struct { // ClientCertFile: "/etc/maestro/certs/client.crt", // ClientKeyFile: "/etc/maestro/certs/client.key", // } -// client, err := NewMaestroClient(ctx, config, log) -func NewMaestroClient(ctx context.Context, config *Config, log logger.Logger) (*Client, error) { +// client, err := NewMaestroClient(ctx, config) +func NewMaestroClient(ctx context.Context, config *Config) (*Client, error) { if config == nil { return nil, apperrors.ConfigurationError("maestro config is required") } @@ -158,11 +159,10 @@ func NewMaestroClient(ctx context.Context, config *Config, log logger.Logger) (* serverHealthinessTimeout = DefaultServerHealthinessTimeout } - log.WithFields(map[string]interface{}{ - "maestroServer": config.MaestroServerAddr, - "grpcServer": config.GRPCServerAddr, - "sourceID": config.SourceID, - }).Info(ctx, "Creating Maestro client") + slog.InfoContext(ctx, "creating Maestro client", + "maestro_server", config.MaestroServerAddr, + "grpc_server", config.GRPCServerAddr, + "source_id", config.SourceID) // Create HTTP client with appropriate TLS configuration httpTransport, transportErr := createHTTPTransport(config) @@ -216,7 +216,7 @@ func NewMaestroClient(ctx context.Context, config *Config, log logger.Logger) (* // This returns a workv1client.WorkV1Interface with Kubernetes-style API workClient, err := grpcsource.NewMaestroGRPCSourceWorkClient( ctx, - newOCMLoggerAdapter(log), + newOCMLoggerAdapter(), maestroAPIClient, grpcOptions, config.SourceID, @@ -225,15 +225,12 @@ func NewMaestroClient(ctx context.Context, config *Config, log logger.Logger) (* return nil, apperrors.MaestroError("failed to create Maestro work client: %v", err) } - log.WithFields(map[string]interface{}{ - "sourceID": config.SourceID, - }).Info(ctx, "Maestro client created successfully") + slog.InfoContext(ctx, "maestro client created successfully", "source_id", config.SourceID) return &Client{ workClient: workClient, maestroAPIClient: maestroAPIClient, config: config, - log: log, grpcOptions: grpcOptions, }, nil } @@ -511,7 +508,7 @@ func (c *Client) ApplyResource( "set TransportContext.ConsumerName") } - ctx = logger.WithMaestroConsumer(ctx, consumerName) + ctx = hfl.Set(ctx, logctx.MaestroConsumerKey, consumerName) // Parse bytes into ManifestWork work, err := parseManifestWork(manifestBytes) @@ -551,7 +548,7 @@ func (c *Client) GetResource( return nil, apierrors.NewNotFound(gr, name) } - ctx = logger.WithMaestroConsumer(ctx, consumerName) + ctx = hfl.Set(ctx, logctx.MaestroConsumerKey, consumerName) // If the GVK is ManifestWork, get the ManifestWork object directly if gvk.Kind == constants.ManifestWorkKind && @@ -607,7 +604,7 @@ func (c *Client) DiscoverResources( return &unstructured.UnstructuredList{}, nil } - ctx = logger.WithMaestroConsumer(ctx, consumerName) + ctx = hfl.Set(ctx, logctx.MaestroConsumerKey, consumerName) // List all ManifestWorks for this consumer workList, err := c.ListManifestWorks(ctx, consumerName, "") diff --git a/internal/maestroclient/ocm_logger_adapter.go b/internal/maestroclient/ocm_logger_adapter.go index 0f4c6cea..a86537a0 100644 --- a/internal/maestroclient/ocm_logger_adapter.go +++ b/internal/maestroclient/ocm_logger_adapter.go @@ -2,83 +2,75 @@ package maestroclient import ( "context" + "fmt" + "log/slog" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-online/ocm-sdk-go/logging" ) // Ensure ocmLoggerAdapter implements the OCM SDK logging.Logger interface var _ logging.Logger = &ocmLoggerAdapter{} -// ocmLoggerAdapter adapts our logger.Logger interface to the OCM SDK logging.Logger interface. -// This allows using our logger with Maestro's grpcsource client. -type ocmLoggerAdapter struct { - log logger.Logger -} +// ocmLoggerAdapter bridges slog.Default() to the OCM SDK logging.Logger interface. +// This allows using the standard slog logger with Maestro's grpcsource client. +type ocmLoggerAdapter struct{} // newOCMLoggerAdapter creates a new OCM SDK compatible logger adapter -func newOCMLoggerAdapter(log logger.Logger) *ocmLoggerAdapter { - return &ocmLoggerAdapter{log: log} +func newOCMLoggerAdapter() *ocmLoggerAdapter { + return &ocmLoggerAdapter{} } // DebugEnabled returns true if the debug level is enabled. -// Always returns true - let the underlying logger filter. func (a *ocmLoggerAdapter) DebugEnabled() bool { - return true + return slog.Default().Enabled(context.Background(), slog.LevelDebug) } // InfoEnabled returns true if the information level is enabled. func (a *ocmLoggerAdapter) InfoEnabled() bool { - return true + return slog.Default().Enabled(context.Background(), slog.LevelInfo) } // WarnEnabled returns true if the warning level is enabled. func (a *ocmLoggerAdapter) WarnEnabled() bool { - return true + return slog.Default().Enabled(context.Background(), slog.LevelWarn) } // ErrorEnabled returns true if the error level is enabled. func (a *ocmLoggerAdapter) ErrorEnabled() bool { - return true + return slog.Default().Enabled(context.Background(), slog.LevelError) } -// Debug logs at debug level with formatting. -func (a *ocmLoggerAdapter) Debug(ctx context.Context, format string, args ...interface{}) { +// logf formats and logs a message at the given level, defaulting to a +// background context when the OCM SDK passes a nil one. +func (a *ocmLoggerAdapter) logf(ctx context.Context, level slog.Level, format string, args ...interface{}) { if ctx == nil { ctx = context.Background() } - a.log.Debugf(ctx, format, args...) + slog.Log(ctx, level, fmt.Sprintf(format, args...)) +} + +// Debug logs at debug level with formatting. +func (a *ocmLoggerAdapter) Debug(ctx context.Context, format string, args ...interface{}) { + a.logf(ctx, slog.LevelDebug, format, args...) } // Info logs at info level with formatting. func (a *ocmLoggerAdapter) Info(ctx context.Context, format string, args ...interface{}) { - if ctx == nil { - ctx = context.Background() - } - a.log.Infof(ctx, format, args...) + a.logf(ctx, slog.LevelInfo, format, args...) } // Warn logs at warn level with formatting. func (a *ocmLoggerAdapter) Warn(ctx context.Context, format string, args ...interface{}) { - if ctx == nil { - ctx = context.Background() - } - a.log.Warnf(ctx, format, args...) + a.logf(ctx, slog.LevelWarn, format, args...) } // Error logs at error level with formatting. func (a *ocmLoggerAdapter) Error(ctx context.Context, format string, args ...interface{}) { - if ctx == nil { - ctx = context.Background() - } - a.log.Errorf(ctx, format, args...) + a.logf(ctx, slog.LevelError, format, args...) } // Fatal logs at error level with formatting. -// Note: Does not exit - the underlying logger handles that behavior. +// Note: Does not exit - matches the current behavior of this adapter. func (a *ocmLoggerAdapter) Fatal(ctx context.Context, format string, args ...interface{}) { - if ctx == nil { - ctx = context.Background() - } - a.log.Errorf(ctx, "FATAL: "+format, args...) + a.logf(ctx, slog.LevelError, "FATAL: "+format, args...) } diff --git a/internal/maestroclient/operations.go b/internal/maestroclient/operations.go index 0aa24cb4..a9ab582c 100644 --- a/internal/maestroclient/operations.go +++ b/internal/maestroclient/operations.go @@ -3,13 +3,15 @@ package maestroclient import ( "context" "encoding/json" + "log/slog" "strings" "time" + "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/logctx" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/manifest" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/constants" apperrors "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/errors" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" + hfl "github.com/openshift-hyperfleet/hyperfleet-logger" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -22,6 +24,13 @@ import ( workv1 "open-cluster-management.io/api/work/v1" ) +// withManifestWorkLogCtx enriches ctx with the Maestro consumer and ManifestWork name +// log fields shared by every operations.go call site acting on a specific ManifestWork. +func withManifestWorkLogCtx(ctx context.Context, consumerName, workName string) context.Context { + ctx = hfl.Set(ctx, logctx.MaestroConsumerKey, consumerName) + return hfl.Set(ctx, logctx.ManifestWorkKey, workName) +} + const ( grpcRetryMaxAttempts = 3 grpcRetryBaseDelay = 1 * time.Second @@ -50,8 +59,7 @@ func (c *Client) retryOnTransientGRPC(ctx context.Context, fn func() error) erro if !isTransientGRPCError(lastErr) { return false, lastErr } - retryCtx := logger.WithErrorField(ctx, lastErr) - c.log.Warn(retryCtx, "transient gRPC error, retrying") + slog.WarnContext(ctx, "transient gRPC error, retrying", "error", lastErr) return false, nil }) if wait.Interrupted(waitErr) && lastErr != nil { @@ -89,9 +97,8 @@ func (c *Client) CreateManifestWork( } // Enrich context with common fields - ctx = logger.WithMaestroConsumer(ctx, consumerName) - ctx = logger.WithLogField(ctx, "manifestwork", work.Name) - ctx = logger.WithObservedGeneration(ctx, manifest.GetGeneration(work.ObjectMeta)) + ctx = withManifestWorkLogCtx(ctx, consumerName, work.Name) + ctx = hfl.Set(ctx, logctx.ObservedGenerationKey, manifest.GetGeneration(work.ObjectMeta)) // Set namespace to consumer name (required by Maestro) work.Namespace = consumerName @@ -120,8 +127,7 @@ func (c *Client) GetManifestWork( consumerName string, workName string, ) (*workv1.ManifestWork, error) { - ctx = logger.WithMaestroConsumer(ctx, consumerName) - ctx = logger.WithLogField(ctx, "manifestwork", workName) + ctx = withManifestWorkLogCtx(ctx, consumerName, workName) var work *workv1.ManifestWork err := c.retryOnTransientGRPC(ctx, func() error { @@ -147,8 +153,7 @@ func (c *Client) PatchManifestWork( workName string, patchData []byte, ) (*workv1.ManifestWork, error) { - ctx = logger.WithMaestroConsumer(ctx, consumerName) - ctx = logger.WithLogField(ctx, "manifestwork", workName) + ctx = withManifestWorkLogCtx(ctx, consumerName, workName) var patched *workv1.ManifestWork err := c.retryOnTransientGRPC(ctx, func() error { @@ -176,8 +181,7 @@ func (c *Client) DeleteManifestWork( consumerName string, workName string, ) error { - ctx = logger.WithMaestroConsumer(ctx, consumerName) - ctx = logger.WithLogField(ctx, "manifestwork", workName) + ctx = withManifestWorkLogCtx(ctx, consumerName, workName) err := c.workClient.ManifestWorks(consumerName).Delete(ctx, workName, metav1.DeleteOptions{}) if err != nil { @@ -198,7 +202,7 @@ func (c *Client) ListManifestWorks( consumerName string, labelSelector string, ) (*workv1.ManifestWorkList, error) { - ctx = logger.WithMaestroConsumer(ctx, consumerName) + ctx = hfl.Set(ctx, logctx.MaestroConsumerKey, consumerName) opts := metav1.ListOptions{} if labelSelector != "" { @@ -248,9 +252,8 @@ func (c *Client) ApplyManifestWork( newGeneration := manifest.GetGeneration(manifestWork.ObjectMeta) // Enrich context with common fields - ctx = logger.WithMaestroConsumer(ctx, consumerName) - ctx = logger.WithLogField(ctx, "manifestwork", manifestWork.Name) - ctx = logger.WithObservedGeneration(ctx, newGeneration) + ctx = withManifestWorkLogCtx(ctx, consumerName, manifestWork.Name) + ctx = hfl.Set(ctx, logctx.ObservedGenerationKey, newGeneration) // Check if ManifestWork exists existing, err := c.GetManifestWork(ctx, consumerName, manifestWork.Name) @@ -268,10 +271,9 @@ func (c *Client) ApplyManifestWork( // Compare generations to determine operation decision := manifest.CompareGenerations(newGeneration, existingGeneration, exists) - c.log.WithFields(map[string]interface{}{ - "operation": decision.Operation, - "reason": decision.Reason, - }).Debug(ctx, "Apply operation determined") + slog.DebugContext(ctx, "apply operation determined", + "operation", decision.Operation, + "reason", decision.Reason) // Execute operation based on comparison result switch decision.Operation { @@ -337,8 +339,7 @@ func (c *Client) DiscoverManifest( workName string, discovery manifest.Discovery, ) (*unstructured.UnstructuredList, error) { - ctx = logger.WithMaestroConsumer(ctx, consumerName) - ctx = logger.WithLogField(ctx, "manifestwork", workName) + ctx = withManifestWorkLogCtx(ctx, consumerName, workName) // Get the ManifestWork work, err := c.GetManifestWork(ctx, consumerName, workName) diff --git a/internal/maestroclient/operations_test.go b/internal/maestroclient/operations_test.go index bb441c7e..65ca8a43 100644 --- a/internal/maestroclient/operations_test.go +++ b/internal/maestroclient/operations_test.go @@ -12,7 +12,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/manifest" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/constants" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -259,7 +258,7 @@ func TestIsTransientGRPCError(t *testing.T) { } func TestRetryOnTransientGRPC(t *testing.T) { - client := &Client{log: logger.NewTestLogger()} + client := &Client{} ctx := context.Background() t.Run("succeeds first try", func(t *testing.T) { diff --git a/pkg/health/metrics.go b/pkg/health/metrics.go index 2c8df826..3954528b 100644 --- a/pkg/health/metrics.go +++ b/pkg/health/metrics.go @@ -2,10 +2,10 @@ package health import ( "context" + "log/slog" "net/http" "time" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" ) @@ -13,7 +13,6 @@ import ( // MetricsServer provides HTTP metrics endpoint for Prometheus. type MetricsServer struct { server *http.Server - log logger.Logger buildInfo *prometheus.GaugeVec upGauge prometheus.Gauge port string @@ -27,7 +26,7 @@ type MetricsConfig struct { } // NewMetricsServer creates a new metrics server with required HyperFleet metrics. -func NewMetricsServer(log logger.Logger, port string, cfg MetricsConfig) *MetricsServer { +func NewMetricsServer(port string, cfg MetricsConfig) *MetricsServer { // Create build_info metric per HyperFleet metrics standard buildInfo := prometheus.NewGaugeVec( prometheus.GaugeOpts{ @@ -63,7 +62,6 @@ func NewMetricsServer(log logger.Logger, port string, cfg MetricsConfig) *Metric mux.Handle("/metrics", promhttp.Handler()) return &MetricsServer{ - log: log, port: port, upGauge: upGauge, buildInfo: buildInfo, @@ -77,12 +75,11 @@ func NewMetricsServer(log logger.Logger, port string, cfg MetricsConfig) *Metric // Start starts the metrics server in a goroutine. func (s *MetricsServer) Start(ctx context.Context) error { - s.log.Infof(ctx, "Starting metrics server on port %s", s.port) + slog.InfoContext(ctx, "starting metrics server", "port", s.port) go func() { if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed { - errCtx := logger.WithErrorField(ctx, err) - s.log.Errorf(errCtx, "Metrics server error") + slog.ErrorContext(ctx, "metrics server error", "error", err) } }() @@ -91,7 +88,7 @@ func (s *MetricsServer) Start(ctx context.Context) error { // Shutdown gracefully shuts down the metrics server. func (s *MetricsServer) Shutdown(ctx context.Context) error { - s.log.Info(ctx, "Shutting down metrics server...") + slog.InfoContext(ctx, "shutting down metrics server...") // Set up to 0 during shutdown s.upGauge.Set(0) return s.server.Shutdown(ctx) diff --git a/pkg/health/server.go b/pkg/health/server.go index 35d437dc..62aca804 100644 --- a/pkg/health/server.go +++ b/pkg/health/server.go @@ -3,12 +3,11 @@ package health import ( "context" "encoding/json" + "log/slog" "net/http" "sync" "sync/atomic" "time" - - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" ) // CheckStatus represents the status of a single health check. @@ -36,7 +35,6 @@ type ReadyResponse struct { // Server provides HTTP health check endpoints. type Server struct { - log logger.Logger server *http.Server checks map[string]CheckStatus port string @@ -50,9 +48,8 @@ type Server struct { } // NewServer creates a new health check server. -func NewServer(log logger.Logger, port string, component string) *Server { +func NewServer(port string, component string) *Server { s := &Server{ - log: log, port: port, component: component, checks: map[string]CheckStatus{ @@ -77,12 +74,11 @@ func NewServer(log logger.Logger, port string, component string) *Server { // Start starts the health server in a goroutine. func (s *Server) Start(ctx context.Context) error { - s.log.Infof(ctx, "Starting health server on port %s", s.port) + slog.InfoContext(ctx, "starting health server", "port", s.port) go func() { if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed { - errCtx := logger.WithErrorField(ctx, err) - s.log.Errorf(errCtx, "Health server error") + slog.ErrorContext(ctx, "health server error", "error", err) } }() @@ -91,7 +87,7 @@ func (s *Server) Start(ctx context.Context) error { // Shutdown gracefully shuts down the health server. func (s *Server) Shutdown(ctx context.Context) error { - s.log.Info(ctx, "Shutting down health server...") + slog.InfoContext(ctx, "shutting down health server...") return s.server.Shutdown(ctx) } diff --git a/pkg/health/server_test.go b/pkg/health/server_test.go index 58b3d902..afc160ed 100644 --- a/pkg/health/server_test.go +++ b/pkg/health/server_test.go @@ -8,29 +8,12 @@ import ( "testing" "time" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// mockLogger implements logger.Logger for testing -type mockLogger struct{} - -func (m *mockLogger) Debug(ctx context.Context, msg string) {} -func (m *mockLogger) Debugf(ctx context.Context, format string, args ...interface{}) {} -func (m *mockLogger) Info(ctx context.Context, msg string) {} -func (m *mockLogger) Infof(ctx context.Context, format string, args ...interface{}) {} -func (m *mockLogger) Warn(ctx context.Context, msg string) {} -func (m *mockLogger) Warnf(ctx context.Context, format string, args ...interface{}) {} -func (m *mockLogger) Error(ctx context.Context, msg string) {} -func (m *mockLogger) Errorf(ctx context.Context, format string, args ...interface{}) {} -func (m *mockLogger) Fatal(ctx context.Context, msg string) {} -func (m *mockLogger) With(key string, value interface{}) logger.Logger { return m } -func (m *mockLogger) WithFields(fields map[string]interface{}) logger.Logger { return m } -func (m *mockLogger) Without(key string) logger.Logger { return m } - func TestHealthzHandler(t *testing.T) { - server := NewServer(&mockLogger{}, "8080", "test-adapter") + server := NewServer("8080", "test-adapter") req := httptest.NewRequest(http.MethodGet, "/healthz", nil) w := httptest.NewRecorder() @@ -51,7 +34,7 @@ func TestHealthzHandler(t *testing.T) { } func TestReadyzHandler_NotReady(t *testing.T) { - server := NewServer(&mockLogger{}, "8080", "test-adapter") + server := NewServer("8080", "test-adapter") // By default, checks are in error state req := httptest.NewRequest(http.MethodGet, "/readyz", nil) @@ -76,7 +59,7 @@ func TestReadyzHandler_NotReady(t *testing.T) { } func TestReadyzHandler_Ready(t *testing.T) { - server := NewServer(&mockLogger{}, "8080", "test-adapter") + server := NewServer("8080", "test-adapter") server.SetConfigLoaded() server.SetBrokerReady(true) @@ -102,7 +85,7 @@ func TestReadyzHandler_Ready(t *testing.T) { } func TestSetBrokerReady(t *testing.T) { - server := NewServer(&mockLogger{}, "8080", "test-adapter") + server := NewServer("8080", "test-adapter") // Initially not ready (both checks are error) assert.False(t, server.IsReady()) @@ -121,7 +104,7 @@ func TestSetBrokerReady(t *testing.T) { } func TestSetCheck(t *testing.T) { - server := NewServer(&mockLogger{}, "8080", "test-adapter") + server := NewServer("8080", "test-adapter") // Set a custom check server.SetCheck("custom", CheckOK) @@ -145,7 +128,7 @@ func TestSetCheck(t *testing.T) { } func TestReadyzHandler_PartialReady(t *testing.T) { - server := NewServer(&mockLogger{}, "8080", "test-adapter") + server := NewServer("8080", "test-adapter") // Only config is loaded, broker is not ready server.SetConfigLoaded() @@ -168,7 +151,7 @@ func TestReadyzHandler_PartialReady(t *testing.T) { } func TestReadyzHandler_ReadyToNotReady(t *testing.T) { - server := NewServer(&mockLogger{}, "8080", "test-adapter") + server := NewServer("8080", "test-adapter") // Set all checks to ok server.SetConfigLoaded() @@ -199,7 +182,7 @@ func TestReadyzHandler_ReadyToNotReady(t *testing.T) { } func TestSetShuttingDown(t *testing.T) { - server := NewServer(&mockLogger{}, "8080", "test-adapter") + server := NewServer("8080", "test-adapter") // Initially not shutting down assert.False(t, server.IsShuttingDown()) @@ -214,7 +197,7 @@ func TestSetShuttingDown(t *testing.T) { } func TestReadyzHandler_ShuttingDown(t *testing.T) { - server := NewServer(&mockLogger{}, "8080", "test-adapter") + server := NewServer("8080", "test-adapter") // Set all checks to ok (server is ready) server.SetConfigLoaded() @@ -248,7 +231,7 @@ func TestReadyzHandler_ShuttingDown(t *testing.T) { } func TestIsReady_ShuttingDown(t *testing.T) { - server := NewServer(&mockLogger{}, "8080", "test-adapter") + server := NewServer("8080", "test-adapter") // Set all checks to ok server.SetConfigLoaded() @@ -261,7 +244,7 @@ func TestIsReady_ShuttingDown(t *testing.T) { } func TestReadyzHandler_ShuttingDownPriority(t *testing.T) { - server := NewServer(&mockLogger{}, "8080", "test-adapter") + server := NewServer("8080", "test-adapter") // Set all checks to ok server.SetConfigLoaded() @@ -287,7 +270,7 @@ func TestReadyzHandler_ShuttingDownPriority(t *testing.T) { func TestServerLifecycle_StartAndShutdown(t *testing.T) { // Use a unique port to avoid conflicts port := "18080" - server := NewServer(&mockLogger{}, port, "test-adapter") + server := NewServer(port, "test-adapter") ctx := context.Background() @@ -326,7 +309,7 @@ func TestServerLifecycle_StartAndShutdown(t *testing.T) { func TestServerLifecycle_ReadyzWhileRunning(t *testing.T) { port := "18081" - server := NewServer(&mockLogger{}, port, "test-adapter") + server := NewServer(port, "test-adapter") ctx := context.Background() @@ -363,7 +346,7 @@ func TestServerLifecycle_ReadyzWhileRunning(t *testing.T) { func TestServerLifecycle_GracefulShutdownStateTransitions(t *testing.T) { port := "18082" - server := NewServer(&mockLogger{}, port, "test-adapter") + server := NewServer(port, "test-adapter") ctx := context.Background() @@ -421,7 +404,7 @@ func TestServerLifecycle_GracefulShutdownStateTransitions(t *testing.T) { func TestServerLifecycle_ShutdownTimeout(t *testing.T) { port := "18083" - server := NewServer(&mockLogger{}, port, "test-adapter") + server := NewServer(port, "test-adapter") ctx := context.Background() diff --git a/pkg/logger/context.go b/pkg/logger/context.go deleted file mode 100644 index b04436d6..00000000 --- a/pkg/logger/context.go +++ /dev/null @@ -1,227 +0,0 @@ -package logger - -import ( - "context" - "strings" - - "go.opentelemetry.io/otel/trace" -) - -// contextKey is a custom type for context keys to avoid collisions -type contextKey string - -// Context keys for storing values in context.Context -const ( - LogFieldsKey contextKey = "log_fields" -) - -// Log field name constants - use these directly in WithFields maps -const ( - // Required fields (per logging spec) - ComponentKey = "component" - VersionKey = "version" - HostnameKey = "hostname" - - // Error fields (per logging spec) - ErrorKey = "error" - StackTraceKey = "stack_trace" - - // Correlation fields (distributed tracing) - TraceIDKey = "trace_id" - SpanIDKey = "span_id" - EventIDKey = "event_id" - - // Resource fields (from event data) - ResourceTypeKey = "resource_type" - - // K8s manifest fields - K8sKindKey = "k8s_kind" - K8sNameKey = "k8s_name" - K8sNamespaceKey = "k8s_namespace" - K8sResultKey = "k8s_result" - - // Adapter-specific fields - AdapterKey = "adapter" - ObservedGenerationKey = "observed_generation" - SubscriptionKey = "subscription" - - // Maestro-specific fields - MaestroConsumerKey = "maestro_consumer" -) - -// LogFields holds dynamic key-value pairs for logging -type LogFields map[string]interface{} - -// ----------------------------------------------------------------------------- -// Context Setters -// ----------------------------------------------------------------------------- - -// WithLogField adds a single dynamic log field to the context -// These fields will be extracted and included in all log entries -func WithLogField(ctx context.Context, key string, value interface{}) context.Context { - fields := GetLogFields(ctx) - if fields == nil { - fields = make(LogFields) - } - fields[key] = value - return context.WithValue(ctx, LogFieldsKey, fields) -} - -// WithLogFields adds multiple dynamic log fields to the context -// These fields will be extracted and included in all log entries -func WithLogFields(ctx context.Context, newFields LogFields) context.Context { - fields := GetLogFields(ctx) - if fields == nil { - fields = make(LogFields) - } - for k, v := range newFields { - fields[k] = v - } - return context.WithValue(ctx, LogFieldsKey, fields) -} - -// WithDynamicResourceID adds a resource ID as a dynamic log field -// The field name is derived from the resource type (e.g., "Cluster" -> "cluster_id", "NodePool" -> "nodepool_id") -func WithDynamicResourceID(ctx context.Context, resourceType string, resourceID string) context.Context { - fieldName := strings.ToLower(resourceType) + "_id" - return WithLogField(ctx, fieldName, resourceID) -} - -// WithTraceID returns a context with the trace ID set -func WithTraceID(ctx context.Context, traceID string) context.Context { - return WithLogField(ctx, TraceIDKey, traceID) -} - -// WithSpanID returns a context with the span ID set -func WithSpanID(ctx context.Context, spanID string) context.Context { - return WithLogField(ctx, SpanIDKey, spanID) -} - -// WithEventID returns a context with the event ID set -func WithEventID(ctx context.Context, eventID string) context.Context { - return WithLogField(ctx, EventIDKey, eventID) -} - -// WithResourceType returns a context with the event resource type set (e.g., "cluster", "nodepool") -func WithResourceType(ctx context.Context, resourceType string) context.Context { - return WithLogField(ctx, ResourceTypeKey, resourceType) -} - -// WithK8sKind returns a context with the K8s resource kind set (e.g., "Deployment", "Job") -func WithK8sKind(ctx context.Context, kind string) context.Context { - return WithLogField(ctx, K8sKindKey, kind) -} - -// WithK8sName returns a context with the K8s resource name set -func WithK8sName(ctx context.Context, name string) context.Context { - return WithLogField(ctx, K8sNameKey, name) -} - -// WithK8sNamespace returns a context with the K8s resource namespace set -func WithK8sNamespace(ctx context.Context, namespace string) context.Context { - return WithLogField(ctx, K8sNamespaceKey, namespace) -} - -// WithK8sResult returns a context with the K8s resource operation result set (SUCCESS/FAILED) -func WithK8sResult(ctx context.Context, result string) context.Context { - return WithLogField(ctx, K8sResultKey, result) -} - -// WithAdapter returns a context with the adapter name set -func WithAdapter(ctx context.Context, adapter string) context.Context { - return WithLogField(ctx, AdapterKey, adapter) -} - -// WithObservedGeneration returns a context with the observed generation set -func WithObservedGeneration(ctx context.Context, generation int64) context.Context { - return WithLogField(ctx, ObservedGenerationKey, generation) -} - -// WithSubscription returns a context with the subscription name set -func WithSubscription(ctx context.Context, subscription string) context.Context { - return WithLogField(ctx, SubscriptionKey, subscription) -} - -// WithMaestroConsumer returns a context with the Maestro consumer name set -func WithMaestroConsumer(ctx context.Context, consumer string) context.Context { - return WithLogField(ctx, MaestroConsumerKey, consumer) -} - -// WithErrorField returns a context with the error message set. -// Stack traces are captured only for unexpected/internal errors to avoid -// performance overhead under high event load. Expected operational errors -// (network issues, not found, auth failures) skip stack trace capture. -// If err is nil, returns the context unchanged. -func WithErrorField(ctx context.Context, err error) context.Context { - if err == nil { - return ctx - } - ctx = WithLogField(ctx, ErrorKey, err.Error()) - - // Only capture stack trace for unexpected/internal errors - if shouldCaptureStackTrace(err) { - ctx = withStackTraceField(ctx, CaptureStackTrace(1)) - } - - return ctx -} - -// WithOTelTraceContext extracts OpenTelemetry trace context (trace_id, span_id) -// from the context and adds them as log fields for distributed tracing correlation. -// If no active span exists, returns the context unchanged. -// -// This function is safe to call multiple times (e.g., once per span creation). -// Since Go contexts are immutable, each call returns a new context with the -// current span's IDs. The parent function's context remains unchanged, so logs -// after a child span completes will correctly use the parent's span_id. -// -// Example flow: -// -// func Parent(ctx context.Context) { -// ctx, span := tracer.Start(ctx, "Parent") -// ctx = logger.WithOTelTraceContext(ctx) // span_id=A -// Child(ctx) // Child logs use span_id=B -// log.Info(ctx, "Back in parent") // Still uses span_id=A -// } -// -// This will produce logs with trace_id and span_id fields: -// -// {"message":"...","trace_id":"4bf92f3577b34da6a3ce929d0e0e4736","span_id":"00f067aa0ba902b7",...} -func WithOTelTraceContext(ctx context.Context) context.Context { - spanCtx := trace.SpanContextFromContext(ctx) - if !spanCtx.IsValid() { - return ctx - } - - // Add trace_id if valid - if spanCtx.HasTraceID() { - ctx = WithLogField(ctx, TraceIDKey, spanCtx.TraceID().String()) - } - - // Add span_id if valid - if spanCtx.HasSpanID() { - ctx = WithLogField(ctx, SpanIDKey, spanCtx.SpanID().String()) - } - - return ctx -} - -// ----------------------------------------------------------------------------- -// Context Getters -// ----------------------------------------------------------------------------- - -// GetLogFields returns the dynamic log fields from the context, or nil if not set -func GetLogFields(ctx context.Context) LogFields { - if ctx == nil { - return nil - } - if v, ok := ctx.Value(LogFieldsKey).(LogFields); ok { - // Return a copy to avoid mutation - fields := make(LogFields, len(v)) - for k, val := range v { - fields[k] = val - } - return fields - } - return nil -} diff --git a/pkg/logger/logger.go b/pkg/logger/logger.go deleted file mode 100644 index fc1ece0c..00000000 --- a/pkg/logger/logger.go +++ /dev/null @@ -1,303 +0,0 @@ -package logger - -import ( - "context" - "fmt" - "io" - "log/slog" - "os" - "strings" -) - -const ( - FormatJSON = "json" - FormatText = "text" -) - -// Logger is the interface for structured logging. -// Context is passed as a parameter to each method (aligned with sentinel pattern). -type Logger interface { - // Debug logs at debug level - Debug(ctx context.Context, message string) - // Debugf logs at debug level with formatting - Debugf(ctx context.Context, format string, args ...interface{}) - // Info logs at info level - Info(ctx context.Context, message string) - // Infof logs at info level with formatting - Infof(ctx context.Context, format string, args ...interface{}) - // Warn logs at warn level - Warn(ctx context.Context, message string) - // Warnf logs at warn level with formatting - Warnf(ctx context.Context, format string, args ...interface{}) - // Error logs at error level - Error(ctx context.Context, message string) - // Errorf logs at error level with formatting - Errorf(ctx context.Context, format string, args ...interface{}) - // Fatal logs at error level and exits - Fatal(ctx context.Context, message string) - - // With returns a new logger with additional fields - With(key string, value interface{}) Logger - // WithFields returns a new logger with multiple additional fields - WithFields(fields map[string]interface{}) Logger - // Without returns a new logger with the specified field removed - Without(key string) Logger -} - -var _ Logger = &logger{} - -// logger is the concrete implementation using log/slog -type logger struct { - slog *slog.Logger - fields map[string]interface{} - component string - version string - hostname string -} - -// Config holds logger configuration -type Config struct { - // Level is the minimum log level: "debug", "info", "warn", "error" - Level string - // Format is the output format: FormatText ("text") or FormatJSON ("json") - Format string - // Output is the output destination: "stdout", "stderr", or empty (defaults to stdout) - // Ignored if Writer is set. - Output string - // Writer is an optional custom io.Writer for log output. - // If set, Output is ignored. Useful for testing (e.g., bytes.Buffer). - Writer io.Writer - // Component is the component name (e.g., "adapter", "sentinel") - Component string - // Version is the component version - Version string -} - -// DefaultConfig returns a configuration with sensible defaults -func DefaultConfig() Config { - return Config{ - Level: "info", - Format: FormatJSON, - Output: "stdout", - Component: "adapter", - Version: "unknown", - } -} - -// ConfigFromEnv creates a Config from environment variables with defaults -func ConfigFromEnv() Config { - cfg := DefaultConfig() - - if level := os.Getenv("LOG_LEVEL"); level != "" { - cfg.Level = strings.ToLower(level) - } - if format := os.Getenv("LOG_FORMAT"); format != "" { - cfg.Format = strings.ToLower(format) - } - if output := os.Getenv("LOG_OUTPUT"); output != "" { - cfg.Output = output - } - - return cfg -} - -// NewLogger creates a new Logger with the given configuration -// Returns error if output is invalid (must be "stdout", "stderr", or empty) -func NewLogger(cfg Config) (Logger, error) { - // Determine output writer - var writer io.Writer - if cfg.Writer != nil { - // Use custom writer (e.g., for testing with bytes.Buffer) - writer = cfg.Writer - } else { - // Use Output string config - switch cfg.Output { - case "stdout", "": - writer = os.Stdout - case "stderr": - writer = os.Stderr - default: - return nil, fmt.Errorf("invalid log output %q: must be 'stdout', 'stderr', or empty", cfg.Output) - } - } - - // Parse log level - level := parseLevel(cfg.Level) - - // Create handler options - opts := &slog.HandlerOptions{ - Level: level, - // Add source location for error level only - AddSource: false, - } - - // Create handler based on format - var handler slog.Handler - switch strings.ToLower(cfg.Format) { - case FormatJSON: - handler = slog.NewJSONHandler(writer, opts) - case FormatText: - handler = slog.NewTextHandler(writer, opts) - default: - return nil, fmt.Errorf("invalid log format %q: must be %q or %q", cfg.Format, FormatJSON, FormatText) - } - - // Get hostname - hostname, _ := os.Hostname() //nolint:errcheck // fallback to alternatives below - if hostname == "" { - hostname = os.Getenv("POD_NAME") - } - if hostname == "" { - hostname = "unknown" - } - - // Create base logger with required fields (per logging spec) - slogLogger := slog.New(handler).With( - ComponentKey, cfg.Component, - VersionKey, cfg.Version, - HostnameKey, hostname, - ) - - return &logger{ - slog: slogLogger, - fields: make(map[string]interface{}), - component: cfg.Component, - version: cfg.Version, - hostname: hostname, - }, nil -} - -// parseLevel converts string level to slog.Level -func parseLevel(level string) slog.Level { - switch strings.ToLower(level) { - case "debug": - return slog.LevelDebug - case "warn", "warning": - return slog.LevelWarn - case "error": - return slog.LevelError - default: - return slog.LevelInfo - } -} - -// buildArgs builds the slog args from fields and context -func (l *logger) buildArgs(ctx context.Context) []any { - args := make([]any, 0, len(l.fields)*2+10) - - // Add fields from the logger - for k, v := range l.fields { - args = append(args, k, v) - } - - // Extract all log fields from context (flat structure) - if ctx != nil { - if logFields, ok := ctx.Value(LogFieldsKey).(LogFields); ok { - for k, v := range logFields { - args = append(args, k, v) - } - } - } - - return args -} - -// Debug logs at debug level -func (l *logger) Debug(ctx context.Context, message string) { - l.slog.DebugContext(ctx, message, l.buildArgs(ctx)...) -} - -// Debugf logs at debug level with formatting -func (l *logger) Debugf(ctx context.Context, format string, args ...interface{}) { - l.slog.DebugContext(ctx, fmt.Sprintf(format, args...), l.buildArgs(ctx)...) -} - -// Info logs at info level -func (l *logger) Info(ctx context.Context, message string) { - l.slog.InfoContext(ctx, message, l.buildArgs(ctx)...) -} - -// Infof logs at info level with formatting -func (l *logger) Infof(ctx context.Context, format string, args ...interface{}) { - l.slog.InfoContext(ctx, fmt.Sprintf(format, args...), l.buildArgs(ctx)...) -} - -// Warn logs at warn level -func (l *logger) Warn(ctx context.Context, message string) { - l.slog.WarnContext(ctx, message, l.buildArgs(ctx)...) -} - -// Warnf logs at warn level with formatting -func (l *logger) Warnf(ctx context.Context, format string, args ...interface{}) { - l.slog.WarnContext(ctx, fmt.Sprintf(format, args...), l.buildArgs(ctx)...) -} - -// Error logs at error level -func (l *logger) Error(ctx context.Context, message string) { - l.slog.ErrorContext(ctx, message, l.buildArgs(ctx)...) -} - -// Errorf logs at error level with formatting -func (l *logger) Errorf(ctx context.Context, format string, args ...interface{}) { - l.slog.ErrorContext(ctx, fmt.Sprintf(format, args...), l.buildArgs(ctx)...) -} - -// Fatal logs at error level and exits -func (l *logger) Fatal(ctx context.Context, message string) { - l.slog.ErrorContext(ctx, message, l.buildArgs(ctx)...) - os.Exit(1) -} - -// copyFields creates a shallow copy of the fields map -func copyFields(f map[string]interface{}) map[string]interface{} { - if f == nil { - return make(map[string]interface{}) - } - newFields := make(map[string]interface{}, len(f)) - for k, v := range f { - newFields[k] = v - } - return newFields -} - -// With returns a new logger with an additional field -func (l *logger) With(key string, value interface{}) Logger { - newFields := copyFields(l.fields) - newFields[key] = value - return &logger{ - slog: l.slog, - fields: newFields, - component: l.component, - version: l.version, - hostname: l.hostname, - } -} - -// WithFields returns a new logger with multiple additional fields -func (l *logger) WithFields(fields map[string]interface{}) Logger { - newFields := copyFields(l.fields) - for k, v := range fields { - newFields[k] = v - } - return &logger{ - slog: l.slog, - fields: newFields, - component: l.component, - version: l.version, - hostname: l.hostname, - } -} - -// Without returns a new logger with the specified field removed. -// If the field doesn't exist, returns a new logger with the same fields. -func (l *logger) Without(key string) Logger { - newFields := copyFields(l.fields) - delete(newFields, key) - return &logger{ - slog: l.slog, - fields: newFields, - component: l.component, - version: l.version, - hostname: l.hostname, - } -} diff --git a/pkg/logger/logger_test.go b/pkg/logger/logger_test.go deleted file mode 100644 index 2362cbc7..00000000 --- a/pkg/logger/logger_test.go +++ /dev/null @@ -1,455 +0,0 @@ -package logger - -import ( - "context" - "testing" -) - -func TestNewLogger(t *testing.T) { - tests := []struct { - name string - config Config - }{ - { - name: "create_logger_with_default_config", - config: DefaultConfig(), - }, - { - name: "create_logger_with_json_format", - config: Config{ - Level: "debug", - Format: FormatJSON, - Output: "stdout", - Component: "test-adapter", - Version: "v1.0.0", - }, - }, - { - name: "create_logger_with_text_format", - config: Config{ - Level: "info", - Format: FormatText, - Output: "stderr", - Component: "test-adapter", - Version: "v1.0.0", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - log, err := NewLogger(tt.config) - if err != nil { - t.Fatalf("NewLogger returned error: %v", err) - } - if log == nil { - t.Fatal("New returned nil") - } - - // Type assertion to check implementation - if _, ok := log.(*logger); !ok { - t.Error("New didn't return *logger type") - } - }) - } -} - -func TestNewLoggerInvalidOutput(t *testing.T) { - _, err := NewLogger(Config{ - Level: "info", - Format: FormatText, - Output: "invalid_output", - Component: "test", - Version: "v1.0.0", - }) - if err == nil { - t.Fatal("Expected error for invalid output, got nil") - } -} - -func TestNewLoggerInvalidFormat(t *testing.T) { - _, err := NewLogger(Config{ - Level: "info", - Format: "yaml", - Output: "stdout", - Component: "test", - Version: "v1.0.0", - }) - if err == nil { - t.Fatal("Expected error for invalid format, got nil") - } -} - -func TestLoggerWith(t *testing.T) { - log, err := NewLogger(DefaultConfig()) - if err != nil { - t.Fatalf("NewLogger returned error: %v", err) - } - - tests := []struct { - value interface{} - name string - key string - }{ - { - name: "add_string_field", - key: "request_id", - value: "12345", - }, - { - name: "add_int_field", - key: "status_code", - value: 200, - }, - { - name: "add_bool_field", - key: "success", - value: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := log.With(tt.key, tt.value) - if result == nil { - t.Fatal("With() returned nil") - } - - // Verify it returns a Logger - impl, ok := result.(*logger) - if !ok { - t.Error("With() didn't return *logger type") - } - - // Verify the field was added - if impl.fields[tt.key] != tt.value { - t.Errorf("Expected field %s=%v, got %v", tt.key, tt.value, impl.fields[tt.key]) - } - }) - } -} - -func TestLoggerWithFields(t *testing.T) { - log, err := NewLogger(DefaultConfig()) - if err != nil { - t.Fatalf("NewLogger returned error: %v", err) - } - - fields := map[string]interface{}{ - "cluster_id": "cls-123", - "event_id": "evt-456", - "count": 42, - } - - result := log.WithFields(fields) - if result == nil { - t.Fatal("WithFields() returned nil") - } - - impl, ok := result.(*logger) - if !ok { - t.Error("WithFields() didn't return *logger type") - } - - for k, v := range fields { - if impl.fields[k] != v { - t.Errorf("Expected field %s=%v, got %v", k, v, impl.fields[k]) - } - } -} - -type testError struct { - msg string -} - -func (e *testError) Error() string { - return e.msg -} - -func TestLoggerMethods(t *testing.T) { - // These tests verify the methods don't panic - log, err := NewLogger(Config{ - Level: "debug", // Enable all levels - Format: FormatText, - Output: "stdout", - Component: "test", - Version: "v1.0.0", - }) - if err != nil { - t.Fatalf("NewLogger returned error: %v", err) - } - ctx := context.Background() - - t.Run("Debug_does_not_panic", func(t *testing.T) { - defer func() { - if r := recover(); r != nil { - t.Errorf("Debug panicked: %v", r) - } - }() - log.Debug(ctx, "Test debug message") - }) - - t.Run("Debugf_does_not_panic", func(t *testing.T) { - defer func() { - if r := recover(); r != nil { - t.Errorf("Debugf panicked: %v", r) - } - }() - log.Debugf(ctx, "Test debug: %s", "value") - }) - - t.Run("Info_does_not_panic", func(t *testing.T) { - defer func() { - if r := recover(); r != nil { - t.Errorf("Info panicked: %v", r) - } - }() - log.Info(ctx, "Test info message") - }) - - t.Run("Infof_does_not_panic", func(t *testing.T) { - defer func() { - if r := recover(); r != nil { - t.Errorf("Infof panicked: %v", r) - } - }() - log.Infof(ctx, "Test info: %s", "value") - }) - - t.Run("Warn_does_not_panic", func(t *testing.T) { - defer func() { - if r := recover(); r != nil { - t.Errorf("Warn panicked: %v", r) - } - }() - log.Warn(ctx, "Test warning") - }) - - t.Run("Warnf_does_not_panic", func(t *testing.T) { - defer func() { - if r := recover(); r != nil { - t.Errorf("Warnf panicked: %v", r) - } - }() - log.Warnf(ctx, "Test warning: %s", "value") - }) - - t.Run("Error_does_not_panic", func(t *testing.T) { - defer func() { - if r := recover(); r != nil { - t.Errorf("Error panicked: %v", r) - } - }() - log.Error(ctx, "Test error") - }) - - t.Run("Errorf_does_not_panic", func(t *testing.T) { - defer func() { - if r := recover(); r != nil { - t.Errorf("Errorf panicked: %v", r) - } - }() - log.Errorf(ctx, "Test error: %s", "value") - }) -} - -func TestLoggerChaining(t *testing.T) { - log, err := NewLogger(DefaultConfig()) - if err != nil { - t.Fatalf("NewLogger returned error: %v", err) - } - ctx := context.Background() - - t.Run("chain_With_multiple_times", func(t *testing.T) { - defer func() { - if r := recover(); r != nil { - t.Errorf("Chaining panicked: %v", r) - } - }() - - log.With("key1", "value1").With("key2", "value2").Info(ctx, "Test chaining") - }) - - t.Run("chain_WithFields_and_With", func(t *testing.T) { - defer func() { - if r := recover(); r != nil { - t.Errorf("Chaining panicked: %v", r) - } - }() - - log.WithFields(map[string]interface{}{"a": 1}).With("b", 2).Info(ctx, "Test mixed chaining") - }) - - t.Run("chain_WithErrorField", func(t *testing.T) { - defer func() { - if r := recover(); r != nil { - t.Errorf("Chaining panicked: %v", r) - } - }() - - err := &testError{msg: "test error"} - ctxWithErr := WithErrorField(ctx, err) - log.With("extra", "info").Error(ctxWithErr, "Error with context") - }) -} - -func TestFieldConstants(t *testing.T) { - tests := []struct { - name string - key string - expected string - }{ - { - name: "TraceIDKey", - key: TraceIDKey, - expected: "trace_id", - }, - { - name: "SpanIDKey", - key: SpanIDKey, - expected: "span_id", - }, - { - name: "EventIDKey", - key: EventIDKey, - expected: "event_id", - }, - { - name: "AdapterKey", - key: AdapterKey, - expected: "adapter", - }, - { - name: "SubscriptionKey", - key: SubscriptionKey, - expected: "subscription", - }, - { - name: "MaestroConsumerKey", - key: MaestroConsumerKey, - expected: "maestro_consumer", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if tt.key != tt.expected { - t.Errorf("Expected %s, got %s", tt.expected, tt.key) - } - }) - } -} - -func TestContextHelpers(t *testing.T) { - ctx := context.Background() - - t.Run("WithEventID", func(t *testing.T) { - ctxWithEvent := WithEventID(ctx, "evt-123") - fields := GetLogFields(ctxWithEvent) - if fields["event_id"] != "evt-123" { - t.Errorf("Expected evt-123, got %v", fields["event_id"]) - } - }) - - t.Run("WithTraceID", func(t *testing.T) { - ctxWithTrace := WithTraceID(ctx, "trace-789") - fields := GetLogFields(ctxWithTrace) - if fields["trace_id"] != "trace-789" { - t.Errorf("Expected trace-789, got %v", fields["trace_id"]) - } - }) -} - -func TestConfigFromEnv(t *testing.T) { - t.Run("defaults_without_env_vars", func(t *testing.T) { - cfg := ConfigFromEnv() - if cfg.Level != "info" { - t.Errorf("Expected default level 'info', got %s", cfg.Level) - } - if cfg.Format != FormatJSON { - t.Errorf("Expected default format %q, got %s", FormatJSON, cfg.Format) - } - if cfg.Output != "stdout" { - t.Errorf("Expected default output 'stdout', got %s", cfg.Output) - } - }) - - t.Run("reads_LOG_LEVEL_env_var", func(t *testing.T) { - t.Setenv("LOG_LEVEL", "DEBUG") - cfg := ConfigFromEnv() - if cfg.Level != "debug" { - t.Errorf("Expected level 'debug', got %s", cfg.Level) - } - }) - - t.Run("reads_LOG_FORMAT_env_var", func(t *testing.T) { - t.Setenv("LOG_FORMAT", "JSON") - cfg := ConfigFromEnv() - if cfg.Format != FormatJSON { - t.Errorf("Expected format %q, got %s", FormatJSON, cfg.Format) - } - }) - - t.Run("reads_LOG_OUTPUT_env_var", func(t *testing.T) { - t.Setenv("LOG_OUTPUT", "stderr") - cfg := ConfigFromEnv() - if cfg.Output != "stderr" { - t.Errorf("Expected output 'stderr', got %s", cfg.Output) - } - }) -} - -func TestParseLevel(t *testing.T) { - tests := []struct { - input string - expected string - }{ - {"debug", "DEBUG"}, - {"DEBUG", "DEBUG"}, - {"info", "INFO"}, - {"INFO", "INFO"}, - {"warn", "WARN"}, - {"warning", "WARN"}, - {"error", "ERROR"}, - {"ERROR", "ERROR"}, - {"invalid", "INFO"}, // Default to INFO - {"", "INFO"}, // Default to INFO - } - - for _, tt := range tests { - t.Run(tt.input, func(t *testing.T) { - level := parseLevel(tt.input) - if level.String() != tt.expected { - t.Errorf("parseLevel(%q) = %s, want %s", tt.input, level.String(), tt.expected) - } - }) - } -} - -func TestLoggerContextExtraction(t *testing.T) { - log, err := NewLogger(Config{ - Level: "debug", - Format: FormatText, - Output: "stdout", - Component: "test", - Version: "v1.0.0", - }) - if err != nil { - t.Fatalf("NewLogger returned error: %v", err) - } - - // Build context with various values - ctx := context.Background() - ctx = WithTraceID(ctx, "trace-123") - ctx = WithEventID(ctx, "evt-456") - - // This should not panic and should include context values in log - t.Run("logs_with_context_values", func(t *testing.T) { - defer func() { - if r := recover(); r != nil { - t.Errorf("Logging with context panicked: %v", r) - } - }() - log.Info(ctx, "Test message with context") - }) -} diff --git a/pkg/logger/test_support.go b/pkg/logger/test_support.go deleted file mode 100644 index ba5c9a8e..00000000 --- a/pkg/logger/test_support.go +++ /dev/null @@ -1,83 +0,0 @@ -package logger - -import ( - "bytes" - "strings" - "sync" -) - -var ( - testLoggerInstance Logger - testLoggerOnce sync.Once -) - -// NewTestLogger returns a shared logger instance suitable for tests. -// Uses singleton pattern to avoid creating multiple logger instances. -// Configured with "error" level to minimize noise during test runs. -func NewTestLogger() Logger { - testLoggerOnce.Do(func() { - var err error - testLoggerInstance, err = NewLogger(Config{ - Level: "error", - Format: "text", - Output: "stderr", - Component: "test", - Version: "test", - }) - if err != nil { - panic(err) - } - }) - return testLoggerInstance -} - -// LogCapture wraps a buffer to capture and inspect log output. -type LogCapture struct { - buf *bytes.Buffer - mu sync.Mutex -} - -// Messages returns all captured log output as a string. -func (c *LogCapture) Messages() string { - c.mu.Lock() - defer c.mu.Unlock() - return c.buf.String() -} - -// Contains checks if the captured log output contains the given substring. -func (c *LogCapture) Contains(substr string) bool { - return strings.Contains(c.Messages(), substr) -} - -// Reset clears all captured messages. -func (c *LogCapture) Reset() { - c.mu.Lock() - defer c.mu.Unlock() - c.buf.Reset() -} - -// NewCaptureLogger creates a real logger that writes to a buffer for testing. -// Returns the logger and a LogCapture to inspect the output. -// -// Example: -// -// log, capture := logger.NewCaptureLogger() -// // ... use log in your code ... -// if capture.Contains("expected message") { ... } -func NewCaptureLogger() (Logger, *LogCapture) { - buf := &bytes.Buffer{} - capture := &LogCapture{buf: buf} - - log, err := NewLogger(Config{ - Level: "debug", // Capture all levels - Format: "text", - Writer: buf, - Component: "test", - Version: "test", - }) - if err != nil { - panic(err) - } - - return log, capture -} diff --git a/pkg/logger/with_error_field_test.go b/pkg/logger/with_error_field_test.go deleted file mode 100644 index d505b6c5..00000000 --- a/pkg/logger/with_error_field_test.go +++ /dev/null @@ -1,498 +0,0 @@ -package logger - -import ( - "context" - "errors" - "fmt" - "io" - "strings" - "testing" - - apperrors "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/errors" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime/schema" -) - -// ----------------------------------------------------------------------------- -// WithErrorField Tests -// ----------------------------------------------------------------------------- - -func TestWithErrorField(t *testing.T) { - t.Run("nil_error_returns_unchanged_context", func(t *testing.T) { - ctx := context.Background() - result := WithErrorField(ctx, nil) - - fields := GetLogFields(result) - if fields != nil && fields["error"] != nil { - t.Error("Expected no error field for nil error") - } - }) - - t.Run("sets_error_message_in_context", func(t *testing.T) { - ctx := context.Background() - err := errors.New("test error message") - result := WithErrorField(ctx, err) - - fields := GetLogFields(result) - if fields == nil { - t.Fatal("Expected log fields, got nil") - } - if fields["error"] != "test error message" { - t.Errorf("Expected 'test error message', got %v", fields["error"]) - } - }) - - t.Run("captures_stack_trace_for_unexpected_error", func(t *testing.T) { - ctx := context.Background() - err := errors.New("unexpected internal error") - result := WithErrorField(ctx, err) - - fields := GetLogFields(result) - if fields == nil { - t.Fatal("Expected log fields, got nil") - } - - stackTrace, ok := fields["stack_trace"].([]string) - if !ok || len(stackTrace) == 0 { - t.Error("Expected stack_trace to be captured for unexpected error") - } - }) - - t.Run("preserves_existing_context_fields", func(t *testing.T) { - ctx := context.Background() - ctx = WithEventID(ctx, "evt-123") - - err := errors.New("test error") - result := WithErrorField(ctx, err) - - fields := GetLogFields(result) - if fields["error"] != "test error" { - t.Errorf("Expected error='test error', got %v", fields["error"]) - } - }) -} - -// ----------------------------------------------------------------------------- -// Stack Trace Capture Decision Tests -// ----------------------------------------------------------------------------- - -func TestShouldCaptureStackTrace_ContextErrors(t *testing.T) { - tests := []struct { - err error - name string - expectCapture bool - }{ - { - name: "context.Canceled_skips_stack_trace", - err: context.Canceled, - expectCapture: false, - }, - { - name: "context.DeadlineExceeded_skips_stack_trace", - err: context.DeadlineExceeded, - expectCapture: false, - }, - { - name: "wrapped_context.Canceled_skips_stack_trace", - err: fmt.Errorf("operation failed: %w", context.Canceled), - expectCapture: false, - }, - { - name: "io.EOF_skips_stack_trace", - err: io.EOF, - expectCapture: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := shouldCaptureStackTrace(tt.err) - if result != tt.expectCapture { - t.Errorf("shouldCaptureStackTrace() = %v, want %v", result, tt.expectCapture) - } - }) - } -} - -func TestShouldCaptureStackTrace_K8sAPIErrors(t *testing.T) { - tests := []struct { - err error - name string - expectCapture bool - }{ - { - name: "NotFound_skips_stack_trace", - err: apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "pods"}, "my-pod"), - expectCapture: false, - }, - { - name: "Conflict_skips_stack_trace", - err: apierrors.NewConflict( - schema.GroupResource{Group: "", Resource: "pods"}, "my-pod", errors.New("conflict"), - ), - expectCapture: false, - }, - { - name: "AlreadyExists_skips_stack_trace", - err: apierrors.NewAlreadyExists(schema.GroupResource{Group: "", Resource: "pods"}, "my-pod"), - expectCapture: false, - }, - { - name: "Forbidden_skips_stack_trace", - err: apierrors.NewForbidden( - schema.GroupResource{Group: "", Resource: "pods"}, "my-pod", errors.New("forbidden"), - ), - expectCapture: false, - }, - { - name: "Unauthorized_skips_stack_trace", - err: apierrors.NewUnauthorized("unauthorized"), - expectCapture: false, - }, - { - name: "BadRequest_skips_stack_trace", - err: apierrors.NewBadRequest("bad request"), - expectCapture: false, - }, - { - name: "ServiceUnavailable_skips_stack_trace", - err: apierrors.NewServiceUnavailable("service unavailable"), - expectCapture: false, - }, - { - name: "Timeout_skips_stack_trace", - err: apierrors.NewTimeoutError("timeout", 30), - expectCapture: false, - }, - { - name: "TooManyRequests_skips_stack_trace", - err: apierrors.NewTooManyRequestsError("too many requests"), - expectCapture: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := shouldCaptureStackTrace(tt.err) - if result != tt.expectCapture { - t.Errorf("shouldCaptureStackTrace() = %v, want %v", result, tt.expectCapture) - } - }) - } -} - -func TestShouldCaptureStackTrace_K8sResourceDataErrors(t *testing.T) { - tests := []struct { - err error - name string - expectCapture bool - }{ - { - name: "K8sResourceKeyNotFoundError_skips_stack_trace", - err: apperrors.NewK8sResourceKeyNotFoundError( - "Secret", "default", "my-secret", "password", - ), - expectCapture: false, - }, - { - name: "K8sInvalidPathError_skips_stack_trace", - err: apperrors.NewK8sInvalidPathError( - "Secret", "invalid/path", "namespace.name.key", - ), - expectCapture: false, - }, - { - name: "K8sResourceDataError_skips_stack_trace", - err: apperrors.NewK8sResourceDataError( - "ConfigMap", "default", "my-config", "data field missing", nil, - ), - expectCapture: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := shouldCaptureStackTrace(tt.err) - if result != tt.expectCapture { - t.Errorf("shouldCaptureStackTrace() = %v, want %v", result, tt.expectCapture) - } - }) - } -} - -func TestShouldCaptureStackTrace_APIErrors(t *testing.T) { - tests := []struct { - err error - name string - expectCapture bool - }{ - { - name: "APIError_NotFound_skips_stack_trace", - err: apperrors.NewAPIError( - "GET", "/api/v1/clusters/123", 404, "Not Found", nil, 1, 0, errors.New("not found"), - ), - expectCapture: false, - }, - { - name: "APIError_Unauthorized_skips_stack_trace", - err: apperrors.NewAPIError( - "GET", "/api/v1/clusters", 401, "Unauthorized", nil, 1, 0, errors.New("unauthorized"), - ), - expectCapture: false, - }, - { - name: "APIError_Forbidden_skips_stack_trace", - err: apperrors.NewAPIError( - "POST", "/api/v1/clusters", 403, "Forbidden", nil, 1, 0, errors.New("forbidden"), - ), - expectCapture: false, - }, - { - name: "APIError_BadRequest_skips_stack_trace", - err: apperrors.NewAPIError( - "POST", "/api/v1/clusters", 400, "Bad Request", nil, 1, 0, errors.New("bad request"), - ), - expectCapture: false, - }, - { - name: "APIError_Conflict_skips_stack_trace", - err: apperrors.NewAPIError( - "PUT", "/api/v1/clusters/123", 409, "Conflict", nil, 1, 0, errors.New("conflict"), - ), - expectCapture: false, - }, - { - name: "APIError_RateLimited_skips_stack_trace", - err: apperrors.NewAPIError( - "GET", "/api/v1/clusters", 429, "Too Many Requests", nil, 1, 0, errors.New("rate limited"), - ), - expectCapture: false, - }, - { - name: "APIError_Timeout_skips_stack_trace", - err: apperrors.NewAPIError( - "GET", "/api/v1/clusters", 408, "Request Timeout", nil, 1, 0, errors.New("timeout"), - ), - expectCapture: false, - }, - { - name: "APIError_ServerError_skips_stack_trace", - err: apperrors.NewAPIError( - "GET", "/api/v1/clusters", 503, "Service Unavailable", nil, 3, 0, errors.New("server error"), - ), - expectCapture: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := shouldCaptureStackTrace(tt.err) - if result != tt.expectCapture { - t.Errorf("shouldCaptureStackTrace() = %v, want %v", result, tt.expectCapture) - } - }) - } -} - -func TestShouldCaptureStackTrace_UnexpectedErrors(t *testing.T) { - tests := []struct { - err error - name string - expectCapture bool - }{ - { - name: "generic_error_captures_stack_trace", - err: errors.New("unexpected error"), - expectCapture: true, - }, - { - name: "wrapped_generic_error_captures_stack_trace", - err: fmt.Errorf("failed to process: %w", errors.New("internal error")), - expectCapture: true, - }, - { - name: "nil_error_does_not_capture", - err: nil, - expectCapture: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := shouldCaptureStackTrace(tt.err) - if result != tt.expectCapture { - t.Errorf("shouldCaptureStackTrace() = %v, want %v", result, tt.expectCapture) - } - }) - } -} - -// ----------------------------------------------------------------------------- -// CaptureStackTrace Tests -// ----------------------------------------------------------------------------- - -func TestCaptureStackTrace(t *testing.T) { - t.Run("captures_call_stack", func(t *testing.T) { - stack := CaptureStackTrace(0) - if len(stack) == 0 { - t.Fatal("Expected non-empty stack trace") - } - - // First frame should be from this test file - found := false - for _, frame := range stack { - if strings.Contains(frame, "with_error_field_test.go") { - found = true - break - } - } - if !found { - t.Error("Expected stack trace to contain with_error_field_test.go") - } - }) - - t.Run("skip_parameter_works", func(t *testing.T) { - stack0 := CaptureStackTrace(0) - stack1 := CaptureStackTrace(1) - - // Verify skip behavior: stack1 should either have fewer frames OR - // (when maxFrames cap is reached) be equal to stack0 with the first frame removed. - // This handles the case where CaptureStackTrace hits maxFrames limit. - if len(stack1) < len(stack0) { - // Normal case: skip=1 results in fewer frames - return - } - - if len(stack1) == len(stack0) && len(stack0) > 0 { - // maxFrames cap reached: verify stack1 equals stack0[1:] - // (the first frame was skipped, but a new frame was captured at the end) - for i := 0; i < len(stack1)-1; i++ { - if stack1[i] != stack0[i+1] { - t.Errorf( - "Expected stack1[%d] to equal stack0[%d], got %q vs %q", - i, i+1, stack1[i], stack0[i+1], - ) - } - } - return - } - - t.Errorf( - "Expected skip=1 to result in fewer frames or shifted stack, got len(stack0)=%d, len(stack1)=%d", - len(stack0), len(stack1), - ) - }) - - t.Run("frames_contain_file_line_function", func(t *testing.T) { - stack := CaptureStackTrace(0) - if len(stack) == 0 { - t.Fatal("Expected non-empty stack trace") - } - - // Each frame should have format "file:line function" - for _, frame := range stack { - if !strings.Contains(frame, ":") { - t.Errorf("Frame missing colon separator: %s", frame) - } - if !strings.Contains(frame, " ") { - t.Errorf("Frame missing space separator: %s", frame) - } - } - }) -} - -// ----------------------------------------------------------------------------- -// Integration Tests -// ----------------------------------------------------------------------------- - -func TestWithErrorField_StackTraceIntegration(t *testing.T) { - t.Run("unexpected_error_has_stack_trace", func(t *testing.T) { - ctx := context.Background() - err := errors.New("unexpected internal error") - result := WithErrorField(ctx, err) - - fields := GetLogFields(result) - if fields == nil { - t.Fatal("Expected log fields") - } - - // Should have error field - if fields["error"] != "unexpected internal error" { - t.Errorf("Expected error message, got %v", fields["error"]) - } - - // Should have stack_trace field - stackTrace, ok := fields["stack_trace"].([]string) - if !ok { - t.Fatal("Expected stack_trace to be []string") - } - if len(stackTrace) == 0 { - t.Error("Expected non-empty stack trace") - } - }) - - t.Run("k8s_not_found_error_no_stack_trace", func(t *testing.T) { - ctx := context.Background() - err := apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "pods"}, "my-pod") - result := WithErrorField(ctx, err) - - fields := GetLogFields(result) - if fields == nil { - t.Fatal("Expected log fields") - } - - // Should have error field - if fields["error"] == nil { - t.Error("Expected error field") - } - - // Should NOT have stack_trace field - if fields["stack_trace"] != nil { - t.Error("Expected no stack_trace for K8s NotFound error") - } - }) - - t.Run("context_canceled_no_stack_trace", func(t *testing.T) { - ctx := context.Background() - result := WithErrorField(ctx, context.Canceled) - - fields := GetLogFields(result) - if fields == nil { - t.Fatal("Expected log fields") - } - - // Should have error field - if fields["error"] == nil { - t.Error("Expected error field") - } - - // Should NOT have stack_trace field - if fields["stack_trace"] != nil { - t.Error("Expected no stack_trace for context.Canceled") - } - }) - - t.Run("api_error_server_error_no_stack_trace", func(t *testing.T) { - ctx := context.Background() - err := apperrors.NewAPIError( - "GET", "/api/v1/clusters", 500, "Internal Server Error", nil, 1, 0, - errors.New("server error"), - ) - result := WithErrorField(ctx, err) - - fields := GetLogFields(result) - if fields == nil { - t.Fatal("Expected log fields") - } - - // Should have error field - if fields["error"] == nil { - t.Error("Expected error field") - } - - // Should NOT have stack_trace field (server errors are expected) - if fields["stack_trace"] != nil { - t.Error("Expected no stack_trace for API server error") - } - }) -} diff --git a/pkg/telemetry/otel.go b/pkg/telemetry/otel.go index e264ea59..7b86b9e9 100644 --- a/pkg/telemetry/otel.go +++ b/pkg/telemetry/otel.go @@ -4,11 +4,11 @@ package telemetry import ( "context" "fmt" + "log/slog" "os" "strconv" "strings" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "go.opentelemetry.io/contrib/propagators/autoprop" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc" @@ -61,7 +61,7 @@ const ( // createExporter creates a SpanExporter based on OTLP environment variables. // When no endpoint is configured, returns a stdout exporter for local development. // The protocol defaults to grpc (per HyperFleet tracing standard), configurable via OTEL_EXPORTER_OTLP_PROTOCOL. -func createExporter(ctx context.Context, log logger.Logger) (sdktrace.SpanExporter, error) { +func createExporter(ctx context.Context) (sdktrace.SpanExporter, error) { // Check if an OTLP endpoint is configured (presence check only). // The actual endpoint value is read by the OTel SDK from env vars directly, // so we don't pass otlpEndpoint to the exporter constructors. @@ -70,8 +70,9 @@ func createExporter(ctx context.Context, log logger.Logger) (sdktrace.SpanExport otlpEndpoint = os.Getenv(envOtelExporterOtlpEndpoint) } if otlpEndpoint == "" { - log.Infof(ctx, "No %s or %s configured, using stdout exporter", - envOtelExporterOtlpTracesEndpoint, envOtelExporterOtlpEndpoint) + slog.InfoContext(ctx, "no otlp endpoint configured, using stdout exporter", + "traces_endpoint_var", envOtelExporterOtlpTracesEndpoint, + "endpoint_var", envOtelExporterOtlpEndpoint) return stdouttrace.New() } @@ -91,8 +92,8 @@ func createExporter(ctx context.Context, log logger.Logger) (sdktrace.SpanExport protocol = defaultOtlpProtocol exporter, err = otlptracegrpc.New(ctx) default: - log.Warnf(ctx, "Unrecognized %s value %q, using default %s", - protocolSource, protocol, defaultOtlpProtocol) + slog.WarnContext(ctx, "unrecognized protocol value, using default", + "var", protocolSource, "value", protocol, "default", defaultOtlpProtocol) protocol = defaultOtlpProtocol exporter, err = otlptracegrpc.New(ctx) } @@ -100,7 +101,7 @@ func createExporter(ctx context.Context, log logger.Logger) (sdktrace.SpanExport return nil, fmt.Errorf("failed to create OTLP exporter (protocol=%s): %w", protocol, err) } - log.Infof(ctx, "OTLP trace exporter configured: protocol=%s", protocol) + slog.InfoContext(ctx, "otlp trace exporter configured", "protocol", protocol) return exporter, nil } @@ -113,10 +114,10 @@ func createExporter(ctx context.Context, log logger.Logger) (sdktrace.SpanExport // - OTEL_TRACES_SAMPLER_ARG: sampling rate 0.0-1.0 (default: 1.0) // - OTEL_PROPAGATORS: list of propagators to use (default: "tracecontext,baggage") func InitTraceProvider( - ctx context.Context, log logger.Logger, serviceName, serviceVersion string, + ctx context.Context, serviceName, serviceVersion string, ) (*sdktrace.TracerProvider, error) { // Create exporter (nil when no OTLP endpoint configured) - exporter, err := createExporter(ctx, log) + exporter, err := createExporter(ctx) if err != nil { return nil, fmt.Errorf("failed to create trace exporter: %w", err) } @@ -135,7 +136,7 @@ func InitTraceProvider( if err != nil { if shutdownErr := exporter.Shutdown(ctx); shutdownErr != nil { - log.Warnf(ctx, "Failed to shutdown exporter during cleanup: %v", shutdownErr) + slog.WarnContext(ctx, "failed to shutdown exporter during cleanup", "error", shutdownErr) } return nil, fmt.Errorf("failed to create resource: %w", err) } @@ -144,7 +145,7 @@ func InitTraceProvider( // - If parent span exists: inherit parent's sampling decision // - If no parent (root span): apply probabilistic sampling based on trace ID // This enables proper sampling propagation across service boundaries - sampler := selectSampler(ctx, log) + sampler := selectSampler(ctx) tp := sdktrace.NewTracerProvider( sdktrace.WithBatcher(exporter), @@ -162,7 +163,7 @@ func InitTraceProvider( return tp, nil } -func selectSampler(ctx context.Context, log logger.Logger) sdktrace.Sampler { +func selectSampler(ctx context.Context) sdktrace.Sampler { samplerType := strings.ToLower(os.Getenv(envOtelTracesSampler)) switch samplerType { case samplerAlwaysOn: @@ -170,29 +171,29 @@ func selectSampler(ctx context.Context, log logger.Logger) sdktrace.Sampler { case samplerAlwaysOff: return sdktrace.NeverSample() case samplerTraceIDRatio: - return sdktrace.TraceIDRatioBased(parseSamplingRate(ctx, log)) + return sdktrace.TraceIDRatioBased(parseSamplingRate(ctx)) case parentBasedTraceIDRatio, "": - return sdktrace.ParentBased(sdktrace.TraceIDRatioBased(parseSamplingRate(ctx, log))) + return sdktrace.ParentBased(sdktrace.TraceIDRatioBased(parseSamplingRate(ctx))) case parentBasedAlwaysOn: return sdktrace.ParentBased(sdktrace.AlwaysSample()) case parentBasedAlwaysOff: return sdktrace.ParentBased(sdktrace.NeverSample()) default: - log.Warnf(ctx, "Unrecognized %s value %q, using default parentbased_traceidratio", - envOtelTracesSampler, samplerType) - return sdktrace.ParentBased(sdktrace.TraceIDRatioBased(parseSamplingRate(ctx, log))) + slog.WarnContext(ctx, "unrecognized sampler value, using default parentbased_traceidratio", + "var", envOtelTracesSampler, "value", samplerType) + return sdktrace.ParentBased(sdktrace.TraceIDRatioBased(parseSamplingRate(ctx))) } } -func parseSamplingRate(ctx context.Context, log logger.Logger) float64 { +func parseSamplingRate(ctx context.Context) float64 { rate := defaultSamplingRate if arg := os.Getenv(envOtelTracesSamplerArg); arg != "" { if parsedRate, err := strconv.ParseFloat(arg, 64); err == nil && parsedRate >= 0.0 && parsedRate <= 1.0 { rate = parsedRate } else { - log.Warnf(ctx, "Invalid %s value %q, using default %.1f", - envOtelTracesSamplerArg, arg, defaultSamplingRate) + slog.WarnContext(ctx, "invalid sampler arg value, using default", + "var", envOtelTracesSamplerArg, "value", arg, "default", defaultSamplingRate) } } return rate diff --git a/pkg/telemetry/otel_test.go b/pkg/telemetry/otel_test.go index c5bde595..3c755f88 100644 --- a/pkg/telemetry/otel_test.go +++ b/pkg/telemetry/otel_test.go @@ -4,17 +4,11 @@ import ( "context" "testing" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel" ) -func testLogger() logger.Logger { - log, _ := logger.NewLogger(logger.Config{Level: "error", Output: "stdout", Format: "json"}) - return log -} - // clearOtelEnv ensures all OTel env vars are cleared to prevent interference from the local shell environment. func clearOtelEnv(t *testing.T) { t.Setenv(envOtelExporterOtlpEndpoint, "") @@ -26,12 +20,11 @@ func clearOtelEnv(t *testing.T) { } func TestCreateExporter(t *testing.T) { - log := testLogger() ctx := context.Background() t.Run("stdout exporter when no endpoint set", func(t *testing.T) { clearOtelEnv(t) - exporter, err := createExporter(ctx, log) + exporter, err := createExporter(ctx) require.NoError(t, err) assert.NotNil(t, exporter) assert.NoError(t, exporter.Shutdown(ctx)) @@ -40,7 +33,7 @@ func TestCreateExporter(t *testing.T) { t.Run("grpc exporter when endpoint set with default protocol", func(t *testing.T) { clearOtelEnv(t) t.Setenv(envOtelExporterOtlpEndpoint, "http://localhost:4318") - exporter, err := createExporter(ctx, log) + exporter, err := createExporter(ctx) require.NoError(t, err) assert.NotNil(t, exporter) assert.NoError(t, exporter.Shutdown(ctx)) @@ -50,7 +43,7 @@ func TestCreateExporter(t *testing.T) { clearOtelEnv(t) t.Setenv(envOtelExporterOtlpEndpoint, "localhost:4317") t.Setenv(envOtelExporterOtlpProtocol, "grpc") - exporter, err := createExporter(ctx, log) + exporter, err := createExporter(ctx) require.NoError(t, err) assert.NotNil(t, exporter) assert.NoError(t, exporter.Shutdown(ctx)) @@ -60,7 +53,7 @@ func TestCreateExporter(t *testing.T) { clearOtelEnv(t) t.Setenv(envOtelExporterOtlpEndpoint, "http://localhost:4318") t.Setenv(envOtelExporterOtlpProtocol, "unknown-protocol") - exporter, err := createExporter(ctx, log) + exporter, err := createExporter(ctx) require.NoError(t, err) assert.NotNil(t, exporter) assert.NoError(t, exporter.Shutdown(ctx)) @@ -69,7 +62,7 @@ func TestCreateExporter(t *testing.T) { t.Run("traces-specific endpoint takes precedence", func(t *testing.T) { clearOtelEnv(t) t.Setenv(envOtelExporterOtlpTracesEndpoint, "http://localhost:4318") - exporter, err := createExporter(ctx, log) + exporter, err := createExporter(ctx) require.NoError(t, err) assert.NotNil(t, exporter) assert.NoError(t, exporter.Shutdown(ctx)) @@ -80,7 +73,7 @@ func TestCreateExporter(t *testing.T) { t.Setenv(envOtelExporterOtlpEndpoint, "http://localhost:4318") t.Setenv(envOtelExporterOtlpProtocol, "grpc") t.Setenv(envOtelExporterOtlpTracesProtocol, "http/protobuf") - exporter, err := createExporter(ctx, log) + exporter, err := createExporter(ctx) require.NoError(t, err) assert.NotNil(t, exporter) assert.NoError(t, exporter.Shutdown(ctx)) @@ -88,7 +81,6 @@ func TestCreateExporter(t *testing.T) { } func TestInitTraceProvider(t *testing.T) { - log := testLogger() ctx := context.Background() t.Run("initializes with stdout exporter when no endpoint", func(t *testing.T) { @@ -99,7 +91,7 @@ func TestInitTraceProvider(t *testing.T) { otel.SetTextMapPropagator(prevProp) }) clearOtelEnv(t) - tp, err := InitTraceProvider(ctx, log, "test-service", "0.0.1") + tp, err := InitTraceProvider(ctx, "test-service", "0.0.1") require.NoError(t, err) require.NotNil(t, tp) assert.NoError(t, tp.Shutdown(ctx)) @@ -113,7 +105,7 @@ func TestInitTraceProvider(t *testing.T) { }) clearOtelEnv(t) t.Setenv(envOtelExporterOtlpEndpoint, "http://localhost:4318") - tp, err := InitTraceProvider(ctx, log, "test-service", "0.0.1") + tp, err := InitTraceProvider(ctx, "test-service", "0.0.1") require.NoError(t, err) require.NotNil(t, tp) assert.NoError(t, tp.Shutdown(ctx)) @@ -121,7 +113,6 @@ func TestInitTraceProvider(t *testing.T) { } func TestSelectSampler(t *testing.T) { - log := testLogger() ctx := context.Background() tests := []struct { @@ -149,14 +140,13 @@ func TestSelectSampler(t *testing.T) { t.Setenv(envOtelTracesSamplerArg, tt.samplerArg) } - sampler := selectSampler(ctx, log) + sampler := selectSampler(ctx) assert.NotNil(t, sampler) }) } } func TestParseSamplingRate(t *testing.T) { - log := testLogger() ctx := context.Background() tests := []struct { @@ -180,14 +170,13 @@ func TestParseSamplingRate(t *testing.T) { t.Setenv(envOtelTracesSamplerArg, tt.envValue) } - rate := parseSamplingRate(ctx, log) + rate := parseSamplingRate(ctx) assert.Equal(t, tt.expected, rate) }) } } func TestInitTraceProvider_SamplerEnvironmentVariables(t *testing.T) { - log := testLogger() ctx := context.Background() tests := []struct { @@ -221,7 +210,7 @@ func TestInitTraceProvider_SamplerEnvironmentVariables(t *testing.T) { t.Setenv(envOtelTracesSamplerArg, tt.samplerArg) } - tp, err := InitTraceProvider(ctx, log, "test-service", "0.0.1") + tp, err := InitTraceProvider(ctx, "test-service", "0.0.1") require.NoError(t, err) defer func() { assert.NoError(t, tp.Shutdown(ctx)) }() diff --git a/test/integration/config-loader/config_criteria_integration_test.go b/test/integration/config-loader/config_criteria_integration_test.go index d74ab00f..d461a14f 100644 --- a/test/integration/config-loader/config_criteria_integration_test.go +++ b/test/integration/config-loader/config_criteria_integration_test.go @@ -13,7 +13,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/configloader" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/criteria" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" ) // TestMain sets up environment variables required by the adapter config template @@ -94,7 +93,7 @@ func TestConfigLoadAndCriteriaEvaluation(t *testing.T) { }, }) - evaluator, err := criteria.NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := criteria.NewEvaluator(context.Background(), ctx) require.NoError(t, err) t.Run("evaluate precondition conditions from config", func(t *testing.T) { @@ -154,7 +153,7 @@ func TestConfigWithFailingPreconditions(t *testing.T) { ctx := criteria.NewEvaluationContext() ctx.Set("reconciledConditionStatus", "True") - evaluator, err := criteria.NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := criteria.NewEvaluator(context.Background(), ctx) require.NoError(t, err) conditions := make([]criteria.ConditionDef, len(precond.Conditions)) for i, cond := range precond.Conditions { @@ -175,7 +174,7 @@ func TestConfigWithFailingPreconditions(t *testing.T) { ctx := criteria.NewEvaluationContext() ctx.Set("reconciledConditionStatus", "Unknown") // Not matching expected "True" - evaluator, err := criteria.NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := criteria.NewEvaluator(context.Background(), ctx) require.NoError(t, err) conditions := make([]criteria.ConditionDef, len(precond.Conditions)) for i, cond := range precond.Conditions { @@ -195,7 +194,7 @@ func TestConfigWithFailingPreconditions(t *testing.T) { ctx := criteria.NewEvaluationContext() // vpcId not set - should fail exists check - evaluator, err := criteria.NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := criteria.NewEvaluator(context.Background(), ctx) require.NoError(t, err) // Just check the vpcId exists condition (this is a general test, not tied to template) result, err := evaluator.EvaluateCondition("vpcId", criteria.OperatorExists, true) @@ -279,7 +278,7 @@ func TestConfigPostProcessingEvaluation(t *testing.T) { }, }) - evaluator, err := criteria.NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := criteria.NewEvaluator(context.Background(), ctx) require.NoError(t, err) // Test accessing nested K8s resource data t.Run("access namespace status", func(t *testing.T) { @@ -342,7 +341,7 @@ func TestConfigNullSafetyWithMissingResources(t *testing.T) { "clusterController": nil, // Not created yet }) - evaluator, err := criteria.NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := criteria.NewEvaluator(context.Background(), ctx) require.NoError(t, err) // ExtractValue returns nil value (not error) for null path - allows default to be used @@ -367,7 +366,7 @@ func TestConfigNullSafetyWithMissingResources(t *testing.T) { }, }) - evaluator, err := criteria.NewEvaluator(context.Background(), ctx, logger.NewTestLogger()) + evaluator, err := criteria.NewEvaluator(context.Background(), ctx) require.NoError(t, err) // Should return nil value (not error) for null status path diff --git a/test/integration/executor/executor_integration_test.go b/test/integration/executor/executor_integration_test.go index c3f0d552..22d1f370 100644 --- a/test/integration/executor/executor_integration_test.go +++ b/test/integration/executor/executor_integration_test.go @@ -1,8 +1,10 @@ package executorintegrationtest import ( + "bytes" "context" "encoding/json" + "log/slog" "net/http" "os" "strings" @@ -13,7 +15,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/configloader" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/executor" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/hyperfleetapi" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-adapter/test/integration/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -181,7 +182,7 @@ func TestExecutor_FullFlow_Success(t *testing.T) { // Create config and executor config := createTestConfig(mockAPI.URL()) - apiClient, err := hyperfleetapi.NewClient(testLog(), + apiClient, err := hyperfleetapi.NewClient( hyperfleetapi.WithTimeout(10*time.Second), hyperfleetapi.WithRetryAttempts(1), ) @@ -190,7 +191,6 @@ func TestExecutor_FullFlow_Success(t *testing.T) { exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(k8sEnv.Log). WithTransportClient(k8sEnv.Client). Build() if err != nil { @@ -319,12 +319,11 @@ func TestExecutor_PreconditionNotMet(t *testing.T) { // Create config and executor config := createTestConfig(mockAPI.URL()) - apiClient, err := hyperfleetapi.NewClient(k8sEnv.Log) + apiClient, err := hyperfleetapi.NewClient() assert.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(k8sEnv.Log). WithTransportClient(k8sEnv.Client). Build() if err != nil { @@ -419,7 +418,7 @@ func TestExecutor_PreconditionAPIFailure(t *testing.T) { // Create config and executor config := createTestConfig(mockAPI.URL()) - apiClient, err := hyperfleetapi.NewClient(testLog(), + apiClient, err := hyperfleetapi.NewClient( hyperfleetapi.WithRetryAttempts(1), ) assert.NoError(t, err) @@ -427,7 +426,6 @@ func TestExecutor_PreconditionAPIFailure(t *testing.T) { exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(k8sEnv.Log). WithTransportClient(k8sEnv.Client). Build() if err != nil { @@ -513,7 +511,7 @@ func setup404IntegrationTest(t *testing.T) (*testutil.MockAPIServer, *executor.E k8sEnv := getK8sEnvForTest(t) config := createTestConfig(mockAPI.URL()) - apiClient, err := hyperfleetapi.NewClient(testLog(), + apiClient, err := hyperfleetapi.NewClient( hyperfleetapi.WithRetryAttempts(1), ) require.NoError(t, err) @@ -521,7 +519,6 @@ func setup404IntegrationTest(t *testing.T) (*testutil.MockAPIServer, *executor.E exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(k8sEnv.Log). WithTransportClient(k8sEnv.Client). Build() require.NoError(t, err) @@ -607,14 +604,13 @@ func TestExecutor_CELExpressionEvaluation(t *testing.T) { }, } - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() assert.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(getK8sEnvForTest(t).Log). WithTransportClient(getK8sEnvForTest(t).Client). Build() if err != nil { @@ -655,12 +651,11 @@ func TestExecutor_MultipleMessages(t *testing.T) { t.Setenv("HYPERFLEET_API_VERSION", "v1") config := createTestConfig(mockAPI.URL()) - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() assert.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(getK8sEnvForTest(t).Log). WithTransportClient(getK8sEnvForTest(t).Client). Build() if err != nil { @@ -708,12 +703,11 @@ func TestExecutor_Handler_Integration(t *testing.T) { t.Setenv("HYPERFLEET_API_VERSION", "v1") config := createTestConfig(mockAPI.URL()) - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() assert.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(getK8sEnvForTest(t).Log). WithTransportClient(getK8sEnvForTest(t).Client). Build() if err != nil { @@ -721,7 +715,7 @@ func TestExecutor_Handler_Integration(t *testing.T) { } // Get the handler function - handler := executor.AlwaysAck(exec.CreateHandler(), logger.NewTestLogger()) + handler := executor.AlwaysAck(exec.CreateHandler()) // Simulate broker calling the handler evt := createTestEvent("cluster-handler-test") @@ -760,19 +754,18 @@ func TestExecutor_Handler_PreconditionNotMet_ReturnsNil(t *testing.T) { t.Setenv("HYPERFLEET_API_VERSION", "v1") config := createTestConfig(mockAPI.URL()) - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() assert.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(getK8sEnvForTest(t).Log). WithTransportClient(getK8sEnvForTest(t).Client). Build() if err != nil { t.Fatalf("Failed to create executor: %v", err) } - handler := executor.AlwaysAck(exec.CreateHandler(), logger.NewTestLogger()) + handler := executor.AlwaysAck(exec.CreateHandler()) evt := createTestEvent("cluster-skip") // Handler should return nil even when precondition not met @@ -793,12 +786,11 @@ func TestExecutor_ContextCancellation(t *testing.T) { t.Setenv("HYPERFLEET_API_VERSION", "v1") config := createTestConfig(mockAPI.URL()) - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() assert.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(getK8sEnvForTest(t).Log). WithTransportClient(getK8sEnvForTest(t).Client). Build() if err != nil { @@ -828,13 +820,12 @@ func TestExecutor_MissingRequiredParam(t *testing.T) { t.Setenv("HYPERFLEET_API_VERSION", "v1") config := createTestConfig(mockAPI.URL()) - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() assert.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(getK8sEnvForTest(t).Log). WithTransportClient(getK8sEnvForTest(t).Client). Build() if err != nil { @@ -875,7 +866,7 @@ func TestExecutor_MissingRequiredParam(t *testing.T) { } // Test handler behavior: should ACK (not NACK) invalid events - handler := executor.AlwaysAck(exec.CreateHandler(), logger.NewTestLogger()) + handler := executor.AlwaysAck(exec.CreateHandler()) err = handler(context.Background(), evt) if err != nil { t.Errorf("Handler should ACK (return nil) for param extraction failures, got error: %v", err) @@ -894,13 +885,12 @@ func TestExecutor_InvalidEventJSON(t *testing.T) { t.Setenv("HYPERFLEET_API_VERSION", "v1") config := createTestConfig(mockAPI.URL()) - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() assert.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(getK8sEnvForTest(t).Log). WithTransportClient(getK8sEnvForTest(t).Client). Build() if err != nil { @@ -931,7 +921,7 @@ func TestExecutor_InvalidEventJSON(t *testing.T) { assert.Empty(t, result.ResourceResults, "Resources should not execute for invalid event") // Test handler behavior: should ACK (not NACK) invalid events - handler := executor.AlwaysAck(exec.CreateHandler(), logger.NewTestLogger()) + handler := executor.AlwaysAck(exec.CreateHandler()) err = handler(context.Background(), &evt) assert.Nil(t, err, "Handler should ACK (return nil) for invalid events, not NACK") @@ -947,12 +937,11 @@ func TestExecutor_MissingEventFields(t *testing.T) { t.Setenv("HYPERFLEET_API_VERSION", "v1") config := createTestConfig(mockAPI.URL()) - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() assert.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(getK8sEnvForTest(t).Log). WithTransportClient(getK8sEnvForTest(t).Client). Build() if err != nil { @@ -989,7 +978,7 @@ func TestExecutor_MissingEventFields(t *testing.T) { assert.Empty(t, result.ResourceResults, "Resources should not execute for missing required field") // Test handler behavior: should ACK (not NACK) events with missing required fields - handler := executor.AlwaysAck(exec.CreateHandler(), logger.NewTestLogger()) + handler := executor.AlwaysAck(exec.CreateHandler()) errPhase = handler(context.Background(), &evt) assert.Nil(t, errPhase, "Handler should ACK (return nil) for missing required fields, not NACK") @@ -1004,8 +993,11 @@ func TestExecutor_LogAction(t *testing.T) { t.Setenv("HYPERFLEET_API_BASE_URL", mockAPI.URL()) t.Setenv("HYPERFLEET_API_VERSION", "v1") - // Create a logger that captures log messages for assertions - log, logCapture := logger.NewCaptureLogger() + // Capture log output via the process-global default logger for assertions + var logBuf bytes.Buffer + prevLogger := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logBuf, &slog.HandlerOptions{Level: slog.LevelDebug}))) + t.Cleanup(func() { slog.SetDefault(prevLogger) }) // Create config with log actions in preconditions and post-actions config := &configloader.Config{ @@ -1092,12 +1084,11 @@ func TestExecutor_LogAction(t *testing.T) { }, } - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() assert.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(log). WithTransportClient(getK8sEnvForTest(t).Client). Build() if err != nil { @@ -1114,7 +1105,7 @@ func TestExecutor_LogAction(t *testing.T) { } // Verify log messages were captured - capturedLogs := logCapture.Messages() + capturedLogs := logBuf.String() t.Logf("Captured logs:\n%s", capturedLogs) // Check for expected log messages (with [config] prefix) @@ -1126,7 +1117,7 @@ func TestExecutor_LogAction(t *testing.T) { } for _, expected := range expectedLogs { - if !logCapture.Contains(expected) { + if !strings.Contains(capturedLogs, expected) { t.Errorf("Expected log message not found: %s", expected) } } @@ -1159,14 +1150,13 @@ func TestExecutor_PostActionAPIFailure(t *testing.T) { // Create config and executor config := createTestConfig(mockAPI.URL()) - apiClient, err := hyperfleetapi.NewClient(testLog(), + apiClient, err := hyperfleetapi.NewClient( hyperfleetapi.WithRetryAttempts(1), ) assert.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(getK8sEnvForTest(t).Log). WithTransportClient(getK8sEnvForTest(t).Client). Build() if err != nil { @@ -1346,12 +1336,11 @@ func TestExecutor_ExecutionError_CELAccess(t *testing.T) { }, } - apiClient, err := hyperfleetapi.NewClient(testLog(), hyperfleetapi.WithRetryAttempts(1)) + apiClient, err := hyperfleetapi.NewClient(hyperfleetapi.WithRetryAttempts(1)) assert.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(getK8sEnvForTest(t).Log). WithTransportClient(getK8sEnvForTest(t).Client). Build() if err != nil { @@ -1430,6 +1419,12 @@ func TestExecutor_PayloadBuildFailure(t *testing.T) { t.Setenv("HYPERFLEET_API_BASE_URL", mockAPI.URL()) t.Setenv("HYPERFLEET_API_VERSION", "v1") + // Capture log output via the process-global default logger to verify error logging + var logBuf bytes.Buffer + prevLogger := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logBuf, &slog.HandlerOptions{Level: slog.LevelDebug}))) + t.Cleanup(func() { slog.SetDefault(prevLogger) }) + // Create config with invalid CEL expression in payload build (will cause build failure) config := &configloader.Config{ Adapter: configloader.AdapterInfo{ @@ -1483,15 +1478,12 @@ func TestExecutor_PayloadBuildFailure(t *testing.T) { }, } - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() assert.NoError(t, err) - // Use capture logger to verify error logging - log, logCapture := logger.NewCaptureLogger() exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(log). WithTransportClient(getK8sEnvForTest(t).Client). Build() if err != nil { @@ -1530,9 +1522,9 @@ func TestExecutor_PayloadBuildFailure(t *testing.T) { // Verify error was logged (should contain "failed to build") // slog uses "level=ERROR" format - capturedLogs := logCapture.Messages() + capturedLogs := logBuf.String() t.Logf("Captured logs:\n%s", capturedLogs) - foundErrorLog := logCapture.Contains("level=ERROR") && logCapture.Contains("failed to build") + foundErrorLog := strings.Contains(capturedLogs, "level=ERROR") && strings.Contains(capturedLogs, "failed to build") assert.True(t, foundErrorLog, "Expected to find error log for payload build failure") // Verify NO API call was made to the post action endpoint (blocked) @@ -1640,7 +1632,7 @@ func TestExecutor_PayloadWhenCondition(t *testing.T) { }, } - apiClient, err := hyperfleetapi.NewClient(testLog(), + apiClient, err := hyperfleetapi.NewClient( hyperfleetapi.WithTimeout(10*time.Second), hyperfleetapi.WithRetryAttempts(1), ) @@ -1649,7 +1641,6 @@ func TestExecutor_PayloadWhenCondition(t *testing.T) { exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(k8sEnv.Log). WithTransportClient(k8sEnv.Client). Build() require.NoError(t, err) @@ -1798,13 +1789,12 @@ func TestExecutor_CELNamespaces_EnvAndEvent(t *testing.T) { }, } - apiClient, err := hyperfleetapi.NewClient(testLog(), hyperfleetapi.WithRetryAttempts(1)) + apiClient, err := hyperfleetapi.NewClient(hyperfleetapi.WithRetryAttempts(1)) require.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(k8sEnv.Log). WithTransportClient(k8sEnv.Client). Build() require.NoError(t, err) @@ -1981,13 +1971,12 @@ func TestExecutor_CELNamespaces_ResourcesAdapterCaptures(t *testing.T) { }, } - apiClient, err := hyperfleetapi.NewClient(testLog(), hyperfleetapi.WithRetryAttempts(1)) + apiClient, err := hyperfleetapi.NewClient(hyperfleetapi.WithRetryAttempts(1)) require.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). - WithLogger(k8sEnv.Log). WithTransportClient(k8sEnv.Client). Build() require.NoError(t, err) diff --git a/test/integration/executor/executor_k8s_integration_test.go b/test/integration/executor/executor_k8s_integration_test.go index 4d7d638f..85c48060 100644 --- a/test/integration/executor/executor_k8s_integration_test.go +++ b/test/integration/executor/executor_k8s_integration_test.go @@ -329,7 +329,7 @@ func TestExecutor_K8s_CreateResources(t *testing.T) { // Create config with K8s resources config := createK8sTestConfig(testNamespace) - apiClient, err := hyperfleetapi.NewClient(testLog(), + apiClient, err := hyperfleetapi.NewClient( hyperfleetapi.WithTimeout(10*time.Second), hyperfleetapi.WithRetryAttempts(1), ) @@ -340,7 +340,6 @@ func TestExecutor_K8s_CreateResources(t *testing.T) { WithConfig(config). WithAPIClient(apiClient). WithTransportClient(k8sEnv.Client). - WithLogger(k8sEnv.Log). Build() require.NoError(t, err) @@ -488,13 +487,12 @@ func TestExecutor_K8s_UpdateExistingResource(t *testing.T) { // Only include ConfigMap resource for this test config.Resources = config.Resources[:1] - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() require.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). WithTransportClient(k8sEnv.Client). - WithLogger(k8sEnv.Log). Build() require.NoError(t, err) @@ -595,13 +593,12 @@ func TestExecutor_K8s_DiscoveryByLabels(t *testing.T) { }, } - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() require.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). WithTransportClient(k8sEnv.Client). - WithLogger(k8sEnv.Log). Build() require.NoError(t, err) @@ -666,13 +663,12 @@ func TestExecutor_K8s_RecreateOnChange(t *testing.T) { }, } - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() require.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). WithTransportClient(k8sEnv.Client). - WithLogger(k8sEnv.Log). Build() require.NoError(t, err) @@ -724,13 +720,12 @@ func TestExecutor_K8s_MultipleResourceTypes(t *testing.T) { // Execute with default config (ConfigMap + Secret) config := createK8sTestConfig(testNamespace) - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() require.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). WithTransportClient(k8sEnv.Client). - WithLogger(k8sEnv.Log). Build() require.NoError(t, err) @@ -772,13 +767,12 @@ func TestExecutor_K8s_ResourceCreationFailure(t *testing.T) { t.Setenv("HYPERFLEET_API_VERSION", "v1") config := createK8sTestConfig(nonExistentNamespace) - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() require.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). WithTransportClient(k8sEnv.Client). - WithLogger(k8sEnv.Log). Build() require.NoError(t, err) @@ -920,13 +914,12 @@ func TestExecutor_K8s_MultipleMatchingResources(t *testing.T) { }, } - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() require.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). WithTransportClient(k8sEnv.Client). - WithLogger(k8sEnv.Log). Build() require.NoError(t, err) @@ -995,13 +988,12 @@ func TestExecutor_K8s_PostActionsAfterPreconditionNotMet(t *testing.T) { t.Setenv("HYPERFLEET_API_VERSION", "v1") config := createK8sTestConfig(testNamespace) - apiClient, err := hyperfleetapi.NewClient(testLog()) + apiClient, err := hyperfleetapi.NewClient() require.NoError(t, err) exec, err := executor.NewBuilder(). WithConfig(config). WithAPIClient(apiClient). WithTransportClient(k8sEnv.Client). - WithLogger(k8sEnv.Log). Build() require.NoError(t, err) diff --git a/test/integration/executor/main_test.go b/test/integration/executor/main_test.go index 935955cc..13c998de 100644 --- a/test/integration/executor/main_test.go +++ b/test/integration/executor/main_test.go @@ -11,7 +11,6 @@ import ( "time" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/k8sclient" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-adapter/test/integration/testutil" "github.com/testcontainers/testcontainers-go" "github.com/testcontainers/testcontainers-go/wait" @@ -104,7 +103,6 @@ func TestMain(m *testing.M) { // setupSharedK8sEnvtestEnv creates the shared envtest environment for executor tests func setupSharedK8sEnvtestEnv() (*K8sTestEnv, error) { ctx := context.Background() - log := logger.NewTestLogger() imageName := os.Getenv("INTEGRATION_ENVTEST_IMAGE") @@ -148,7 +146,7 @@ func setupSharedK8sEnvtestEnv() (*K8sTestEnv, error) { println(" ✅ API server is ready!") // Create K8s client - client, err := k8sclient.NewClientFromConfig(ctx, restConfig, log) + client, err := k8sclient.NewClientFromConfig(ctx, restConfig) if err != nil { sharedContainer.Cleanup() return nil, fmt.Errorf("failed to create K8s client: %w", err) @@ -175,7 +173,6 @@ func setupSharedK8sEnvtestEnv() (*K8sTestEnv, error) { Client: client, Config: restConfig, Ctx: ctx, - Log: log, cleanup: func() { sharedContainer.Cleanup() }, diff --git a/test/integration/executor/setup_test.go b/test/integration/executor/setup_test.go index d8b95db9..b110bf8f 100644 --- a/test/integration/executor/setup_test.go +++ b/test/integration/executor/setup_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/k8sclient" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" @@ -17,15 +16,9 @@ type K8sTestEnv struct { Client *k8sclient.Client Config *rest.Config Ctx context.Context - Log logger.Logger cleanup func() } -// testLog returns a shared logger for tests (independent of K8s environment) -func testLog() logger.Logger { - return logger.NewTestLogger() -} - // SetupK8sTestEnv returns the shared K8s test environment func SetupK8sTestEnv(t *testing.T) *K8sTestEnv { t.Helper() diff --git a/test/integration/k8sclient/client_integration_test.go b/test/integration/k8sclient/client_integration_test.go index f2b116b3..85d98010 100644 --- a/test/integration/k8sclient/client_integration_test.go +++ b/test/integration/k8sclient/client_integration_test.go @@ -36,7 +36,6 @@ func TestIntegration_NewClient(t *testing.T) { t.Run("client is properly initialized", func(t *testing.T) { assert.NotNil(t, env.GetClient()) assert.NotNil(t, env.GetContext()) - assert.NotNil(t, env.GetLogger()) }) } diff --git a/test/integration/k8sclient/helper_envtest_prebuilt.go b/test/integration/k8sclient/helper_envtest_prebuilt.go index 9689ac31..e712b334 100644 --- a/test/integration/k8sclient/helper_envtest_prebuilt.go +++ b/test/integration/k8sclient/helper_envtest_prebuilt.go @@ -18,7 +18,6 @@ import ( "k8s.io/client-go/rest" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/k8sclient" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-adapter/test/integration/testutil" ) @@ -91,7 +90,6 @@ type TestEnvPrebuilt struct { Client *k8sclient.Client Config *rest.Config Ctx context.Context - Log logger.Logger } // Cleanup terminates the container (no-op for shared containers, use CleanupSharedEnv) @@ -106,7 +104,6 @@ func (e *TestEnvPrebuilt) Cleanup(t *testing.T) { // Returns an error instead of panicking to allow graceful handling. func setupSharedTestEnv() (*TestEnvPrebuilt, error) { ctx := context.Background() - log := logger.NewTestLogger() // Check that INTEGRATION_ENVTEST_IMAGE is set imageName := os.Getenv("INTEGRATION_ENVTEST_IMAGE") @@ -164,7 +161,7 @@ func setupSharedTestEnv() (*TestEnvPrebuilt, error) { } // Create client - client, err := k8sclient.NewClientFromConfig(ctx, restConfig, log) + client, err := k8sclient.NewClientFromConfig(ctx, restConfig) if err != nil { sharedContainer.Cleanup() return nil, fmt.Errorf("failed to create K8s client: %w", err) @@ -181,7 +178,6 @@ func setupSharedTestEnv() (*TestEnvPrebuilt, error) { Client: client, Config: restConfig, Ctx: ctx, - Log: log, }, nil } diff --git a/test/integration/k8sclient/helper_selector.go b/test/integration/k8sclient/helper_selector.go index e7fc6814..37e4f9d2 100644 --- a/test/integration/k8sclient/helper_selector.go +++ b/test/integration/k8sclient/helper_selector.go @@ -7,7 +7,6 @@ import ( "testing" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/k8sclient" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "k8s.io/client-go/rest" ) @@ -16,7 +15,6 @@ type TestEnv interface { GetClient() *k8sclient.Client GetConfig() *rest.Config GetContext() context.Context - GetLogger() logger.Logger Cleanup(t *testing.T) } @@ -38,11 +36,6 @@ func (e *TestEnvPrebuilt) GetContext() context.Context { return e.Ctx } -// GetLogger returns the logger -func (e *TestEnvPrebuilt) GetLogger() logger.Logger { - return e.Log -} - // isAlreadyExistsError checks if the error is an "already exists" error func isAlreadyExistsError(err error) bool { if err == nil { diff --git a/test/integration/maestroclient/client_integration_test.go b/test/integration/maestroclient/client_integration_test.go index 68d3c2cd..ae1ee730 100644 --- a/test/integration/maestroclient/client_integration_test.go +++ b/test/integration/maestroclient/client_integration_test.go @@ -8,7 +8,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/maestroclient" "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/constants" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -41,13 +40,6 @@ func createTestClient(t *testing.T, sourceID string, timeout time.Duration) *tes env := GetSharedEnv(t) - log, err := logger.NewLogger(logger.Config{ - Level: "debug", - Format: "text", - Component: "maestro-integration-test", - }) - require.NoError(t, err) - ctx, cancel := context.WithTimeout(context.Background(), timeout) config := &maestroclient.Config{ @@ -57,7 +49,7 @@ func createTestClient(t *testing.T, sourceID string, timeout time.Duration) *tes Insecure: true, } - client, err := maestroclient.NewMaestroClient(ctx, config, log) + client, err := maestroclient.NewMaestroClient(ctx, config) if err != nil { cancel() require.NoError(t, err, "Should create Maestro client successfully") diff --git a/test/integration/maestroclient/client_tls_integration_test.go b/test/integration/maestroclient/client_tls_integration_test.go index 526fba5c..c0ad5834 100644 --- a/test/integration/maestroclient/client_tls_integration_test.go +++ b/test/integration/maestroclient/client_tls_integration_test.go @@ -16,7 +16,6 @@ import ( "time" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/maestroclient" - "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/logger" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -25,16 +24,9 @@ import ( func createTLSTestClient(t *testing.T, config *maestroclient.Config, timeout time.Duration) *testClient { t.Helper() - log, err := logger.NewLogger(logger.Config{ - Level: "debug", - Format: "text", - Component: "maestro-tls-integration-test", - }) - require.NoError(t, err) - ctx, cancel := context.WithTimeout(context.Background(), timeout) - client, err := maestroclient.NewMaestroClient(ctx, config, log) + client, err := maestroclient.NewMaestroClient(ctx, config) if err != nil { cancel() require.NoError(t, err, "Should create TLS Maestro client successfully") @@ -235,17 +227,10 @@ func TestTLSNoConfigFails(t *testing.T) { Insecure: false, } - log, err := logger.NewLogger(logger.Config{ - Level: "debug", - Format: "text", - Component: "maestro-tls-no-config-test", - }) - require.NoError(t, err) - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - _, err = maestroclient.NewMaestroClient(ctx, config, log) + _, err := maestroclient.NewMaestroClient(ctx, config) require.Error(t, err, "Should fail when Insecure=false and no TLS config provided") assert.Contains(t, err.Error(), "no TLS configuration provided") t.Logf("No TLS config correctly rejected: %v", err)