[LFXV2-612] Add OpenTelemetry tracing support #10
Conversation
- Add pkg/utils/otel.go with OTEL SDK setup and configuration - Add pkg/utils/otel_test.go with unit tests - Initialize OTEL in main.go with graceful shutdown - Wrap HTTP handler with otelhttp instrumentation Issue: LFXV2-612 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: Trevor Bramwell <[email protected]>
WalkthroughIntegrates OpenTelemetry distributed tracing into the lfx-access-check application by establishing SDK initialization in main, instrumenting the HTTP server, introducing configuration utilities with environment variable support, updating dependencies to OTEL v1.39.0, and providing comprehensive tests. Changes
Sequence DiagramsequenceDiagram
participant App as Application<br/>(main)
participant Env as Environment<br/>Variables
participant Config as OTelConfig
participant Prop as Propagator
participant TP as TracerProvider
participant Exp as OTLP Exporter
participant SDK as OTel SDK
App->>Env: Read OTEL_*<br/>environment vars
Env-->>App: Config values
App->>Config: OTelConfigFromEnv()
Config-->>App: OTelConfig instance
App->>SDK: SetupOTelSDKWithConfig(ctx, config)
SDK->>Prop: newPropagator()
Prop->>SDK: Install TextMapPropagator<br/>(TraceContext + Baggage)
alt ExporterEnabled
SDK->>TP: newTraceProvider(ctx, config)
TP->>Exp: Create OTLP Exporter<br/>(gRPC or HTTP)
Exp-->>TP: Exporter instance
TP->>SDK: Configure with Resource<br/>(service name/version)
TP-->>SDK: TracerProvider ready
SDK->>SDK: Register shutdown<br/>function
end
SDK-->>App: shutdown function,<br/>error (if any)
App->>App: Defer shutdown()<br/>on application exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/utils/otel.go (2)
95-98: Minor formatting and potential concern withhandleErrpattern.There's a missing space after the comma on line 97. Additionally, this
handleErrfunction is defined but only used whennewTraceProviderfails. The pattern is correct for cleanup, but ensure the named returnerris intended to be set via closure side-effect.🔧 Suggested formatting fix
handleErr := func(inErr error) { - err = errors.Join(inErr,shutdown(ctx)) + err = errors.Join(inErr, shutdown(ctx)) }
128-144: Consider validating the protocol value.Unknown protocol values (anything other than "http") silently default to gRPC. This could cause confusion if a user misconfigures
OTEL_EXPORTER_OTLP_PROTOCOLwith an invalid value like "https" or a typo.Consider either:
- Logging when falling back to gRPC
- Validating and returning an error for unsupported protocols
♻️ Option: Add explicit protocol validation
func newTraceProvider(ctx context.Context, config OTelConfig) (*trace.TracerProvider, error) { var traceClient otlptrace.Client - if config.Protocol == "http" { + switch config.Protocol { + case "http": opts := []otlptracehttp.Option{ otlptracehttp.WithEndpoint(config.Endpoint), } if config.InsecureExporter { opts = append(opts, otlptracehttp.WithInsecure()) } traceClient = otlptracehttp.NewClient(opts...) - } else { + case "grpc", "": opts := []otlptracegrpc.Option{ otlptracegrpc.WithEndpoint(config.Endpoint), } if config.InsecureExporter { opts = append(opts, otlptracegrpc.WithInsecure()) } traceClient = otlptracegrpc.NewClient(opts...) + default: + return nil, fmt.Errorf("unsupported OTLP protocol: %s (expected 'grpc' or 'http')", config.Protocol) }pkg/utils/otel_test.go (2)
62-69: Consider usingt.Setenvfor automatic cleanup.The current approach of manually calling
os.Unsetenvandos.Setenvdoesn't restore the original environment after the test completes. Usingt.Setenv(available since Go 1.17) automatically restores the original value when the test completes, providing better isolation.♻️ Suggested improvement
t.Run(tt.name, func(t *testing.T) { - // Clear relevant env vars - envKeys := []string{ - "OTEL_SERVICE_NAME", - "OTEL_SERVICE_VERSION", - "OTEL_EXPORTER_OTLP_ENDPOINT", - "OTEL_EXPORTER_OTLP_PROTOCOL", - "OTEL_TRACES_EXPORTER", - "OTEL_EXPORTER_OTLP_INSECURE", - } - for _, key := range envKeys { - os.Unsetenv(key) - } - - // Set test env vars + // Set test env vars (t.Setenv auto-restores after test) for key, value := range tt.envVars { - os.Setenv(key, value) + t.Setenv(key, value) }Note: For default values test case, you may need to explicitly set empty values or use a helper to ensure vars are unset.
117-135: Test forSetupOTelSDKshould ensure complete environment isolation.This test only unsets
OTEL_TRACES_EXPORTERbut other OTEL environment variables from previous tests or the system environment could still affect the test. Consider clearing all relevant environment variables for consistent test behavior.♻️ Suggested improvement
func TestSetupOTelSDK(t *testing.T) { - // Clear env vars to use defaults - os.Unsetenv("OTEL_TRACES_EXPORTER") + // Clear all OTEL env vars to ensure defaults are used + for _, key := range []string{ + "OTEL_SERVICE_NAME", + "OTEL_SERVICE_VERSION", + "OTEL_EXPORTER_OTLP_ENDPOINT", + "OTEL_EXPORTER_OTLP_PROTOCOL", + "OTEL_TRACES_EXPORTER", + "OTEL_EXPORTER_OTLP_INSECURE", + } { + t.Setenv(key, "") + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
cmd/lfx-access-check/main.gocmd/lfx-access-check/server.gogo.modpkg/utils/otel.gopkg/utils/otel_test.go
🧰 Additional context used
📓 Path-based instructions (4)
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
All public packages should be placed under the pkg directory
Files:
pkg/utils/otel_test.gopkg/utils/otel.go
cmd/lfx-access-check/{main.go,server.go}
📄 CodeRabbit inference engine (CLAUDE.md)
All requests require valid JWT tokens for authentication
Files:
cmd/lfx-access-check/server.gocmd/lfx-access-check/main.go
cmd/lfx-access-check/server.go
📄 CodeRabbit inference engine (CLAUDE.md)
All API endpoints must implement liveness and readiness probes at /livez and /readyz
Files:
cmd/lfx-access-check/server.go
{cmd/lfx-access-check/server.go,internal/middleware/*.go}
📄 CodeRabbit inference engine (CLAUDE.md)
All requests must be tracked with unique request IDs for correlation in logs
Files:
cmd/lfx-access-check/server.go
🧬 Code graph analysis (2)
pkg/utils/otel_test.go (1)
pkg/utils/otel.go (4)
OTelConfig(23-30)OTelConfigFromEnv(34-70)SetupOTelSDKWithConfig(80-116)SetupOTelSDK(74-76)
cmd/lfx-access-check/main.go (1)
pkg/utils/otel.go (2)
OTelConfigFromEnv(34-70)SetupOTelSDKWithConfig(80-116)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL analysis (go)
- GitHub Check: Agent
- GitHub Check: MegaLinter
🔇 Additional comments (5)
cmd/lfx-access-check/server.go (1)
101-103: LGTM! OTel HTTP instrumentation is correctly applied.The
otelhttp.NewHandlerwrapper is placed as the outermost layer in the middleware stack, which ensures all incoming requests are traced. The operation name "access-check" provides a reasonable default span name. Request ID middleware is correctly applied before tracing, so trace context will include request correlation.cmd/lfx-access-check/main.go (1)
28-40: LGTM! OTel SDK initialization follows best practices.The initialization sequence correctly:
- Creates config from environment variables
- Sets up the SDK early in the application lifecycle
- Defers shutdown with a fresh
context.Background()(appropriate since the original context may be cancelled during shutdown)- Exits on setup failure with proper error logging
pkg/utils/otel.go (1)
151-169: LGTM! Resource and TracerProvider configuration follows best practices.The implementation correctly:
- Merges default resource attributes with custom service name/version
- Uses semantic conventions (semconv v1.26.0)
- Configures a batched exporter with a reasonable 5-second timeout
pkg/utils/otel_test.go (1)
95-115: LGTM! Basic SDK setup test covers the no-exporter path.The test validates that when
TracesExporteris set to "none", the SDK initializes successfully and the shutdown function works correctly.go.mod (1)
11-15: OpenTelemetry version mismatch may cause compatibility issues.The OTel dependencies have inconsistent versions:
otel/sdkv1.39.0otelv1.39.0otlptracev1.36.0otlptracegrpcv1.35.0otlptracehttpv1.35.0OpenTelemetry documentation explicitly recommends keeping exporters and the SDK on the same major/minor release to avoid incompatibilities. The current state pairs SDK v1.39 with exporters at v1.35-1.36. Consider aligning all
go.opentelemetry.io/otel/exporters/otlp/otlptrace/*packages to v1.39.0 to match the SDK version.
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive OpenTelemetry (OTel) tracing support to the lfx-v2-access-check service. The implementation includes SDK configuration from environment variables, automatic HTTP request instrumentation, and proper lifecycle management for the tracing infrastructure.
Key Changes:
- Introduced new OTel utility package with environment-based configuration and SDK setup functions
- Integrated OTel initialization in the main entrypoint with proper shutdown handling
- Instrumented HTTP server with OTel middleware for automatic request tracing
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/utils/otel.go |
New utility for OTel SDK configuration and initialization with support for gRPC/HTTP protocols |
pkg/utils/otel_test.go |
Unit tests for OTel configuration parsing and SDK setup/shutdown |
cmd/lfx-access-check/main.go |
Added OTel SDK initialization and shutdown hooks at service startup |
cmd/lfx-access-check/server.go |
Wrapped HTTP handler with OTel instrumentation middleware |
go.mod |
Added OpenTelemetry dependencies and updated related packages |
go.sum |
Updated checksums for new and modified dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ServiceName string | ||
| ServiceVersion string | ||
| Endpoint string | ||
| Protocol string // "grpc" or "http" |
There was a problem hiding this comment.
The Protocol field accepts any string value without validation. If a value other than "grpc" or "http" is provided (line 50-53 in otel.go), the code will default to gRPC behavior (line 136-143), but this could be confusing for users. Consider adding validation to return an error for invalid protocol values, or at least document the default behavior in the OTelConfig struct comments.
| Protocol string // "grpc" or "http" | |
| Protocol string // "grpc" (default) or "http"; any value other than "http" is treated as "grpc" |
| func TestOTelConfigFromEnv(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| envVars map[string]string | ||
| expected OTelConfig | ||
| }{ | ||
| { | ||
| name: "default values", | ||
| envVars: map[string]string{}, | ||
| expected: OTelConfig{ | ||
| ServiceName: "lfx-v2-access-check", | ||
| ServiceVersion: "1.0.0", | ||
| Endpoint: "localhost:4317", | ||
| Protocol: "grpc", | ||
| TracesExporter: "none", | ||
| InsecureExporter: false, | ||
| }, | ||
| }, | ||
| { | ||
| name: "custom values", | ||
| envVars: map[string]string{ | ||
| "OTEL_SERVICE_NAME": "custom-service", | ||
| "OTEL_SERVICE_VERSION": "2.0.0", | ||
| "OTEL_EXPORTER_OTLP_ENDPOINT": "otel-collector:4317", | ||
| "OTEL_EXPORTER_OTLP_PROTOCOL": "http", | ||
| "OTEL_TRACES_EXPORTER": "otlp", | ||
| "OTEL_EXPORTER_OTLP_INSECURE": "true", | ||
| }, | ||
| expected: OTelConfig{ | ||
| ServiceName: "custom-service", | ||
| ServiceVersion: "2.0.0", | ||
| Endpoint: "otel-collector:4317", | ||
| Protocol: "http", | ||
| TracesExporter: "otlp", | ||
| InsecureExporter: true, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| // Clear relevant env vars | ||
| envKeys := []string{ | ||
| "OTEL_SERVICE_NAME", | ||
| "OTEL_SERVICE_VERSION", | ||
| "OTEL_EXPORTER_OTLP_ENDPOINT", | ||
| "OTEL_EXPORTER_OTLP_PROTOCOL", | ||
| "OTEL_TRACES_EXPORTER", | ||
| "OTEL_EXPORTER_OTLP_INSECURE", | ||
| } | ||
| for _, key := range envKeys { | ||
| os.Unsetenv(key) | ||
| } | ||
|
|
||
| // Set test env vars | ||
| for key, value := range tt.envVars { | ||
| os.Setenv(key, value) | ||
| } | ||
|
|
||
| config := OTelConfigFromEnv() | ||
|
|
||
| if config.ServiceName != tt.expected.ServiceName { | ||
| t.Errorf("ServiceName = %v, want %v", config.ServiceName, tt.expected.ServiceName) | ||
| } | ||
| if config.ServiceVersion != tt.expected.ServiceVersion { | ||
| t.Errorf("ServiceVersion = %v, want %v", config.ServiceVersion, tt.expected.ServiceVersion) | ||
| } | ||
| if config.Endpoint != tt.expected.Endpoint { | ||
| t.Errorf("Endpoint = %v, want %v", config.Endpoint, tt.expected.Endpoint) | ||
| } | ||
| if config.Protocol != tt.expected.Protocol { | ||
| t.Errorf("Protocol = %v, want %v", config.Protocol, tt.expected.Protocol) | ||
| } | ||
| if config.TracesExporter != tt.expected.TracesExporter { | ||
| t.Errorf("TracesExporter = %v, want %v", config.TracesExporter, tt.expected.TracesExporter) | ||
| } | ||
| if config.InsecureExporter != tt.expected.InsecureExporter { | ||
| t.Errorf("InsecureExporter = %v, want %v", config.InsecureExporter, tt.expected.InsecureExporter) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test suite doesn't cover the scenario where an invalid protocol value is provided (neither "grpc" nor "http"). While the implementation defaults to gRPC in this case (line 136-143 in otel.go), adding a test case would document this behavior and prevent regressions.
| go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.36.0 | ||
| go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.35.0 | ||
| go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.35.0 |
There was a problem hiding this comment.
The OpenTelemetry dependency versions are inconsistent across the module. The core SDK and trace packages are at version 1.39.0, while the otlptrace exporters are at mixed versions (v1.36.0 for the base package and v1.35.0 for the gRPC and HTTP variants). This version mismatch could lead to compatibility issues or unexpected behavior. All OpenTelemetry packages from the same release should typically use the same version to ensure compatibility.
| go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.36.0 | |
| go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.35.0 | |
| go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.35.0 | |
| go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.39.0 | |
| go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.39.0 | |
| go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.39.0 |
| // Clear relevant env vars | ||
| envKeys := []string{ | ||
| "OTEL_SERVICE_NAME", | ||
| "OTEL_SERVICE_VERSION", | ||
| "OTEL_EXPORTER_OTLP_ENDPOINT", | ||
| "OTEL_EXPORTER_OTLP_PROTOCOL", | ||
| "OTEL_TRACES_EXPORTER", | ||
| "OTEL_EXPORTER_OTLP_INSECURE", | ||
| } | ||
| for _, key := range envKeys { | ||
| os.Unsetenv(key) | ||
| } | ||
|
|
||
| // Set test env vars | ||
| for key, value := range tt.envVars { | ||
| os.Setenv(key, value) | ||
| } |
There was a problem hiding this comment.
The test manipulates global environment variables using os.Setenv and os.Unsetenv without proper cleanup, which can cause test pollution and race conditions when tests run in parallel. Consider using t.Setenv() instead, which was introduced in Go 1.17 and automatically handles cleanup and prevents parallel execution of tests that modify the same environment variable.
| // Clear env vars to use defaults | ||
| os.Unsetenv("OTEL_TRACES_EXPORTER") |
There was a problem hiding this comment.
The test modifies global environment variables without proper cleanup. Similar to TestOTelConfigFromEnv, consider using t.Setenv() instead of os.Unsetenv() for automatic cleanup and better test isolation.
| // Clear env vars to use defaults | |
| os.Unsetenv("OTEL_TRACES_EXPORTER") | |
| // Clear env vars to use defaults; t.Setenv ensures cleanup after the test | |
| t.Setenv("OTEL_TRACES_EXPORTER", "") |
| defer func() { | ||
| if shutdownErr := otelShutdown(context.Background()); shutdownErr != nil { | ||
| slog.Error("error shutting down OpenTelemetry SDK", "error", shutdownErr) | ||
| } | ||
| }() |
There was a problem hiding this comment.
The OTel shutdown is deferred to use a new context.Background() instead of the ctx variable that was used for initialization. This means any shutdown timeout or cancellation in the main context won't be respected during shutdown. Consider creating a separate shutdown context with a timeout (e.g., context.WithTimeout) to allow graceful shutdown with a reasonable deadline.
This pull request adds OpenTelemetry (OTel) tracing support to the
lfx-v2-access-checkservice, enabling distributed tracing and improved observability. The changes include initializing the OTel SDK with configuration from environment variables, instrumenting the HTTP server, and introducing utility code for OTel setup and configuration. Unit tests are also added to ensure correct OTel configuration handling.OpenTelemetry Integration:
pkg/utils/otel.go) to configure and initialize the OpenTelemetry SDK, supporting both gRPC and HTTP protocols, and allowing configuration via environment variables. This includes helpers for propagators, trace providers, and exporter setup.cmd/lfx-access-check/main.go) to initialize the OTel SDK at startup and ensure proper shutdown, using the new utility functions. [1] [2]otelhttp.NewHandler), enabling automatic tracing of incoming HTTP requests. [1] [2]Dependency and Testing Updates:
go.modto add required OpenTelemetry dependencies, update some existing dependencies, and ensure compatibility. [1] [2]pkg/utils/otel_test.go, covering environment variable parsing and basic SDK setup/shutdown scenarios.