Add support for otel spans #11
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 OpenTelemetry integration alongside OpenTracing: new otel helper package, OTEL span creation/propagation/attributes/error recording across client/server/internal/datastore spans, updates ClientSpan to return an OTEL span, and updates docs, go.mod, and CI Go version. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant App as Application
participant Tr as tracing.ClientSpan
participant OT as OpenTracing
participant OTel as OpenTelemetry
participant Prop as Propagators
Caller->>App: call operation(ctx)
App->>Tr: ClientSpan(opName, ctx)
Tr->>OT: StartSpan(kind=client)
Tr->>OTel: StartSpan(kind=client)
Tr->>Prop: Inject OT & OTEL into headers/metadata
Tr-->>App: (ctx', opentracing.Span, oteltracing.Span)
rect rgba(220,240,230,0.6)
note right of App: Outgoing HTTP/gRPC call with propagated contexts
end
App->>Tr: SetTag / SetError / SetQuery
Tr->>OT: SetTag / LogError
Tr->>OTel: SetAttributes / RecordError / SetStatus
App->>Tr: Finish/End
Tr->>OT: Finish (guarded)
Tr->>OTel: End
sequenceDiagram
autonumber
actor Client
participant Svc as gRPC Server
participant Tr as tracing.GRPCTracingSpan
participant OT as OpenTracing
participant OTel as OpenTelemetry
Client->>Svc: gRPC request (metadata with propagated context)
Svc->>Tr: GRPCTracingSpan(opName, ctx)
Tr->>OT: StartSpan(kind=server, extract)
Tr->>OTel: StartSpan(kind=server, extract)
Tr-->>Svc: (ctx', opentracing.Span, oteltracing.Span)
Svc->>Tr: SetStatus / Finish
Tr->>OT: Finish (guarded)
Tr->>OTel: SetStatus / End
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![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
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)
363-391: Preserve OTEL parent context inGRPCTracingSpan.We start an OTEL span but never extract upstream OTEL context from the incoming metadata or inject the new span back into the outgoing metadata. That breaks distributed traces for any caller relying on OTEL headers—every gRPC span becomes a new root instead of attaching to its parent. Please use
otel.GetTextMapPropagator()(with an appropriate metadata carrier) to extract beforeStart(...)and inject after, so the OTEL span shares the same propagation guarantees we give to OpenTracing.
📜 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 (4)
README.md(7 hunks)go.mod(1 hunks)newrelic/README.md(2 hunks)tracing.go(12 hunks)
🧰 Additional context used
🪛 GitHub Actions: Go
go.mod
[error] 3-3: go: errors parsing go.mod: invalid go version '1.24.2': must match format 1.23
There was a problem hiding this comment.
Pull Request Overview
Adds OpenTelemetry (OTel) span creation and propagation alongside existing OpenTracing/New Relic spans, extending public APIs to return OTel spans and updating documentation accordingly. Core changes include augmenting span lifecycle methods to handle OTel, updating client/internal/datastore span constructors to start OTel spans, and adding HTTP/gRPC context propagation for OTel. Documentation and module/toolchain versions were updated to reflect these changes.
- Added OTel span support (creation, attributes, error recording, propagation).
- Updated public tracing APIs (e.g., ClientSpan) to return OTel spans; mismatch exists for GRPCTracingSpan between code and docs.
- Upgraded Go toolchain and dependencies; go.mod contains an invalid go directive version format.
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tracing.go | Integrates OTel spans into existing span implementation and propagation; adds helpers and modifies ClientSpan; partial gRPC integration has a lifecycle bug. |
| README.md | Updates exported API docs to include OTel spans; contains mismatch for GRPCTracingSpan signature. |
| newrelic/README.md | Reformats and restructures documentation anchors and variable section. |
| go.mod | Bumps Go version/toolchain and adds OTel dependencies; contains invalid go directive format. |
| .github/workflows/go.yml | Updates CI to use Go 1.24. |
Comments suppressed due to low confidence (1)
tracing.go:1
- otelSpan is started but never ended and is not returned to the caller, resulting in a leaked/open span. Either return the otelSpan (and the OpenTracing span for symmetry) so the caller can defer End(), or end it within this function (likely inappropriate for a server span). Align this with ClientSpan’s pattern.
package tracing
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
svetha-cvl
left a comment
svetha-cvl
left a comment
There was a problem hiding this comment.
@rahulpr22 Can we introduce an otel package (similar to the newrelic package here) and abstract some of the otel specific handling there?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tracing.go (1)
88-109: Consider preserving type information for OTEL attributes.The current implementation converts all values to strings for OpenTelemetry attributes, which loses type information that could be valuable for analysis and filtering in observability tools.
Consider using a type switch to preserve native types:
if span.otelSpan != nil { - span.otelSpan.SetAttributes(attribute.String(key, toString(value))) + switch v := value.(type) { + case bool: + span.otelSpan.SetAttributes(attribute.Bool(key, v)) + case int: + span.otelSpan.SetAttributes(attribute.Int(key, v)) + case int64: + span.otelSpan.SetAttributes(attribute.Int64(key, v)) + case float64: + span.otelSpan.SetAttributes(attribute.Float64(key, v)) + case string: + span.otelSpan.SetAttributes(attribute.String(key, v)) + default: + span.otelSpan.SetAttributes(attribute.String(key, toString(v))) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tracing.go(13 hunks)
🔇 Additional comments (13)
tracing.go (13)
6-7: LGTM! Imports support OTEL integration.The added imports for OpenTelemetry propagation, tracing, attributes, and error codes are appropriate for the integration being implemented.
Also applies to: 12-13, 19-24
43-43: LGTM! OTEL span field added to support dual tracing.The
otelSpanfield enables OpenTelemetry tracing alongside the existing OpenTracing implementation.
53-82: LGTM! Span lifecycle properly managed.The
End()method correctly terminates all tracing resources with appropriate nil checks. The OTEL span is ended after other resources, which is a reasonable cleanup order.
111-126: LGTM! Query attribute properly propagated.The
SetQuery()method correctly sets the query on OpenTracing, NewRelic, and OpenTelemetry spans with appropriate nil guards.
128-157: LGTM! Error recording follows OTEL best practices.The
SetError()method correctly records errors on OpenTelemetry spans using bothRecordError()andSetStatus(), which is the recommended pattern for error handling in OTEL.
159-167: LGTM! Helper function handles string conversion.The
toString()helper provides reasonable string conversion for OTEL attributes. The newline replacement prevents formatting issues in trace viewers.
171-202: LGTM! Internal span creation supports dual tracing.The function correctly creates both OpenTracing and OpenTelemetry spans, with the OTEL span created from the updated context to ensure proper parent-child relationships. Returning
otelCtxmaintains both tracing contexts.
206-246: LGTM! Datastore span creation supports dual tracing.The function correctly creates both OpenTracing and OpenTelemetry datastore spans with proper parent-child relationships maintained through context propagation.
248-283: LGTM! External span construction uses new ClientSpan signature.The function correctly captures and utilizes the OTEL span returned from
ClientSpan, assigning it to thetracingSpanstructure for dual tracing support.
300-311: LGTM! HTTP headers propagate both tracing contexts.The function correctly propagates both OpenTracing and OpenTelemetry contexts into HTTP headers, enabling distributed tracing across services using either or both standards.
338-360: LGTM! TextMapCarrier enables OTEL gRPC propagation.The
metadataTextMapCarriercorrectly implementspropagation.TextMapCarrier, enabling OpenTelemetry context extraction from gRPC metadata. The implementation appropriately handles the metadata structure.
364-385: LGTM! ClientSpan signature extended for OTEL support.The function signature now returns an additional
otelTrace.Span, enabling callers to work with OpenTelemetry spans. Both OpenTracing and OTEL client spans are correctly created with proper span kinds and parent-child relationships.Note: This is a breaking API change documented in the PR objectives.
389-424: LGTM! gRPC server span properly extracts OTEL parent.The function correctly extracts the OpenTelemetry parent context from gRPC metadata (Line 417) before creating the server span, ensuring proper trace continuity across gRPC calls. This addresses the previously identified issue.
There was a problem hiding this comment.
Actionable comments posted: 1
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)
347-377: OTel context never injected into outgoing gRPC metadataWe correctly extract the incoming OTel parent (
ExtractHTTPHeaders) and start a server span, but we never inject the updated context back intomdbefore callingmetadata.NewOutgoingContext. Downstream RPCs therefore miss the OTel propagation. After starting the span (and before returning), call the OTel propagator’sInjectwith a metadata carrier (e.g. export anInjectGRPCMetadatahelper fromotelusing the existingmetadataTextMapCarrier) somdcontains the new trace context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(5 hunks)go.mod(1 hunks)newrelic/README.md(1 hunks)otel/README.md(1 hunks)otel/otel.go(1 hunks)tracing.go(13 hunks)
✅ Files skipped from review due to trivial changes (1)
- otel/README.md
🧰 Additional context used
🧬 Code graph analysis (2)
otel/otel.go (1)
tracing.go (1)
Span(23-34)
tracing.go (1)
otel/otel.go (5)
SetAttributes(42-57)RecordError(60-65)StartSpan(24-29)InjectHTTPHeaders(92-95)ExtractHTTPHeaders(98-101)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
otel/otel.go (2)
41-57: Consider adding support for more numeric types.The current implementation handles common types well. Consider adding explicit cases for
uint,uint64, andfloat32if these types are commonly used in your tracing tags, though the default string conversion provides a safe fallback.
97-107: Consider consistent carrier instantiation.Lines 100 and 106 instantiate
metadataTextMapCarrierdifferently (value vs. pointer). While both work due to value receivers, prefer consistency:Apply this diff for consistency:
func InjectGRPCMetadata(ctx context.Context, md metadata.MD) { propagator := otel.GetTextMapPropagator() - propagator.Inject(ctx, metadataTextMapCarrier{md: &md}) + propagator.Inject(ctx, &metadataTextMapCarrier{md: &md}) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(7 hunks)go.mod(1 hunks)newrelic/README.md(2 hunks)otel/README.md(1 hunks)otel/otel.go(1 hunks)tracing.go(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- otel/README.md
🧰 Additional context used
🧬 Code graph analysis (2)
otel/otel.go (1)
tracing.go (1)
Span(23-34)
tracing.go (1)
otel/otel.go (6)
SetAttributes(42-57)RecordError(60-65)StartSpan(24-29)InjectHTTPHeaders(92-95)ExtractHTTPMetadata(104-107)InjectGRPCMetadata(98-101)
🔇 Additional comments (16)
README.md (1)
17-17: LGTM! Documentation accurately reflects the extended ClientSpan signature.The updated documentation correctly shows that
ClientSpannow returns three values:(context.Context, opentracing.Span, oteltracing.Span), aligning with the implementation changes in tracing.go.Also applies to: 34-34
go.mod (2)
8-10: LGTM! OTEL dependency versions align with best practices.The OpenTelemetry dependencies are using v1.38.0, which according to the learnings includes important spec alignment fixes and improved instrumentation identity semantics. The versions are consistent across otel and otel/trace packages.
Based on learnings.
3-3: Go 1.24.0 is a valid stable release. It was officially released prior to 1.25.1 and is production-ready.tracing.go (9)
48-76: LGTM! Proper nil safety for OTEL span lifecycle.The
End()method correctly guards against nil spans and properly ends the OTEL span alongside OpenTracing and NewRelic spans.
83-103: LGTM! SetTag properly integrates OTEL attributes.The method correctly delegates to
otelutil.SetAttributeswith appropriate nil checks, ensuring tags are propagated to both OpenTracing and OTEL spans.
106-121: LGTM! Query tracking extended to OTEL spans.The
SetQuerymethod correctly propagates query information to OTEL spans using the same helper pattern asSetTag.
123-151: LGTM! Error recording properly integrated with OTEL.The method uses
otelutil.RecordErrorwhich correctly records the error and sets the span status to Error, maintaining consistency across tracing backends.
155-185: LGTM! Internal spans now create OTEL spans correctly.The function properly creates an OTEL span using
otelutil.StartSpan, stores it in thetracingSpanstruct, and returns the updated OTEL context.
189-228: LGTM! Datastore spans now include OTEL tracing.The function follows the same pattern as
NewInternalSpan, properly creating and propagating OTEL spans for datastore operations.
230-265: LGTM! External spans properly receive OTEL span from ClientSpan.The function correctly receives the third return value from
ClientSpan(the OTEL span) and stores it in thetracingSpanstruct.
321-340: LGTM! ClientSpan correctly creates and returns OTEL client span.The function properly:
- Creates an OTEL span with
SpanKindClient- Returns the updated context along with both OpenTracing and OTEL spans
- Maintains the parent-child relationship through context propagation
344-383: LGTM! GRPCTracingSpan properly extracts and propagates OTEL context.The function correctly:
- Extracts incoming OTEL trace context from gRPC metadata (line 371) before creating the server span
- Creates an OTEL server span with
SpanKindServer(line 374)- Injects the OTEL context back into metadata for downstream propagation (line 378)
This addresses the previously flagged issue about missing parent extraction for gRPC server spans.
otel/otel.go (3)
59-65: LGTM! Error recording follows OTEL best practices.The function correctly records the error and sets the span status, ensuring proper error propagation in distributed traces.
67-89: LGTM! metadataTextMapCarrier correctly implements the propagation interface.The carrier properly bridges gRPC metadata with OTEL's text map propagator:
Getreturns the first value or empty stringSetreplaces values usingmetadata.SetKeysprovides all metadata keys for iteration
23-29: No extra nil-span guards required The tracingSpan methods and propagator.Inject calls already no-op when oTelSpan is nil, so all StartSpan callers are safe.newrelic/README.md (1)
15-23: LGTM! Documentation structure properly updated.The anchor updates and link formatting changes maintain consistency with the broader documentation refresh across the repository.
Summary by CodeRabbit
New Features
Documentation
Chores