feat: migrate from OpenTracing to native OpenTelemetry APIs#20
Conversation
Replace all OpenTracing usage with native OTEL APIs: - tracingSpan.openSpan (opentracing.Span) → otelSpan (trace.Span) - opentracing.StartSpanFromContext → otel.Tracer().Start() - span.SetTag → span.SetAttributes(attribute.String()) - span.SetError tags → span.SetStatus(codes.Error) + RecordError() - opentracing.GlobalTracer().Inject/Extract → otel.GetTextMapPropagator() - metadataReaderWriter → metadataCarrier (propagation.TextMapCarrier) - ClientSpan returns trace.Span instead of opentracing.Span Remove opentracing-go dependency entirely. Add OTEL dependencies: go.opentelemetry.io/otel, trace, attribute, codes, propagation. Public API preserved: Span interface, NewInternalSpan, NewDatastoreSpan, NewExternalSpan, NewHTTPExternalSpan, context helpers all unchanged. NewRelic dual-reporting preserved alongside OTEL spans.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces OpenTracing with OpenTelemetry across docs, implementation, tests, and dependency manifest: spans now use oteltrace.Span with End(), tags become OTEL attributes/status/error recordings, and HTTP/gRPC propagation uses OpenTelemetry propagators. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as "Application Handler"
participant gRPC_Server as "gRPC Server"
participant metadataCarrier as "metadataCarrier"
participant Propagator as "OTel Propagator"
participant Tracer as "otel.Tracer"
Client->>gRPC_Server: incoming RPC with metadata
gRPC_Server->>metadataCarrier: wrap incoming metadata
metadataCarrier->>Propagator: Extract trace context
Propagator->>Tracer: provide context for Start
Tracer->>gRPC_Server: return server span + context (SpanKindServer)
gRPC_Server->>Propagator: prepare outgoing metadata
Propagator->>metadataCarrier: Inject trace context
metadataCarrier->>Client: outgoing metadata with trace context
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Migrates the tracing package off deprecated OpenTracing usage and onto native OpenTelemetry APIs while preserving the existing public Span abstraction and New Relic dual-reporting.
Changes:
- Replaced OpenTracing spans/propagation with OpenTelemetry
trace.Spanandotel.GetTextMapPropagator(). - Updated gRPC metadata propagation to use an OTEL
TextMapCarrieradapter. - Updated tests, docs, and module dependencies to remove
opentracing-goand add OTEL modules.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tracing.go |
Swaps the underlying span implementation to OTEL, updates span creation/attributes/status, and updates HTTP + gRPC propagation. |
tracing_test.go |
Updates unit tests for OTEL spans and new metadata carrier; adds HTTP external span test. |
go.mod |
Removes opentracing-go and adds OTEL requirements. |
go.sum |
Updates dependency checksums to reflect OTEL + indirect deps. |
README.md |
Regenerated docs reflecting signature changes (e.g., ClientSpan return type). |
💡 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: 3
🧹 Nitpick comments (1)
tracing_test.go (1)
21-24: These new OTEL tests are still effectively no-ops.
TestNewInternalSpannever asserts on theIsValidbranch, andTestNewHTTPExternalSpanonly checks for non-nil returns. With the default noop tracer provider/propagator, both pass even if context attachment or header injection breaks. Please install a test tracer provider pluspropagation.TraceContext{}and assert a valid span context /traceparentround-trip.Also applies to: 134-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tracing_test.go` around lines 21 - 24, Tests TestNewInternalSpan and TestNewHTTPExternalSpan are no-ops because they rely on the default noop tracer/propagator; install a test tracer provider (e.g., the oteltest or sdk trace test provider) and set propagation.TraceContext{} as the global propagator before running the tests, then in TestNewInternalSpan assert oteltrace.SpanFromContext(newCtx).SpanContext().IsValid() is true, and in TestNewHTTPExternalSpan assert that the injected header (e.g., "traceparent") round-trips by extracting it back with the same TraceContext propagator and confirming the extracted span context is valid; update the test setup/teardown to restore global tracer/provider and propagator after the tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tracing.go`:
- Around line 249-252: The change to have ClientSpan return oteltrace.Span (via
clientSpanOTEL) is a compile-time breaking API change; either bump the module
major version in go.mod to v2 (update the module path to include /v2) or
increment the support package version constant (e.g., change
SupportPackageIsVersion1 to the next sentinel) so consumers fail fast; update
module versioning and any relevant documentation/release notes to indicate this
explicit compatibility break and ensure ClientSpan and clientSpanOTEL signatures
are reflected consistently across the package.
- Around line 74-79: In tracingSpan.SetTag, avoid stringifying every tag before
calling span.otelSpan.SetAttributes: inspect the runtime type of value and call
the matching OTEL attribute constructor (attribute.Int64, attribute.Bool,
attribute.Float64, etc.), falling back to attribute.String(fmt.Sprint(value))
only for unsupported types; keep the existing AWS/AddAttribute calls unchanged
so datastore/segment behavior remains the same and ensure the attribute passed
into span.otelSpan.SetAttributes uses the typed attribute you constructed.
- Around line 257-277: GRPCTracingSpan currently copies the full incoming
metadata (md.Copy()) and reuses it as the outgoing carrier, which can leak
sensitive headers; change it to extract the trace context from the incoming
metadata using metadataCarrier(md) but create a fresh outgoing metadata.MD{}
(e.g. mdOut := metadata.MD{}) and call prop.Inject(ctx, metadataCarrier(mdOut)),
then set ctx = metadata.NewOutgoingContext(ctx, mdOut) instead of reusing the
copied incoming md; keep the Start call and use metadataCarrier for extraction
only (references: function GRPCTracingSpan, metadataCarrier, tracerName).
---
Nitpick comments:
In `@tracing_test.go`:
- Around line 21-24: Tests TestNewInternalSpan and TestNewHTTPExternalSpan are
no-ops because they rely on the default noop tracer/propagator; install a test
tracer provider (e.g., the oteltest or sdk trace test provider) and set
propagation.TraceContext{} as the global propagator before running the tests,
then in TestNewInternalSpan assert
oteltrace.SpanFromContext(newCtx).SpanContext().IsValid() is true, and in
TestNewHTTPExternalSpan assert that the injected header (e.g., "traceparent")
round-trips by extracting it back with the same TraceContext propagator and
confirming the extracted span context is valid; update the test setup/teardown
to restore global tracer/provider and propagator after the tests.
🪄 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: 51102c71-12c2-4d8e-b970-669943691a8a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
README.mdgo.modtracing.gotracing_test.go
- SetTag now preserves value types (int, float64, bool, string) instead of stringifying everything via fmt.Sprint - GRPCTracingSpan: use fresh outgoing metadata instead of reusing inbound metadata for downstream propagation - Remove empty test assertion block - Regenerate README
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tracing_test.go (2)
72-89: BroadenTestMetadataCarriercoverage beyond single-key happy path.Current assertions only validate one key/value path. Consider adding table-driven cases for overwrite behavior and multiple keys to better guard carrier adapter regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tracing_test.go` around lines 72 - 89, Expand TestMetadataCarrier to cover more cases: add a table-driven set of subtests that exercise metadataCarrier's Set, Get and Keys for (a) overwriting an existing key (call metadataCarrier(md).Set("x-request-id","v1") then Set(...,"v2") and assert Get returns "v2" and Keys contains the key once), (b) multiple distinct keys (set several keys and assert Get returns each value and Keys contains all keys), and (c) missing keys (Get returns empty string). Use the existing TestMetadataCarrier test, the metadataCarrier constructor, and its Set/Get/Keys methods to implement these subtests. Ensure assertions validate both values and that Keys length/contents match expectations.
131-143:TestNewHTTPExternalSpanshould assert propagation side-effects.The test only checks non-nil values, but
NewHTTPExternalSpaninjects OTEL trace context into headers viaotel.GetTextMapPropagator().Inject(). Without an explicit propagator configuration, the test relies on the default global propagator (which may be noop), and header injection regressions would go unnoticed. Set a deterministic propagator and assert on the injected header.Suggested test hardening
import ( "context" "errors" + "net/http" "testing" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/propagation" "google.golang.org/grpc/metadata" ) @@ func TestNewHTTPExternalSpan(t *testing.T) { + prev := otel.GetTextMapPropagator() + otel.SetTextMapPropagator(propagation.TraceContext{}) + defer otel.SetTextMapPropagator(prev) + ctx := context.Background() - hdr := make(map[string][]string) + hdr := http.Header{} span, newCtx := NewHTTPExternalSpan(ctx, "external-svc", "/api/data", hdr) if span == nil { t.Fatal("expected non-nil span") } if newCtx == nil { t.Fatal("expected non-nil context") } - // Headers may contain trace propagation if a real propagator is configured. + if hdr.Get("traceparent") == "" { + t.Fatal("expected traceparent header to be injected") + } span.End() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tracing_test.go` around lines 131 - 143, TestNewHTTPExternalSpan currently only checks non-nil span/context but doesn’t verify that NewHTTPExternalSpan injects propagation headers via otel.GetTextMapPropagator().Inject; to fix, set a deterministic global propagator at the start of the test (e.g., use propagation.TraceContext{} or a known composite) so injection is predictable, call NewHTTPExternalSpan(ctx, "external-svc", "/api/data", hdr), then assert that hdr contains the expected propagation key/value (e.g., "traceparent" or the key your chosen propagator uses) and that its value matches the produced span/context; afterwards restore the previous global propagator if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 45-52: Update the README package introduction to consistently
state that tracing uses OpenTelemetry (OTEL) instead of OpenTracing, align
terminology with the documented OTEL types and functions (e.g., ClientSpan and
oteltrace.Span in tracing.go), and then regenerate the docs so the intro,
function descriptions, and examples all reference OTEL APIs and types
consistently.
---
Nitpick comments:
In `@tracing_test.go`:
- Around line 72-89: Expand TestMetadataCarrier to cover more cases: add a
table-driven set of subtests that exercise metadataCarrier's Set, Get and Keys
for (a) overwriting an existing key (call
metadataCarrier(md).Set("x-request-id","v1") then Set(...,"v2") and assert Get
returns "v2" and Keys contains the key once), (b) multiple distinct keys (set
several keys and assert Get returns each value and Keys contains all keys), and
(c) missing keys (Get returns empty string). Use the existing
TestMetadataCarrier test, the metadataCarrier constructor, and its Set/Get/Keys
methods to implement these subtests. Ensure assertions validate both values and
that Keys length/contents match expectations.
- Around line 131-143: TestNewHTTPExternalSpan currently only checks non-nil
span/context but doesn’t verify that NewHTTPExternalSpan injects propagation
headers via otel.GetTextMapPropagator().Inject; to fix, set a deterministic
global propagator at the start of the test (e.g., use propagation.TraceContext{}
or a known composite) so injection is predictable, call NewHTTPExternalSpan(ctx,
"external-svc", "/api/data", hdr), then assert that hdr contains the expected
propagation key/value (e.g., "traceparent" or the key your chosen propagator
uses) and that its value matches the produced span/context; afterwards restore
the previous global propagator if needed.
🪄 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: cfc1092b-4244-4032-9a2d-e016fe735726
📒 Files selected for processing (3)
README.mdtracing.gotracing_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tracing.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- GRPCTracingSpan: preserve existing outgoing metadata instead of replacing it, avoiding loss of non-tracing headers (auth, request-id) - Fix test: use http.Header instead of map[string][]string - Update doc.go to describe OTEL (not OpenTracing), regenerate README
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tracing.go (1)
143-153:⚠️ Potential issue | 🟠 MajorHandle the case where New Relic is not configured.
When
NewRelicAppis nil,nrutil.StartNRTransaction()returns the context unchanged (no-op). This meanstxnremains nil, but the code unconditionally callstxn.StartSegmentNow()on lines 151, 191, and 229, causing a nil pointer panic.After the transaction fetch-or-start block, check whether
txnis still nil. If it is, skip the New Relic segment setup and any subsequenttxn.End()ortxn.AddAttribute()calls for that transaction, or return an error if the transaction is required.This applies to all three constructor sites:
NewInternalSpan(143–153),NewDatastoreSpan(184–195), andbuildExternalSpan(222–231).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tracing.go` around lines 143 - 153, The code calls txn.StartSegmentNow() and later txn.End()/txn.AddAttribute() without handling the case New Relic is not configured; update the three constructors NewInternalSpan, NewDatastoreSpan and buildExternalSpan to check if txn is nil after calling nrutil.StartNRTransaction(name, ctx, ...) and, if nil, skip creating the newrelic.Segment and any subsequent txn operations (StartSegmentNow, End, AddAttribute) for that span (or return early if a transaction is required); ensure you only call txn.* methods when txn != nil and keep the existing flow for when txn is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tracing.go`:
- Around line 254-257: In NewHTTPExternalSpan, guard against a nil http.Header
before injecting OTEL context: check if hdr == nil and if so initialize it with
hdr = make(http.Header) before calling otel.GetTextMapPropagator().Inject(ctx,
propagation.HeaderCarrier(hdr)); this prevents the "assignment to entry in nil
map" panic when propagation.HeaderCarrier(hdr) is written to after
buildExternalSpan has returned the context.
- Around line 305-310: The metadataCarrier.Get currently returns only the first
value from metadata.MD(mc).Get(key) which discards additional comma-separated
propagation entries; change metadataCarrier.Get to join all values returned by
metadata.MD(mc).Get(key) with commas (preserving original order) and return the
combined string so baggage/tracestate propagation is preserved; locate the Get
method on type metadataCarrier and replace the vals[0] return with a
comma-joined return of vals (handling the zero-length case as before).
---
Outside diff comments:
In `@tracing.go`:
- Around line 143-153: The code calls txn.StartSegmentNow() and later
txn.End()/txn.AddAttribute() without handling the case New Relic is not
configured; update the three constructors NewInternalSpan, NewDatastoreSpan and
buildExternalSpan to check if txn is nil after calling
nrutil.StartNRTransaction(name, ctx, ...) and, if nil, skip creating the
newrelic.Segment and any subsequent txn operations (StartSegmentNow, End,
AddAttribute) for that span (or return early if a transaction is required);
ensure you only call txn.* methods when txn != nil and keep the existing flow
for when txn is present.
🪄 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: cf895193-b0e4-4c0d-9f74-23c08da46e25
📒 Files selected for processing (4)
README.mddoc.gotracing.gotracing_test.go
✅ Files skipped from review due to trivial changes (1)
- doc.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tracing_test.go
- NewHTTPExternalSpan: skip Inject when hdr is nil to avoid panic - metadataCarrier.Get: join multiple values with comma for W3C baggage/tracestate compatibility
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -191,7 +216,7 @@ func buildExternalSpan(ctx context.Context, name string, url string) (*tracingSp | |||
| url = "http://" + name + "/" + url | |||
There was a problem hiding this comment.
buildExternalSpan normalizes url to start with /, then unconditionally prefixes it with "http://" + name + "/", which produces an extra slash for already-normalized paths (e.g. /api becomes http://svc//api). Consider concatenating without the additional / once url is guaranteed to be rooted (or only inserting the / when needed) so the recorded URL is canonical.
| url = "http://" + name + "/" + url | |
| url = "http://" + name + url |
| "net/http" | ||
| "runtime/trace" | ||
| "strings" | ||
|
|
||
| nrutil "github.com/go-coldbrew/tracing/newrelic" | ||
| newrelic "github.com/newrelic/go-agent/v3/newrelic" | ||
| opentracing "github.com/opentracing/opentracing-go" | ||
| otext "github.com/opentracing/opentracing-go/ext" | ||
| "go.opentelemetry.io/otel" | ||
| "go.opentelemetry.io/otel/attribute" | ||
| "go.opentelemetry.io/otel/codes" | ||
| "go.opentelemetry.io/otel/propagation" | ||
| oteltrace "go.opentelemetry.io/otel/trace" | ||
| "google.golang.org/grpc/metadata" |
There was a problem hiding this comment.
ClientSpan now returns oteltrace.Span because go.opentelemetry.io/otel/trace is imported with the local name oteltrace (to avoid clashing with runtime/trace). This leaks the alias into generated docs/README signatures and makes examples harder to copy/paste. Consider aliasing runtime/trace instead (e.g. runtimetrace) and importing OTEL as trace so public signatures render as trace.Span.
| require ( | ||
| github.com/newrelic/go-agent/v3 v3.42.0 | ||
| github.com/opentracing/opentracing-go v1.2.0 | ||
| go.opentelemetry.io/otel v1.42.0 |
There was a problem hiding this comment.
Go modules for OpenTelemetry are typically released in lockstep; here go.opentelemetry.io/otel and go.opentelemetry.io/otel/trace are pinned to v1.42.0, but the dependency graph still pulls in go.opentelemetry.io/otel/sdk v1.39.0 (per go.sum). To reduce the risk of subtle incompatibilities, consider aligning OTEL module versions (e.g., explicitly upgrading otel/sdk/related modules to the same minor) and re-running go mod tidy.
| go.opentelemetry.io/otel v1.42.0 | |
| go.opentelemetry.io/otel v1.42.0 | |
| go.opentelemetry.io/otel/sdk v1.42.0 |
Summary
Replace all OpenTracing usage with native OpenTelemetry APIs. This removes the deprecated
opentracing-godependency entirely.What changed
tracingSpannow wrapstrace.Span(OTEL) instead ofopentracing.Spanotel.Tracer().Start()instead ofopentracing.StartSpanFromContext()SetTagusesspan.SetAttributes(attribute.String())instead ofspan.SetTag()SetErrorusesspan.SetStatus(codes.Error)+RecordError()instead of tag-based errorsotel.GetTextMapPropagator()instead ofopentracing.GlobalTracer().Inject/ExtractmetadataReaderWriterreplaced withmetadataCarrierimplementingpropagation.TextMapCarrierClientSpanreturnstrace.Spaninstead ofopentracing.Span(breaking for direct callers)What stays the same
Spaninterface (End,Finish,SetTag,SetQuery,SetError) — unchangedNewInternalSpan,NewDatastoreSpan,NewExternalSpan,NewHTTPExternalSpan— same signaturesCloneContextValues,MergeContextValues) — unchangedBreaking changes
ClientSpanreturns(context.Context, trace.Span)instead of(context.Context, opentracing.Span)— only used by interceptors package internallyopentracing-godependency removed — callers importing it transitively via this package will need their own dependencyDependencies
go.opentelemetry.io/otel,otel/trace,otel/attribute,otel/codes,otel/propagationgithub.com/opentracing/opentracing-goTest plan
go test -race ./...— all passmake lint— 0 issues, 0 vulnerabilitiesmake doc— READMEs regeneratedNext steps (separate PRs)
errors/notifierto use OTEL span contextinterceptorsto useotelgrpcmiddlewarecoreto flip bridge default to falseSummary by CodeRabbit