From 597b7790455f8dd8898b092aff381541247e66c3 Mon Sep 17 00:00:00 2001 From: "coderabbitai[bot]" <136622811+coderabbitai[bot]@users.noreply.github.com> Date: Thu, 8 Jan 2026 12:19:47 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=93=9D=20Add=20docstrings=20to=20`HYPERFL?= =?UTF-8?q?EET-463`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Docstrings generation was requested by @xueli181114. * https://github.com/openshift-hyperfleet/hyperfleet-adapter/pull/26#issuecomment-3723577569 The following files were modified: * `cmd/adapter/main.go` * `internal/executor/utils.go` * `pkg/logger/context.go` * `pkg/logger/logger.go` --- cmd/adapter/main.go | 102 ++++++++++++++++++++----- internal/executor/utils.go | 29 ++++++-- pkg/logger/context.go | 147 ++++++++++++++++++++++++++++++++++--- pkg/logger/logger.go | 39 +++------- 4 files changed, 255 insertions(+), 62 deletions(-) diff --git a/cmd/adapter/main.go b/cmd/adapter/main.go index 1bf8299..c693a87 100644 --- a/cmd/adapter/main.go +++ b/cmd/adapter/main.go @@ -17,6 +17,11 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-broker/broker" "github.com/spf13/cobra" "github.com/spf13/pflag" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/propagation" + "go.opentelemetry.io/otel/sdk/resource" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + semconv "go.opentelemetry.io/otel/semconv/v1.26.0" ) // Build-time variables set via ldflags @@ -108,7 +113,9 @@ and HyperFleet API calls.`, } // buildLoggerConfig creates a logger configuration from environment variables -// and command-line flags. Flags take precedence over environment variables. +// buildLoggerConfig builds a logger.Config by loading defaults from the environment, +// applying any command-line flag overrides (flags take precedence), and setting the +// provided component name and the global version. func buildLoggerConfig(component string) logger.Config { cfg := logger.ConfigFromEnv() @@ -129,7 +136,38 @@ func buildLoggerConfig(component string) logger.Config { return cfg } -// runServe contains the main application logic for the serve command +// initTracer initializes OpenTelemetry TracerProvider for generating trace_id and span_id. +// These IDs are used for: +// 1. Log correlation (via logger.WithOTelTraceContext) +// initTracer creates and registers an OpenTelemetry TracerProvider configured with the +// provided service name and version. It sets the global TracerProvider and installs the +// W3C Trace Context text-map propagator for HTTP request propagation. +// Returns the created TracerProvider, or an error if the required resource cannot be built. +func initTracer(serviceName, serviceVersion string) (*sdktrace.TracerProvider, error) { + res, err := resource.Merge( + resource.Default(), + resource.NewWithAttributes( + semconv.SchemaURL, + semconv.ServiceName(serviceName), + semconv.ServiceVersion(serviceVersion), + ), + ) + if err != nil { + return nil, fmt.Errorf("failed to create resource: %w", err) + } + + tp := sdktrace.NewTracerProvider( + sdktrace.WithResource(res), + sdktrace.WithSampler(sdktrace.AlwaysSample()), + ) + otel.SetTracerProvider(tp) + otel.SetTextMapPropagator(propagation.TraceContext{}) + + return tp, nil +} + +// runServe executes the main application lifecycle for the serve subcommand, including logger and configuration setup, OpenTelemetry initialization, HyperFleet API and Kubernetes client creation, executor and broker subscription setup, and graceful shutdown handling. +// It returns an error if initialization fails or if an unrecoverable runtime condition occurs. func runServe() error { // Create context that cancels on system signals ctx, cancel := context.WithCancel(context.Background()) @@ -148,7 +186,8 @@ func runServe() error { log.Info(ctx, "Loading adapter configuration...") adapterConfig, err := config_loader.Load(configPath, config_loader.WithAdapterVersion(version)) if err != nil { - log.Errorf(ctx, "Failed to load adapter configuration: %v", err) + errCtx := logger.WithStackTraceField(logger.WithErrorField(ctx, err), logger.CaptureStackTrace(1)) + log.Errorf(errCtx, "Failed to load adapter configuration") return fmt.Errorf("failed to load adapter configuration: %w", err) } @@ -164,11 +203,25 @@ func runServe() error { adapterConfig.Spec.HyperfleetAPI.Timeout, adapterConfig.Spec.HyperfleetAPI.RetryAttempts) + // Initialize OpenTelemetry for trace_id/span_id generation and HTTP propagation + tp, err := initTracer(adapterConfig.Metadata.Name, version) + if err != nil { + errCtx := logger.WithStackTraceField(logger.WithErrorField(ctx, err), logger.CaptureStackTrace(1)) + log.Errorf(errCtx, "Failed to initialize OpenTelemetry") + return fmt.Errorf("failed to initialize OpenTelemetry: %w", err) + } + defer func() { + shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer shutdownCancel() + _ = tp.Shutdown(shutdownCtx) + }() + // Create HyperFleet API client from config log.Info(ctx, "Creating HyperFleet API client...") apiClient, err := createAPIClient(adapterConfig.Spec.HyperfleetAPI, log) if err != nil { - log.Errorf(ctx, "Failed to create HyperFleet API client: %v", err) + errCtx := logger.WithStackTraceField(logger.WithErrorField(ctx, err), logger.CaptureStackTrace(1)) + log.Errorf(errCtx, "Failed to create HyperFleet API client") return fmt.Errorf("failed to create HyperFleet API client: %w", err) } @@ -177,7 +230,8 @@ func runServe() error { log.Info(ctx, "Creating Kubernetes client...") k8sClient, err := k8s_client.NewClient(ctx, k8s_client.ClientConfig{}, log) if err != nil { - log.Errorf(ctx, "Failed to create Kubernetes client: %v", err) + errCtx := logger.WithStackTraceField(logger.WithErrorField(ctx, err), logger.CaptureStackTrace(1)) + log.Errorf(errCtx, "Failed to create Kubernetes client") return fmt.Errorf("failed to create Kubernetes client: %w", err) } @@ -190,7 +244,8 @@ func runServe() error { WithLogger(log). Build() if err != nil { - log.Errorf(ctx, "Failed to create executor: %v", err) + errCtx := logger.WithStackTraceField(logger.WithErrorField(ctx, err), logger.CaptureStackTrace(1)) + log.Errorf(errCtx, "Failed to create executor") return fmt.Errorf("failed to create executor: %w", err) } @@ -219,15 +274,19 @@ func runServe() error { // Get subscription ID from environment subscriptionID := os.Getenv(EnvBrokerSubscriptionID) if subscriptionID == "" { - log.Errorf(ctx, "%s environment variable is required", EnvBrokerSubscriptionID) - return fmt.Errorf("%s environment variable is required", EnvBrokerSubscriptionID) + err := fmt.Errorf("%s environment variable is required", EnvBrokerSubscriptionID) + errCtx := logger.WithStackTraceField(logger.WithErrorField(ctx, err), logger.CaptureStackTrace(1)) + log.Errorf(errCtx, "Missing required environment variable") + return err } // Get topic from environment topic := os.Getenv(EnvBrokerTopic) if topic == "" { - log.Errorf(ctx, "%s environment variable is required", EnvBrokerTopic) - return fmt.Errorf("%s environment variable is required", EnvBrokerTopic) + err := fmt.Errorf("%s environment variable is required", EnvBrokerTopic) + errCtx := logger.WithStackTraceField(logger.WithErrorField(ctx, err), logger.CaptureStackTrace(1)) + log.Errorf(errCtx, "Missing required environment variable") + return err } // Create broker subscriber @@ -237,9 +296,10 @@ func runServe() error { // - BROKER_RABBITMQ_URL: RabbitMQ URL (for rabbitmq) // - SUBSCRIBER_PARALLELISM: number of parallel workers (default: 1) log.Info(ctx, "Creating broker subscriber...") - subscriber, err := broker.NewSubscriber(subscriptionID) + subscriber, err := broker.NewSubscriber(log, subscriptionID) if err != nil { - log.Errorf(ctx, "Failed to create subscriber: %v", err) + errCtx := logger.WithStackTraceField(logger.WithErrorField(ctx, err), logger.CaptureStackTrace(1)) + log.Errorf(errCtx, "Failed to create subscriber") return fmt.Errorf("failed to create subscriber: %w", err) } log.Info(ctx, "Broker subscriber created successfully") @@ -248,7 +308,8 @@ func runServe() error { log.Info(ctx, "Subscribing to broker topic...") err = subscriber.Subscribe(ctx, topic, handler) if err != nil { - log.Errorf(ctx, "Failed to subscribe to topic: %v", err) + errCtx := logger.WithStackTraceField(logger.WithErrorField(ctx, err), logger.CaptureStackTrace(1)) + log.Errorf(errCtx, "Failed to subscribe to topic") return fmt.Errorf("failed to subscribe to topic: %w", err) } log.Info(ctx, "Successfully subscribed to broker topic") @@ -259,7 +320,8 @@ func runServe() error { // Monitor subscription errors channel in a separate goroutine go func() { for subErr := range subscriber.Errors() { - log.Errorf(ctx, "Subscription error: %v", subErr) + errCtx := logger.WithStackTraceField(logger.WithErrorField(ctx, subErr), logger.CaptureStackTrace(1)) + log.Errorf(errCtx, "Subscription error") // For critical errors, signal shutdown select { case fatalErrCh <- subErr: @@ -277,7 +339,8 @@ func runServe() error { case <-ctx.Done(): log.Info(ctx, "Context cancelled, shutting down...") case err := <-fatalErrCh: - log.Errorf(ctx, "Fatal subscription error, shutting down: %v", err) + errCtx := logger.WithStackTraceField(logger.WithErrorField(ctx, err), logger.CaptureStackTrace(1)) + log.Errorf(errCtx, "Fatal subscription error, shutting down") cancel() // Cancel context to trigger graceful shutdown } @@ -295,12 +358,15 @@ func runServe() error { select { case err := <-closeDone: if err != nil { - log.Errorf(ctx, "Error closing subscriber: %v", err) + errCtx := logger.WithStackTraceField(logger.WithErrorField(ctx, err), logger.CaptureStackTrace(1)) + log.Errorf(errCtx, "Error closing subscriber") } else { log.Info(ctx, "Subscriber closed successfully") } case <-shutdownCtx.Done(): - log.Error(ctx, "Subscriber close timed out after 30 seconds") + err := fmt.Errorf("subscriber close timed out after 30 seconds") + errCtx := logger.WithStackTraceField(logger.WithErrorField(ctx, err), logger.CaptureStackTrace(1)) + log.Error(errCtx, "Subscriber close timed out") } log.Info(ctx, "Adapter shutdown complete") @@ -338,4 +404,4 @@ func createAPIClient(apiConfig config_loader.HyperfleetAPIConfig, log logger.Log } return hyperfleet_api.NewClient(log, opts...) -} +} \ No newline at end of file diff --git a/internal/executor/utils.go b/internal/executor/utils.go index 87dea7c..f9fdb7e 100644 --- a/internal/executor/utils.go +++ b/internal/executor/utils.go @@ -35,7 +35,12 @@ func ToConditionDefs(conditions []config_loader.Condition) []criteria.ConditionD // ExecuteLogAction executes a log action with the given context // The message is rendered as a Go template with access to all params -// This is a shared utility function used by both PreconditionExecutor and PostActionExecutor +// ExecuteLogAction renders a log message template and emits the result at the configured level. +// +// If logAction is nil or its Message is empty, the function returns immediately. The Message is +// rendered as a Go template using execCtx.Params; if rendering fails the error is logged with an +// attached stack trace and the function returns. The rendered message is logged with a "[config]" +// prefix at the level specified by logAction.Level (defaults to "info"). func ExecuteLogAction(ctx context.Context, logAction *config_loader.LogAction, execCtx *ExecutionContext, log logger.Logger) { if logAction == nil || logAction.Message == "" { return @@ -44,7 +49,8 @@ func ExecuteLogAction(ctx context.Context, logAction *config_loader.LogAction, e // Render the message template message, err := renderTemplate(logAction.Message, execCtx.Params) if err != nil { - log.Errorf(ctx, "failed to render log message: %v", err) + errCtx := logger.WithStackTraceField(logger.WithErrorField(ctx, err), logger.CaptureStackTrace(1)) + log.Errorf(errCtx, "failed to render log message") return } @@ -72,7 +78,13 @@ func ExecuteLogAction(ctx context.Context, logAction *config_loader.LogAction, e // ExecuteAPICall executes an API call with the given configuration and returns the response and rendered URL // This is a shared utility function used by both PreconditionExecutor and PostActionExecutor // On error, it returns an APIError with full context (method, URL, status, body, attempts, duration) -// Returns: response, renderedURL, error +// ExecuteAPICall executes the HTTP API call described by apiCall using apiClient. +// +// It renders the URL, header values, and body templates with execCtx.Params, applies timeout and retry options when specified, +// and performs the request with the HTTP method from apiCall (supports GET, POST, PUT, PATCH, DELETE). +// On error, it returns any available response along with an APIError that includes method, URL, status, body, attempts, duration, and the underlying error. +// If apiCall is nil or template rendering fails, a descriptive error is returned. +// If the HTTP method is unsupported, an error indicating the unsupported method is returned. func ExecuteAPICall(ctx context.Context, apiCall *config_loader.APICall, execCtx *ExecutionContext, apiClient hyperfleet_api.Client, log logger.Logger) (*hyperfleet_api.Response, string, error) { if apiCall == nil { return nil, "", fmt.Errorf("apiCall is nil") @@ -137,7 +149,14 @@ func ExecuteAPICall(ctx context.Context, apiCall *config_loader.APICall, execCtx resp, err = apiClient.Post(ctx, url, body, opts...) // Log body on failure for debugging if err != nil || (resp != nil && !resp.IsSuccess()) { - log.Errorf(ctx, "POST %s failed, request body: %s", url, string(body)) + var logErr error + if err != nil { + logErr = err + } else { + logErr = fmt.Errorf("POST %s returned non-success status: %d", url, resp.StatusCode) + } + errCtx := logger.WithStackTraceField(logger.WithErrorField(ctx, logErr), logger.CaptureStackTrace(1)) + log.Errorf(errCtx, "POST %s failed, request body: %s", url, string(body)) } case http.MethodPut: body := []byte(apiCall.Body) @@ -385,4 +404,4 @@ func adapterMetadataToMap(adapter *AdapterMetadata) map[string]interface{} { "errorMessage": adapter.ErrorMessage, "executionError": executionErrorToMap(adapter.ExecutionError), } -} +} \ No newline at end of file diff --git a/pkg/logger/context.go b/pkg/logger/context.go index b0298a0..f59745a 100644 --- a/pkg/logger/context.go +++ b/pkg/logger/context.go @@ -2,22 +2,36 @@ package logger import ( "context" + "fmt" + "runtime" "strings" + + "go.opentelemetry.io/otel/trace" ) // contextKey is a custom type for context keys to avoid collisions type contextKey string const ( + // Required fields (per logging spec) + ComponentKey contextKey = "component" + VersionKey contextKey = "version" + HostnameKey contextKey = "hostname" + + // Error fields (per logging spec) + ErrorKey contextKey = "error" + StackTraceKey contextKey = "stack_trace" + // Correlation fields (distributed tracing) TraceIDKey contextKey = "trace_id" SpanIDKey contextKey = "span_id" EventIDKey contextKey = "event_id" // Resource fields - ClusterIDKey contextKey = "cluster_id" - ResourceTypeKey contextKey = "resource_type" - ResourceIDKey contextKey = "resource_id" + ClusterIDKey contextKey = "cluster_id" + ResourceTypeKey contextKey = "resource_type" + ResourceNameKey contextKey = "resource_name" + ResourceResultKey contextKey = "resource_result" // Adapter-specific fields AdapterKey contextKey = "adapter" @@ -86,14 +100,19 @@ func WithClusterID(ctx context.Context, clusterID string) context.Context { return WithLogField(ctx, string(ClusterIDKey), clusterID) } -// WithResourceType returns a context with the resource type set +// WithResourceType returns a copy of the context that includes the provided resource type under the ResourceTypeKey log field. func WithResourceType(ctx context.Context, resourceType string) context.Context { return WithLogField(ctx, string(ResourceTypeKey), resourceType) } -// WithResourceID returns a context with the resource ID set -func WithResourceID(ctx context.Context, resourceID string) context.Context { - return WithLogField(ctx, string(ResourceIDKey), resourceID) +// WithResourceName returns a context with the resource name set +func WithResourceName(ctx context.Context, resourceName string) context.Context { + return WithLogField(ctx, string(ResourceNameKey), resourceName) +} + +// WithResourceResult returns a context with the resource operation result set to the provided value (for example, "SUCCESS" or "FAILED"). +func WithResourceResult(ctx context.Context, result string) context.Context { + return WithLogField(ctx, string(ResourceResultKey), result) } // WithAdapter returns a context with the adapter name set @@ -106,11 +125,121 @@ func WithObservedGeneration(ctx context.Context, generation int64) context.Conte return WithLogField(ctx, string(ObservedGenerationKey), generation) } -// WithSubscription returns a context with the subscription name set +// WithSubscription adds the subscription name to the context under SubscriptionKey. +// It returns a new context containing that subscription value. func WithSubscription(ctx context.Context, subscription string) context.Context { return WithLogField(ctx, string(SubscriptionKey), subscription) } +// WithErrorField returns a context with the error message set. +// WithErrorField adds the error message from err to the context under the error key. +// If err is nil, it returns the original context unchanged. +func WithErrorField(ctx context.Context, err error) context.Context { + if err == nil { + return ctx + } + return WithLogField(ctx, string(ErrorKey), err.Error()) +} + +// WithStackTraceField returns a context with the stack trace set. +// WithStackTraceField adds the given stack trace frames to the context's log fields under the stack trace key. +// If frames is nil or empty, the original context is returned unchanged. +func WithStackTraceField(ctx context.Context, frames []string) context.Context { + if len(frames) == 0 { + return ctx + } + return WithLogField(ctx, string(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 +// CaptureStackTrace captures the current goroutine's stack frames and returns them as formatted strings. +// The skip parameter omits that many additional caller frames from the result (skip=0 omits the runtime callers and CaptureStackTrace itself; skip=1 omits one additional frame, etc.). +// Each returned entry is formatted as "file:line function". The function returns nil if no frames are captured. +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 +} + +// WithOTelTraceID extracts only trace_id from OpenTelemetry span context. +// Use this at the event/request entry point where you only need trace correlation. +// If no active span exists, returns the context unchanged. +// +// Example usage: +// +// ctx = logger.WithOTelTraceID(ctx) +// log.Info(ctx, "Processing event") +// +// This will produce logs with trace_id field only: +// +// WithOTelTraceID adds the OpenTelemetry span's trace ID to the context's log fields when present. +// If the current context contains a valid span with a trace ID, the `trace_id` log field is set; +// otherwise the original context is returned unchanged. +func WithOTelTraceID(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, string(TraceIDKey), spanCtx.TraceID().String()) + } + + return ctx +} + +// WithOTelTraceContext extracts OpenTelemetry trace context (trace_id, span_id) +// from the context and adds them as log fields for distributed tracing correlation. +// Use this for HTTP requests where span_id helps identify specific operations. +// If no active span exists, returns the context unchanged. +// +// Example usage: +// +// ctx = logger.WithOTelTraceContext(ctx) +// log.Info(ctx, "Making HTTP request") +// +// This will produce logs with trace_id and span_id fields: +// +// WithOTelTraceContext adds OpenTelemetry trace and span identifiers from ctx's current span to the context's log fields. +// If the span contains a trace ID and/or span ID those values are set under TraceIDKey and SpanIDKey; if no valid span context exists the original context is returned unchanged. +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, string(TraceIDKey), spanCtx.TraceID().String()) + } + + // Add span_id if valid + if spanCtx.HasSpanID() { + ctx = WithLogField(ctx, string(SpanIDKey), spanCtx.SpanID().String()) + } + + return ctx +} + // ----------------------------------------------------------------------------- // Context Getters // ----------------------------------------------------------------------------- @@ -129,4 +258,4 @@ func GetLogFields(ctx context.Context) LogFields { return fields } return nil -} +} \ No newline at end of file diff --git a/pkg/logger/logger.go b/pkg/logger/logger.go index cadf37b..f940224 100644 --- a/pkg/logger/logger.go +++ b/pkg/logger/logger.go @@ -36,8 +36,6 @@ type Logger interface { With(key string, value interface{}) Logger // WithFields returns a new logger with multiple additional fields WithFields(fields map[string]interface{}) Logger - // WithError returns a new logger with error field (no-op if err is nil) - WithError(err error) Logger // Without returns a new logger with the specified field removed Without(key string) Logger } @@ -100,7 +98,10 @@ func ConfigFromEnv() Config { } // NewLogger creates a new Logger with the given configuration -// Returns error if output is invalid (must be "stdout", "stderr", or empty) +// NewLogger creates a Logger configured according to cfg. +// It selects the output writer, log level, and format, initializes the base +// fields (component, version, hostname), and returns a logger ready for use. +// Returns an error if cfg.Output is invalid (must be "stdout", "stderr", or empty). func NewLogger(cfg Config) (Logger, error) { // Determine output writer var writer io.Writer @@ -146,11 +147,11 @@ func NewLogger(cfg Config) (Logger, error) { hostname = "unknown" } - // Create base logger with required fields + // Create base logger with required fields (per logging spec) slogLogger := slog.New(handler).With( - "component", cfg.Component, - "version", cfg.Version, - "hostname", hostname, + string(ComponentKey), cfg.Component, + string(VersionKey), cfg.Version, + string(HostnameKey), hostname, ) return &logger{ @@ -283,28 +284,6 @@ func (l *logger) WithFields(fields map[string]interface{}) Logger { } } -// WithError returns a new logger with the error field set. -// If err is nil, returns the same logger instance (no-op) to avoid -// unnecessary allocations. This allows safe usage like: -// -// log.WithError(maybeNilErr).Info("message") -// -// To remove an existing error field, use Without("error"). -func (l *logger) WithError(err error) Logger { - if err == nil { - return l - } - newFields := copyFields(l.fields) - newFields["error"] = err.Error() - 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 { @@ -334,4 +313,4 @@ func GetStackTrace(skip int) []string { } } return stack -} +} \ No newline at end of file