Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a generic OTLP tracing API (OTLPConfig + SetupOpenTelemetry), makes SetupNROpenTelemetry a wrapper, extends config.Config with OTLP and gRPC size fields, updates core initialization to prefer a custom OTLP endpoint before New Relic fallback, updates docs, and upgrades tooling and observability dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Core as SetupOpenTelemetry
participant GRPC as OTLP gRPC Client
participant Exp as OTLP Exporter
participant TP as TracerProvider
participant Bridge as OpenTracing Bridge (optional)
App->>Core: SetupOpenTelemetry(OTLPConfig)
Core->>Core: Validate config & apply defaults
Core->>GRPC: Build client (endpoint, headers, compression, insecure)
GRPC-->>Core: client options
Core->>Exp: Create OTLP exporter (grpc)
Exp-->>Core: exporter ready
Core->>TP: Build Resource + TracerProvider (sampling)
TP-->>Core: provider ready
alt UseOpenTracingBridge
Core->>Bridge: Install bridge
Bridge-->>Core: bridge active
end
Core-->>App: success / error
sequenceDiagram
autonumber
actor App
participant CoreProc as processConfig
participant OTLP as SetupOpenTelemetry
participant NR as SetupNROpenTelemetry
App->>CoreProc: processConfig(config)
alt config.OTLPEndpoint set
CoreProc->>OTLP: SetupOpenTelemetry(from config)
OTLP-->>CoreProc: err / success
else if NewRelicOpenTelemetry enabled
CoreProc->>NR: SetupNROpenTelemetry(...)
NR-->>CoreProc: err / success
else
CoreProc-->>App: skip OTEL setup
end
CoreProc-->>App: continue startup (logs on error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (12)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
Adds support for custom OpenTelemetry OTLP exporters, making the framework compatible with any OTLP-compatible backend (Jaeger, Honeycomb, etc.) instead of being limited to New Relic.
- Introduces a new generic
OTLPConfigstruct andSetupOpenTelemetryfunction for configuring any OTLP backend - Refactors the existing New Relic OpenTelemetry setup to use the new generic configuration
- Adds environment variable configuration for custom OTLP settings with precedence over New Relic
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| initializers.go | Introduces generic OTLP configuration and refactors New Relic setup to use the new implementation |
| config/config.go | Adds environment variables for custom OTLP configuration |
| core.go | Adds header parsing utility and integration logic for custom OTLP vs New Relic precedence |
| config/README.md | Updates documentation to reflect new OTLP configuration fields |
| README.md | Updates API documentation with new functions and types |
| go.mod | Updates Go version and dependency versions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
initializers.go (5)
134-170: Consider adding environment attribute support to OTLPConfig.Adding an optional Environment field and emitting semconv.DeploymentEnvironmentKey improves backend filtering.
Apply this diff and wire it in SetupOpenTelemetry:
type OTLPConfig struct { @@ // Insecure disables TLS verification for the connection // Only use this for local development or testing Insecure bool + + // Environment describes the deployment environment (e.g., "prod", "staging") + Environment string }Outside this hunk, in SetupOpenTelemetry’s resource attributes:
resource.WithAttributes( // the service name used to display traces in backends semconv.ServiceNameKey.String(config.ServiceName), semconv.ServiceVersionKey.String(config.ServiceVersion), + semconv.DeploymentEnvironmentKey.String(config.Environment), ),
211-221: Harden exporter connection: add retry/backoff and a bounded dial timeout.Without retry/timeout, exporter connection can be flaky or hang on startup.
Apply this diff:
// Build client options clientOpts := []otlptracegrpc.Option{ otlptracegrpc.WithEndpoint(config.Endpoint), otlptracegrpc.WithHeaders(config.Headers), + otlptracegrpc.WithRetry(otlptracegrpc.RetryConfig{Enabled: true}), + // Block connect so we fail fast within the context timeout below. + otlptracegrpc.WithDialOption(grpc.WithBlock()), } @@ - otlpExporter, err := otlptrace.New(context.Background(), otlptracegrpc.NewClient(clientOpts...)) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + otlpExporter, err := otlptrace.New(ctx, otlptracegrpc.NewClient(clientOpts...))Add the missing import in this file:
import ( + "google.golang.org/grpc" )Also applies to: 227-227
251-255: Clamp sampling ratio to [0,1] to avoid invalid sampler configuration.Protect against misconfigured env values.
- tracerProvider := sdktrace.NewTracerProvider( - sdktrace.WithSampler(sdktrace.ParentBased(sdktrace.TraceIDRatioBased(config.SamplingRatio))), + // Clamp sampling ratio to [0,1] + sr := config.SamplingRatio + if sr < 0 { + sr = 0 + } else if sr > 1 { + sr = 1 + } + tracerProvider := sdktrace.NewTracerProvider( + sdktrace.WithSampler(sdktrace.ParentBased(sdktrace.TraceIDRatioBased(sr))), sdktrace.WithBatcher(otlpExporter), sdktrace.WithResource(r), )
282-294: Skip NR OTLP init when license key is empty.Avoid unauthorized calls and noisy logs when the license is not set.
func SetupNROpenTelemetry(serviceName, license, version string, ratio float64) error { - // Use the generic SetupOpenTelemetry with New Relic specific configuration + // Use the generic SetupOpenTelemetry with New Relic specific configuration + if strings.TrimSpace(license) == "" { + log.Info(context.Background(), "msg", "not initializing NR OpenTelemetry: empty license key") + return nil + } config := OTLPConfig{ Endpoint: "otlp.nr-data.net:4317", Headers: map[string]string{"api-key": license}, ServiceName: serviceName, ServiceVersion: version, SamplingRatio: ratio, Compression: "gzip", UseOpenTracingBridge: true, } return SetupOpenTelemetry(config) }
257-266: Return a shutdown func to flush spans on graceful stop.Currently the tracer provider isn’t exposed for Shutdown, risking span loss on exit.
Consider changing the signature to return a shutdown function:
-func SetupOpenTelemetry(config OTLPConfig) error { +func SetupOpenTelemetry(config OTLPConfig) (func(context.Context) error, error) { @@ - return err + return nil, err @@ - } else { - otel.SetTracerProvider(tracerProvider) - } + } else { + otel.SetTracerProvider(tracerProvider) + } @@ - return nil + return tracerProvider.Shutdown, nilCall sites (e.g., in core.go) can then register this with your existing closers.
core.go (2)
63-79: Harden header parsing: skip empty keys/values and normalize header names.Prevents invalid metadata (empty key) and aligns with gRPC’s lowercase header requirement.
func parseHeaders(headerString string) map[string]string { headers := make(map[string]string) if headerString == "" { return headers } pairs := strings.Split(headerString, ",") for _, pair := range pairs { kv := strings.SplitN(strings.TrimSpace(pair), "=", 2) if len(kv) == 2 { - headers[strings.TrimSpace(kv[0])] = strings.TrimSpace(kv[1]) + key := strings.ToLower(strings.TrimSpace(kv[0])) + val := strings.TrimSpace(kv[1]) + if key == "" || val == "" { + continue + } + headers[key] = val } } return headers }
116-141: Log NR OTLP initialization errors; consider not starting Jaeger when OTLP is configured.
- Errors from SetupNROpenTelemetry are currently ignored.
- Optional: you initialize Jaeger earlier regardless; when OTLP is enabled, consider skipping Jaeger to avoid redundant init.
} else if c.config.NewRelicOpentelemetry { // Fall back to New Relic OpenTelemetry if no custom OTLP is configured - SetupNROpenTelemetry( - nrName, - c.config.NewRelicLicenseKey, - c.config.ReleaseName, - c.config.NewRelicOpentelemetrySample, - ) + if err := SetupNROpenTelemetry( + nrName, + c.config.NewRelicLicenseKey, + c.config.ReleaseName, + c.config.NewRelicOpentelemetrySample, + ); err != nil { + log.Error(context.Background(), "msg", "Failed to setup NR OTLP", "err", err) + } }README.md (1)
145-182: Fix markdown lint: specify language for fenced code blocks in generated docs.MD040 flagged missing language on code fences. Either:
- Update comments to use ```go fences so gomarkdoc carries the language, or
- Exclude generated docs from markdownlint.
go.mod (3)
39-41: Duplicate major versions of backoff (v4 and v5).Both v4 and v5 are required indirectly. If nothing imports v4 explicitly, drop v4 to reduce graph size.
Run go mod tidy and inspect imports; if no direct/indirect consumer pins v4, remove:
- github.com/cenkalti/backoff/v4 v4.3.0 // indirect
63-63: Inconsistent OTel internal retry module version.
go.opentelemetry.io/otel/exporters/otlp/internal/retry v1.17.0is far behind the rest of OTel (v1.38.0) and is likely unnecessary to pin. Let the resolver choose the matching transitive version.Apply:
- go.opentelemetry.io/otel/exporters/otlp/internal/retry v1.17.0 // indirectThen run
go mod tidy.
72-74: Avoid mixing genproto root and scoped submodules.You have root
google.golang.org/genprotoat an older snapshot alongside newer.../googleapis/{api,rpc}. Prefer only the scoped submodules to avoid version skew.Apply:
- google.golang.org/genproto v0.0.0-20240903143218-8af14fe29dc1 // indirectThen run
go mod tidyto reconcile.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
README.md(8 hunks)config/README.md(2 hunks)config/config.go(1 hunks)core.go(2 hunks)go.mod(2 hunks)initializers.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core.go (1)
initializers.go (2)
OTLPConfig(138-170)SetupOpenTelemetry(200-270)
🪛 markdownlint-cli2 (0.18.1)
README.md
158-158: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
172-172: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
config/README.md (1)
19-144: Config additions look consistent and well documented.Fields and env tags align with the new OTLP setup flow.
config/config.go (1)
106-129: LGTM — OTLP headers not logged in the repo.parseHeaders (core.go) parses c.config.OTLPHeaders into OTLPConfig and I found no logging/printing/marshalling of the raw or parsed headers.
go.mod (13)
3-3: Go 1.24 directive: verify CI/tooling availability.Bumping to go 1.24.0 (toolchain 1.24.1) looks fine. Ensure your CI images and devtooling have Go 1.24.1 to avoid build drift.
12-13: LGTM: interceptor/log bumps.Incremental, non-breaking updates.
19-19: LGTM: grpc-gateway v2.27.2.
21-21: LGTM: New Relic agent v3.40.1.
23-23: LGTM: Prometheus client_golang v1.23.2.
25-29: LGTM: OTel stack aligned at v1.38.0.Good version alignment across otel core/exporters/sdk.
Please run go mod tidy to ensure no hidden transitive mismatches remain after this upgrade.
31-32: LGTM: grpc/protobuf updates.These are compatible with the newer grpc-gateway/OTel libs.
47-47: LGTM: logr v1.4.3.
55-55: LGTM: nrgrpc v1.4.7.
57-60: LGTM: Prometheus indirects refreshed.
62-62: LGTM: go.opentelemetry.io/auto/sdk v1.2.1.
64-66: LGTM: otel metric/trace + proto/otlp.
69-71: LGTM: x/ indirects updated.*
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
initializers.go (3)
251-255: Clamp sampling ratio to [0,1] and default to 1.0.Prevents accidental “no traces” or invalid sampler configs.
+ // Clamp/Default sampling ratio + ratio := config.SamplingRatio + if ratio <= 0 || ratio > 1 { + ratio = 1.0 + } tracerProvider := sdktrace.NewTracerProvider( - sdktrace.WithSampler(sdktrace.ParentBased(sdktrace.TraceIDRatioBased(config.SamplingRatio))), + sdktrace.WithSampler(sdktrace.ParentBased(sdktrace.TraceIDRatioBased(ratio))), sdktrace.WithBatcher(otlpExporter), sdktrace.WithResource(r), )
251-269: Add a shutdown hook to flush spans on process exit.Without Shutdown, spans can be dropped on termination.
Proposal:
- Keep a package-level tracer provider reference in SetupOpenTelemetry.
- Expose ShutdownOpenTelemetry(ctx) and call it from your signal handler.
Example:
var otelTP *sdktrace.TracerProvider func SetupOpenTelemetry(cfg OTLPConfig) error { // ... otelTP = tracerProvider // ... } func ShutdownOpenTelemetry(ctx context.Context) error { if otelTP != nil { return otelTP.Shutdown(ctx) } return nil }
283-294: Skip NR OTLP init when license is empty (parity with SetupNewRelic).Avoids unnecessary connection attempts and clearer logs.
func SetupNROpenTelemetry(serviceName, license, version string, ratio float64) error { + if strings.TrimSpace(license) == "" { + log.Info(context.Background(), "msg", "not initializing opentelemetry (nr): missing license key") + return nil + } // Use the generic SetupOpenTelemetry with New Relic specific configuration config := OTLPConfig{ Endpoint: "otlp.nr-data.net:4317", Headers: map[string]string{"api-key": license}, ServiceName: serviceName, ServiceVersion: version, SamplingRatio: ratio, Compression: "gzip", UseOpenTracingBridge: true, } return SetupOpenTelemetry(config) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
initializers.go(3 hunks)
🔇 Additional comments (1)
initializers.go (1)
257-266: LGTM: bridge tracer uses a non-empty, descriptive name.Using service name resolves prior feedback.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
core.go (3)
137-137: Fix inconsistent variable declaration styleThere's an extra space before
errwhich makes the declaration inconsistent with the surrounding code style.- err := SetupNROpenTelemetry( + err := SetupNROpenTelemetry(
474-479: Consider simplifying the goroutine closureThe goroutine captures both
ctxandc, butcis already available in the outer scope. You only need to passctxsince it's the one that changes scope. Also, the context is already available in the goroutine without passing it explicitly.- go func(ctx context.Context, c *cb) { + go func() { err := c.httpServer.Shutdown(ctx) if err != nil { log.Error(context.Background(), "msg", "http server shutdown error", "err", err) } - }(ctx, c) // shutdown http server gracefully + }() // shutdown http server gracefully
65-81: Consider adding unit tests for parseHeaders functionThe
parseHeadersfunction handles critical configuration parsing for OTLP headers. Consider adding unit tests to verify correct behavior with various input formats, including edge cases like empty strings, malformed pairs, and pairs with multiple equals signs.Would you like me to generate comprehensive unit tests for the
parseHeadersfunction to ensure robust header parsing?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core.go (1)
initializers.go (3)
OTLPConfig(138-170)SetupOpenTelemetry(200-270)SetupNROpenTelemetry(282-294)
🔇 Additional comments (3)
core.go (3)
77-77: Good improvement over previous review commentThe warning message now includes the actual malformed pair value, which addresses the previous review suggestion about improving debugging.
119-134: Well-structured OTLP configuration precedenceThe implementation correctly prioritizes custom OTLP endpoints over New Relic OpenTelemetry, providing good flexibility for users to choose their telemetry backend. The error logging is appropriate for debugging configuration issues.
440-443: Good addition of error logging for resource cleanupAdding error logging when closing resources improves observability during shutdown. This helps identify potential resource leaks or cleanup failures.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Summary by CodeRabbit
New Features
Documentation
Chores