feat: native stats/opentelemetry + OTEL metrics + OpenTracing removal#71
Merged
feat: native stats/opentelemetry + OTEL metrics + OpenTracing removal#71
Conversation
…cs, remove OpenTracing bridge Migrate gRPC OpenTelemetry instrumentation from the deprecated contrib otelgrpc package to the native grpc stats/opentelemetry package. Add opt-in OTEL metrics dual-export alongside Prometheus. Remove the OpenTracing bridge (already removed from tracing/ package). Key changes: - Default gRPC instrumentation now uses native stats/opentelemetry (ServerOption/DialOption) with MethodAttributeFilter for cardinality - New env vars: OTEL_USE_LEGACY_INSTRUMENTATION (rollback), ENABLE_OTEL_METRICS, OTEL_METRICS_INTERVAL - New public API: SetOTELOptions() for custom native OTel configuration - SetupOTELMetrics() creates MeterProvider sharing resource with traces - TracerProvider now registered for graceful shutdown (was never shut down) - UseOpenTracingBridge removed from OTLPConfig struct - OTLP_USE_OPENTRACING_BRIDGE env var kept but ignored (logs warning) - Legacy otelgrpc path retained behind OTEL_USE_LEGACY_INSTRUMENTATION=true Roadmap items: 8.1, 8.3
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves the OpenTracing bridge; adds native gRPC OpenTelemetry options via Changes
Sequence Diagram(s)sequenceDiagram
participant Config
participant Core as processConfig()
participant Init as SetupOpenTelemetry()/SetupOTELMetrics()
participant BuildOpts as buildOTELOptions()
participant grpcotel
participant otelgrpc
Config->>Core: processConfig() (read OTEL flags)
Core->>Init: SetupOpenTelemetry() (create tracer provider)
alt EnableOTELMetrics && OTLP endpoint available
Core->>Init: SetupOTELMetrics(interval)
Init-->>Core: otelMeterProvider (global)
end
Core->>BuildOpts: buildOTELOptions()
BuildOpts-->>Core: grpcotel.Options (or preserved user options)
Note over Core: choose instrumentation path during gRPC setup
Core->>grpcotel: apply native grpcotel options
alt legacy selected (OTEL_USE_LEGACY_INSTRUMENTATION=true)
Core->>otelgrpc: use legacy otelgrpc stats handler
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates ColdBrew’s observability stack by migrating gRPC tracing from deprecated otelgrpc contrib instrumentation to native grpc/stats/opentelemetry, adding optional OTLP metrics export alongside Prometheus, and removing the OpenTracing bridge (while keeping a warning-only compatibility env var).
Changes:
- Switch gRPC OTel instrumentation to native
google.golang.org/grpc/stats/opentelemetrywith a legacy fallback viaOTEL_USE_LEGACY_INSTRUMENTATION. - Add opt-in OTLP metrics export (
ENABLE_OTEL_METRICS,OTEL_METRICS_INTERVAL) and exposeOTELMeterProvider()/SetupOTELMetrics(). - Remove OpenTracing bridge plumbing and register the TracerProvider (and MeterProvider when enabled) for graceful shutdown.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates generated API docs to reflect new/changed OTEL APIs and removed OpenTracing bridge field. |
| initializers.go | Adds shared OTEL resource building, stores TracerProvider for shutdown, introduces OTLP metrics setup, removes OpenTracing bridge. |
| initializers_test.go | Adds tests for OTEL resource creation, OTEL tracing/metrics setup, and native grpcotel option building. |
| core.go | Integrates native grpcotel options into gRPC server/client wiring, adds shutdown handling for OTEL providers, adds new setter/builders. |
| config/config.go | Adds new env-configurable flags for legacy instrumentation and OTLP metrics export interval. |
| config/README.md | Updates generated config docs for new config fields and deprecation note changes. |
| go.mod / go.sum | Removes OpenTracing deps and adds OTLP metric exporter + OTEL metric/sdkmetric deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core.go`:
- Around line 165-176: The code only calls SetupOTELMetrics when
c.config.OTLPEndpoint is set, causing ENABLE_OTEL_METRICS to be ignored for the
New Relic OTLP path; update the metric initialization logic so SetupOTELMetrics
is invoked when metrics are enabled for either OTLP or New Relic OpenTelemetry.
Concretely, modify the condition that decides to call SetupOTELMetrics to check
(c.config.OTLPEndpoint != "" || c.config.NewRelicOpenTelemetry) &&
c.config.EnableOTELMetrics (or the equivalent config fields), and ensure the
OTLP/NR configuration path (where OTLPConfig is built) passes the correct values
into SetupOTELMetrics so metrics are initialized for the New Relic OTLP branch
as well.
- Around line 217-221: The otelGRPCOptionsSet flag is being used for two
meanings (user-provided vs auto-built) which prevents rolling back to legacy
instrumentation; introduce a second boolean (e.g., otelGRPCOptionsUserSet or
otelGRPCOptionsAutoBuilt) and update processConfig(), SetOTELOptions(), and the
native-build path that calls buildOTELOptions() so that: 1) SetOTELOptions()
sets the “user set” flag, 2) the auto-build path sets only the “auto-built”
flag, and 3) checks for OTELUseLegacyInstrumentation and New() / processConfig()
logic consult the user-set flag separately so a user override or legacy rollback
reliably wins and subsequent New() calls don’t inherit stale options. Ensure all
references to otelGRPCOptionsSet are replaced/adjusted to use the appropriate
new flag(s).
- Around line 192-214: The shutdown closers currently reference package globals
(otelTracerProvider, otelMeterProvider) which may be mutated later; capture the
concrete provider returned/used at setup time into local variables and close
those locals in the closer closures instead of the globals. Specifically, in the
tracer setup use a local variable (e.g., tp := otelTracerProvider) and append a
closer that calls tp.Shutdown(ctx), and in the metrics setup assign mp to a
local (e.g., mpLocal := mp) and append a closer that calls
mpLocal.Shutdown(ctx); update the code paths around SetupOTELMetrics,
otelTracerProvider assignment, and the c.closers append sites to reference these
locals so each instance shuts down the exact provider it created.
In `@README.md`:
- Around line 257-272: The README example for SetupOpenTelemetry generates an
unlabeled fenced code block causing markdownlint MD040; update the doc/generator
template that emits the OTLPConfig/SetupOpenTelemetry example so the fenced
block is language-tagged (use ```go) instead of a plain ``` fence; modify the
generator logic that formats the Example usage for SetupOpenTelemetry/OTLPConfig
to prepend the "go" tag when outputting the code fence so the rendered README no
longer triggers MD040.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7eab4f8-c427-4ff2-a585-e4a299903c6e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
README.mdconfig/README.mdconfig/config.gocore.gogo.modinitializers.goinitializers_test.go
- Add otelUseLegacy flag so OTEL_USE_LEGACY_INSTRUMENTATION reliably forces legacy otelgrpc even if SetOTELOptions() was called - Update buildOTELOptions comment to reflect no-op fallback behavior - Add ServiceName warning in SetupOTELMetrics for empty service identity - Add OTELMetricsInterval validation in Config.Validate() - Fix test cleanup: save/restore global TracerProvider, TextMapPropagator, and MeterProvider to prevent cross-test contamination
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
README.md (1)
257-272:⚠️ Potential issue | 🟡 MinorThe generated Jaeger example still opens with an unlabeled fence.
Line 263 still uses plain triple backticks, so markdownlint MD040 will keep firing. Since this file is generated, please fix the gomarkdoc template or source docs rather than editing
README.mddirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 257 - 272, The README's generated code block for the Jaeger example is missing a language label which triggers markdownlint MD040; update the gomarkdoc template or the source doc that generates the SetupOpenTelemetry/OTLPConfig example so the fenced code block starts with ```go instead of ```, i.e., ensure the template emits a labeled fence for the example usage of SetupOpenTelemetry and OTLPConfig so the generated README no longer violates MD040.core.go (3)
221-225:⚠️ Potential issue | 🟠 MajorSeparate user overrides from auto-built native OTEL options.
otelGRPCOptionsSetstill means both “SetOTELOptionswas called” and “processConfigauto-built defaults”. After the first auto-build, laterNew()calls skipbuildOTELOptions()even when the tracer or meter provider changed, so native gRPC instrumentation can keep stale no-op providers.🔧 Suggested fix
var ( - otelGRPCOptionsSet bool // true after processConfig builds or user calls SetOTELOptions - otelGRPCOptions grpcotel.Options // value used by getGRPCServerOptions / initHTTP - otelMeterProvider *sdkmetric.MeterProvider - otelUseLegacy bool // set from config; forces legacy otelgrpc even if SetOTELOptions was called + otelGRPCOptionsSet bool + otelGRPCOptionsUserSet bool + otelGRPCOptions grpcotel.Options + otelMeterProvider *sdkmetric.MeterProvider + otelUseLegacy bool ) @@ - if !otelUseLegacy && !otelGRPCOptionsSet { + if !otelUseLegacy && !otelGRPCOptionsUserSet { otelGRPCOptions = buildOTELOptions() otelGRPCOptionsSet = true } @@ func SetOTELOptions(opts grpcotel.Options) { otelGRPCOptions = opts otelGRPCOptionsSet = true + otelGRPCOptionsUserSet = true }Also applies to: 529-568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core.go` around lines 221 - 225, The flag otelGRPCOptionsSet conflates “user called SetOTELOptions()” with “we auto-built defaults via buildOTELOptions()”, causing buildOTELOptions() to be skipped on subsequent New() calls and leaving stale providers; introduce a separate boolean (e.g., otelGRPCOptionsUserSet) to track explicit user overrides in SetOTELOptions(), use otelGRPCOptionsUserSet to skip respecting user overrides while allowing auto-rebuild when otelUseLegacy is false and either processConfig or the tracer/meter provider changed, and update New(), SetOTELOptions(), and any code paths that set otelGRPCOptions/otelGRPCOptionsSet so that buildOTELOptions() runs whenever appropriate unless the user explicitly set options via the new user-set flag.
165-176:⚠️ Potential issue | 🟠 MajorWire
ENABLE_OTEL_METRICSthrough the New Relic OTLP branch.
SetupOTELMetricsstill only runs whenc.config.OTLPEndpointis set, but theNewRelicOpentelemetrybranch never populatesotlpConfig. Services using the existing New Relic OTLP path will get traces but never metrics, so the new flag is effectively ignored there.🔧 Suggested fix
} else if c.config.NewRelicOpentelemetry { + if strings.TrimSpace(c.config.NewRelicLicenseKey) != "" { + otlpConfig = OTLPConfig{ + Endpoint: "otlp.nr-data.net:4317", + Headers: map[string]string{"api-key": c.config.NewRelicLicenseKey}, + ServiceName: nrName, + ServiceVersion: c.config.ReleaseName, + SamplingRatio: c.config.NewRelicOpentelemetrySample, + Compression: "gzip", + } + } err := SetupNROpenTelemetry( nrName, c.config.NewRelicLicenseKey, c.config.ReleaseName, c.config.NewRelicOpentelemetrySample, @@ - if c.config.EnableOTELMetrics && c.config.OTLPEndpoint != "" { + if c.config.EnableOTELMetrics && otlpConfig.Endpoint != "" { interval := time.Duration(c.config.OTELMetricsInterval) * time.Second mp, err := SetupOTELMetrics(otlpConfig, interval)Also applies to: 180-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core.go` around lines 165 - 176, The NewRelicOpentelemetry branch never populates otlpConfig so ENABLE_OTEL_METRICS is ignored there; update the NewRelicOpentelemetry branch (where NewRelicOpentelemetry is handled) to build and assign otlpConfig the same way as the c.config.OTLPEndpoint branch (parse headers via parseHeaders(c.config.OTLPHeaders) and set Endpoint, Headers, ServiceName, ServiceVersion, SamplingRatio, Compression, Insecure), or alternatively invoke SetupOTELMetrics conditioned on c.config.ENABLE_OTEL_METRICS so SetupOTELMetrics and otlpConfig are executed for NewRelicOpentelemetry as well.
165-215:⚠️ Potential issue | 🟠 MajorRegister shutdown hooks only for the providers created by this call.
The tracer hook is still keyed off package-global state, so a later
New()can append a closer for a provider created by an earlier instance. Both closers also dereference globals at shutdown time, which means they can close whichever provider was assigned last instead of the one this instance initialized.🔧 Suggested fix
// Setup OpenTelemetry - custom OTLP takes precedence over New Relic + prevTracerProvider := otelTracerProvider var otlpConfig OTLPConfig if c.config.OTLPEndpoint != "" { @@ - if otelTracerProvider != nil { + if otelTracerProvider != nil && otelTracerProvider != prevTracerProvider { + tp := otelTracerProvider c.closers = append(c.closers, closerFunc(func() error { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - return otelTracerProvider.Shutdown(ctx) + return tp.Shutdown(ctx) })) } @@ } else { otelMeterProvider = mp + mpLocal := mp c.closers = append(c.closers, closerFunc(func() error { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - return otelMeterProvider.Shutdown(ctx) + return mpLocal.Shutdown(ctx) })) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core.go` around lines 165 - 215, The shutdown hooks are closing package-global otelTracerProvider/otelMeterProvider which can belong to other instances; instead, when calling SetupOpenTelemetry/SetupNROpenTelemetry and SetupOTELMetrics capture the returned provider in a local variable (e.g., tracerProvider, meterProvider), only append a closer to c.closers if that local provider is non-nil and the setup call actually created it, and have the closer call Shutdown on that local provider (not the package-global otelTracerProvider/otelMeterProvider); update references where you append closers in the New()/core initialization (the blocks that currently check otelTracerProvider and set otelMeterProvider) to use these local variables so each instance unregisters only the providers it created.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core.go`:
- Around line 221-225: The flag otelGRPCOptionsSet conflates “user called
SetOTELOptions()” with “we auto-built defaults via buildOTELOptions()”, causing
buildOTELOptions() to be skipped on subsequent New() calls and leaving stale
providers; introduce a separate boolean (e.g., otelGRPCOptionsUserSet) to track
explicit user overrides in SetOTELOptions(), use otelGRPCOptionsUserSet to skip
respecting user overrides while allowing auto-rebuild when otelUseLegacy is
false and either processConfig or the tracer/meter provider changed, and update
New(), SetOTELOptions(), and any code paths that set
otelGRPCOptions/otelGRPCOptionsSet so that buildOTELOptions() runs whenever
appropriate unless the user explicitly set options via the new user-set flag.
- Around line 165-176: The NewRelicOpentelemetry branch never populates
otlpConfig so ENABLE_OTEL_METRICS is ignored there; update the
NewRelicOpentelemetry branch (where NewRelicOpentelemetry is handled) to build
and assign otlpConfig the same way as the c.config.OTLPEndpoint branch (parse
headers via parseHeaders(c.config.OTLPHeaders) and set Endpoint, Headers,
ServiceName, ServiceVersion, SamplingRatio, Compression, Insecure), or
alternatively invoke SetupOTELMetrics conditioned on
c.config.ENABLE_OTEL_METRICS so SetupOTELMetrics and otlpConfig are executed for
NewRelicOpentelemetry as well.
- Around line 165-215: The shutdown hooks are closing package-global
otelTracerProvider/otelMeterProvider which can belong to other instances;
instead, when calling SetupOpenTelemetry/SetupNROpenTelemetry and
SetupOTELMetrics capture the returned provider in a local variable (e.g.,
tracerProvider, meterProvider), only append a closer to c.closers if that local
provider is non-nil and the setup call actually created it, and have the closer
call Shutdown on that local provider (not the package-global
otelTracerProvider/otelMeterProvider); update references where you append
closers in the New()/core initialization (the blocks that currently check
otelTracerProvider and set otelMeterProvider) to use these local variables so
each instance unregisters only the providers it created.
In `@README.md`:
- Around line 257-272: The README's generated code block for the Jaeger example
is missing a language label which triggers markdownlint MD040; update the
gomarkdoc template or the source doc that generates the
SetupOpenTelemetry/OTLPConfig example so the fenced code block starts with ```go
instead of ```, i.e., ensure the template emits a labeled fence for the example
usage of SetupOpenTelemetry and OTLPConfig so the generated README no longer
violates MD040.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5129c3e-2bc9-4df0-82d4-03fbc4825518
📒 Files selected for processing (5)
README.mdconfig/config.gocore.goinitializers.goinitializers_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- initializers_test.go
- Capture TracerProvider and MeterProvider in local vars for shutdown closures to avoid closing the wrong instance if globals are overwritten - Fix buildOTLPTraceExporterOpts comment (trace-only, not shared) - Remove unused tracetest.NewInMemoryExporter() and import
Build otlpConfig for the NR path so SetupOTELMetrics can reuse the NR endpoint. Change the metrics condition from checking OTLPEndpoint config field to checking otlpConfig.Endpoint, which is set for both custom OTLP and NR paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
initializers.go (1)
306-315: Consider one shared normalizer for OTLP transport settings.This re-implements the same endpoint/headers/compression/insecure handling as
buildOTLPTraceExporterOptson Lines 194-205. The next auth, TLS, or compression change will have to land in both places again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@initializers.go` around lines 306 - 315, The metric exporter opts block duplicates transport option handling found in buildOTLPTraceExporterOpts; extract a shared helper (e.g., normalizeOTLPTransportOptions or buildOTLPExporterTransportOpts) that accepts the config (or Endpoint, Headers, Compression, Insecure) and returns the base []otlpmetricgrpc.Option/[]otlpgrpc.Option slice, then call that helper from both buildOTLPTraceExporterOpts and the metrics exporter code (replace the current exporterOpts construction) so endpoint/headers/compression/insecure logic lives in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@initializers.go`:
- Around line 298-329: The warning about an empty ServiceName is emitted too
early; only validate ServiceName when the fallback resource path is used (i.e.,
when otelResource is nil). Update the SetupOTELMetrics logic to defer the check:
remove or move the initial log.Warn and, inside the block where r == nil (before
calling buildOTELResource), check config.ServiceName and either log a clear
warning or return an error if you want to fail fast; then call buildOTELResource
with the validated name. Refer to otelResource, buildOTELResource, and
config.ServiceName to locate and modify the code paths.
---
Nitpick comments:
In `@initializers.go`:
- Around line 306-315: The metric exporter opts block duplicates transport
option handling found in buildOTLPTraceExporterOpts; extract a shared helper
(e.g., normalizeOTLPTransportOptions or buildOTLPExporterTransportOpts) that
accepts the config (or Endpoint, Headers, Compression, Insecure) and returns the
base []otlpmetricgrpc.Option/[]otlpgrpc.Option slice, then call that helper from
both buildOTLPTraceExporterOpts and the metrics exporter code (replace the
current exporterOpts construction) so endpoint/headers/compression/insecure
logic lives in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b018371-a028-42f2-8ac1-79ed5540059d
📒 Files selected for processing (3)
core.goinitializers.goinitializers_test.go
✅ Files skipped from review due to trivial changes (1)
- initializers_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core.go
Move ServiceName check into the otelResource==nil branch so the warning doesn't fire in the normal trace-first flow where the resource is already populated. Return an error instead of warning when the fallback path needs a ServiceName but none is provided.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- SetupOTELMetrics now returns error for non-positive interval - TestSetupOpenTelemetry_StoresTracerProvider restores global TP/propagator - processConfig tests restore otelUseLegacy and otelMeterProvider
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
initializers_test.go (1)
17-42: RestoreotelResourceinTestBuildOTELResourceto avoid state leakage.This test mutates package-global
otelResourcebut never restores it, which can influence later tests.♻️ Suggested isolation fix
func TestBuildOTELResource(t *testing.T) { + oldRes := otelResource + defer func() { otelResource = oldRes }() + r, err := buildOTELResource("test-service", "v1.0.0") if err != nil { t.Fatalf("buildOTELResource() error: %v", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@initializers_test.go` around lines 17 - 42, TestBuildOTELResource mutates the package-global otelResource and doesn't restore it; save the original otelResource value at the start of TestBuildOTELResource and defer restoring it after the test (use a defer to set otelResource back to the saved value) so buildOTELResource can run without leaking state to other tests; reference the otelResource global and the TestBuildOTELResource/test setup around the buildOTELResource call to locate where to save and restore.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@initializers_test.go`:
- Around line 258-273: TestProcessConfig_NativeOTEL saves/restores package-level
globals but not the global OpenTelemetry tracer provider/propagator, allowing
processConfig() to leak OTel state into other tests; update the test to capture
the current global OTel state before calling processConfig() (use
otel.GetTracerProvider() and otel.GetTextMapPropagator()), then after the test
restore them with otel.SetTracerProvider(...) and
otel.SetTextMapPropagator(...). Ensure these save/restore calls are placed
alongside the existing defer that restores
otelGRPCOptions/otelTracerProvider/etc. so processConfig() cannot leave global
OTel state changed.
- Around line 119-123: The test TestSetupOTELMetrics_NoEndpoint currently
doesn't initialize otelResource so it may fail for a missing resource instead of
an empty endpoint; update the test to initialize otelResource to a valid non-nil
Resource (so resource validation passes) before calling
SetupOTELMetrics(OTLPConfig{}, 60*time.Second) and then assert that the error
returned is due to the empty endpoint. Reference SetupOTELMetrics and the
otelResource global in the test to locate where to set a valid resource.
---
Nitpick comments:
In `@initializers_test.go`:
- Around line 17-42: TestBuildOTELResource mutates the package-global
otelResource and doesn't restore it; save the original otelResource value at the
start of TestBuildOTELResource and defer restoring it after the test (use a
defer to set otelResource back to the saved value) so buildOTELResource can run
without leaking state to other tests; reference the otelResource global and the
TestBuildOTELResource/test setup around the buildOTELResource call to locate
where to save and restore.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64930d03-2c93-4e27-9cbe-ce2b93e21242
📒 Files selected for processing (2)
initializers.goinitializers_test.go
✅ Files skipped from review due to trivial changes (1)
- initializers.go
- Log error when ENABLE_OTEL_METRICS=true but no OTLP endpoint configured - TestSetupOTELMetrics_NoEndpoint: populate resource so test validates endpoint check, not resource fallback - TestProcessConfig_NativeOTEL: restore global OTel TP/propagator
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core.go (1)
233-237:⚠️ Potential issue | 🟠 MajorSeparate user-supplied OTEL options from auto-built ones.
The runtime selection now respects
otelUseLegacy, butotelGRPCOptionsSetstill means both “SetOTELOptions()was called” and “processConfig()auto-built native options once.” After the first native init, laterNew()calls skipbuildOTELOptions(), so they can keep a stale tracer/meter provider or miss OTEL metrics enabled on a subsequent config.🔧 Suggested fix
var ( - otelGRPCOptionsSet bool // true after processConfig builds or user calls SetOTELOptions - otelGRPCOptions grpcotel.Options // value used by getGRPCServerOptions / initHTTP - otelMeterProvider *sdkmetric.MeterProvider - otelUseLegacy bool // set from config; forces legacy otelgrpc even if SetOTELOptions was called + otelGRPCOptionsSet bool + otelGRPCOptionsUserSet bool + otelGRPCOptions grpcotel.Options + otelMeterProvider *sdkmetric.MeterProvider + otelUseLegacy bool ) ... - if !otelUseLegacy && !otelGRPCOptionsSet { + if !otelUseLegacy && !otelGRPCOptionsUserSet { otelGRPCOptions = buildOTELOptions() otelGRPCOptionsSet = true } } func SetOTELOptions(opts grpcotel.Options) { otelGRPCOptions = opts otelGRPCOptionsSet = true + otelGRPCOptionsUserSet = true }Also applies to: 541-580
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core.go` around lines 233 - 237, The code conflates user-supplied OTEL options with auto-built ones by using otelGRPCOptionsSet for both; change the logic so SetOTELOptions() sets a distinct flag (e.g., otelGRPCOptionsUserSet) while auto-built options set a separate flag (e.g., otelGRPCOptionsAutoBuilt), then update processConfig()/New() to call buildOTELOptions() whenever otelUseLegacy is false and otelGRPCOptionsUserSet is false (regardless of otelGRPCOptionsAutoBuilt) so subsequent New() calls won't skip rebuilding OTEL when a new config enables metrics; adjust references to otelGRPCOptionsSet throughout (including the block calling buildOTELOptions() and the other region around lines 541-580) to use the new flags accordingly.
🧹 Nitpick comments (1)
initializers_test.go (1)
265-371: Add a two-passprocessConfig()regression case.These cases all reset the OTEL globals before each call, so they never exercise the state that survives across successive
New()/processConfig()invocations. A native→native case that enables metrics on the second pass would catch the stale auto-builtotelGRPCOptionspath.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@initializers_test.go` around lines 265 - 371, Add a two-pass regression test that calls processConfig (or New() which calls processConfig) twice without resetting the OTEL globals between calls to simulate state that survives across invocations; specifically, first invoke processConfig with a native OTEL config that does not enable metrics, then invoke it again with a native config that sets EnableOTELMetrics=true (and OTLPEndpoint/OTLPInsecure as needed) and assert that otelGRPCOptions/otelGRPCOptionsSet are rebuilt or updated and otelMeterProvider becomes non-nil on the second pass; use the existing symbols processConfig, New, otelGRPCOptions, otelGRPCOptionsSet, and otelMeterProvider to locate where to add this new test and avoid resetting those globals between the two calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core.go`:
- Around line 233-237: The code conflates user-supplied OTEL options with
auto-built ones by using otelGRPCOptionsSet for both; change the logic so
SetOTELOptions() sets a distinct flag (e.g., otelGRPCOptionsUserSet) while
auto-built options set a separate flag (e.g., otelGRPCOptionsAutoBuilt), then
update processConfig()/New() to call buildOTELOptions() whenever otelUseLegacy
is false and otelGRPCOptionsUserSet is false (regardless of
otelGRPCOptionsAutoBuilt) so subsequent New() calls won't skip rebuilding OTEL
when a new config enables metrics; adjust references to otelGRPCOptionsSet
throughout (including the block calling buildOTELOptions() and the other region
around lines 541-580) to use the new flags accordingly.
---
Nitpick comments:
In `@initializers_test.go`:
- Around line 265-371: Add a two-pass regression test that calls processConfig
(or New() which calls processConfig) twice without resetting the OTEL globals
between calls to simulate state that survives across invocations; specifically,
first invoke processConfig with a native OTEL config that does not enable
metrics, then invoke it again with a native config that sets
EnableOTELMetrics=true (and OTLPEndpoint/OTLPInsecure as needed) and assert that
otelGRPCOptions/otelGRPCOptionsSet are rebuilt or updated and otelMeterProvider
becomes non-nil on the second pass; use the existing symbols processConfig, New,
otelGRPCOptions, otelGRPCOptionsSet, and otelMeterProvider to locate where to
add this new test and avoid resetting those globals between the two calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07c87d5e-e192-4a26-8b36-64c9e93c194c
📒 Files selected for processing (2)
core.goinitializers_test.go
Prevents OTEL metrics from attempting export to otlp.nr-data.net with an empty API key when SetupNROpenTelemetry already no-oped.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…if-blocks - Extract nrOTLPEndpoint constant to avoid duplicate string literals - Inline buildOTLPTraceExporterOpts (only used once, not shared with metrics) - Merge two mutually exclusive EnableOTELMetrics if-blocks into single if/else - Add prevTP tracking to only register TP closer when newly initialized - Cache otelResource in buildOTELResource to match docstring
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…der-dependent failures
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
otelgrpccontrib to nativegrpc stats/opentelemetry(roadmap 8.1)ENABLE_OTEL_METRICSalongside Prometheus (roadmap 8.3)New config env vars
OTEL_USE_LEGACY_INSTRUMENTATIONfalseENABLE_OTEL_METRICSfalseOTEL_METRICS_INTERVAL60New public API
SetOTELOptions(opts)— configure native gRPC stats/opentelemetry options during initSetupOTELMetrics(config, interval)— create MeterProvider with OTLP exporterBreaking changes
OTLPConfig.UseOpenTracingBridgefield removed (compile-time break)OTLP_USE_OPENTRACING_BRIDGEenv var kept but ignored (logs warning)Backward compatibility
OTEL_USE_LEGACY_INSTRUMENTATION=trueSetOTELGRPCServerOptions/SetOTELGRPCClientOptionsdeprecated but retained for legacy pathTest plan
make build && make test && make lintin core/make build && make test && make lintfrom rootOTLP_ENDPOINT, verify traces appear in OTLP backendENABLE_OTEL_METRICS=true, verify gRPC metrics exported via OTLPOTEL_USE_LEGACY_INSTRUMENTATION=true, verify legacy path worksOTLP_USE_OPENTRACING_BRIDGE=true, verify warning loggedSummary by CodeRabbit
New Features
Deprecated / Removed
Documentation
Tests