feat: migrate notifier from OpenTracing to OTEL trace context#18
feat: migrate notifier from OpenTracing to OTEL trace context#18
Conversation
Replace opentracing.SpanFromContext + BaggageItem("trace") with
OTEL trace.SpanFromContext + SpanContext().TraceID().String().
Remove opentracing-go dependency. Add go.opentelemetry.io/otel/trace.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced OpenTracing with OpenTelemetry in dependency declarations, updated notifier to extract OTEL span trace IDs (adding Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Pull request overview
Migrates the notifier package’s trace correlation from OpenTracing baggage-based extraction to OpenTelemetry (OTEL) span context, removing the OpenTracing dependency as part of the broader tracing migration.
Changes:
- Replace
opentracing.SpanFromContext(...).BaggageItem("trace")withtrace.SpanFromContext(...).SpanContext().TraceID().String(). - Remove
github.com/opentracing/opentracing-godependency; addgo.opentelemetry.io/otel/trace. - Update generated notifier README links to reflect new line numbers.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| notifier/notifier.go | Switch trace ID extraction to OTEL span context and update imports accordingly |
| notifier/README.md | Regenerate/update documentation links after code movement |
| go.mod | Drop OpenTracing dependency and add OTEL trace dependency |
| go.sum | Remove OpenTracing checksums and add OTEL checksums |
💡 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
notifier/notifier.go (1)
361-367:⚠️ Potential issue | 🟠 MajorPreserve
GetTraceIdprecedence indoNotify.Lines 362-367 now prefer the span trace over the trace already stored by
SetTraceId. That makes this path disagree withSetTraceId's own ordering, so a request carryingx-trace-idcan log one trace value (trace) while Airbrake/Rollbar emit another (traceId) as soon as a span is present.Suggested fix
for _, d := range list { if c, ok := d.(context.Context); ok { - if span := oteltrace.SpanFromContext(c); span.SpanContext().IsValid() { - traceID = span.SpanContext().TraceID().String() - } - if strings.TrimSpace(traceID) == "" { - traceID = GetTraceId(c) - } + traceID = GetTraceId(c) + if strings.TrimSpace(traceID) == "" { + if span := oteltrace.SpanFromContext(c); span.SpanContext().IsValid() { + traceID = span.SpanContext().TraceID().String() + } + } ctx = c break } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notifier/notifier.go` around lines 361 - 367, In doNotify, the current logic lets oteltrace.SpanFromContext take precedence over a trace previously set by SetTraceId; change the order so GetTraceId(c) is consulted first and used if non-empty, and only if it returns an empty string fall back to extracting the span trace via oteltrace.SpanFromContext(c). Update the traceID assignment in the block that checks if d is a context.Context to call GetTraceId(c) first, then only call span.SpanContext().TraceID().String() when GetTraceId returned an empty/blank value, ensuring trace semantics match SetTraceId/GetTraceId ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@notifier/notifier.go`:
- Around line 361-367: In doNotify, the current logic lets
oteltrace.SpanFromContext take precedence over a trace previously set by
SetTraceId; change the order so GetTraceId(c) is consulted first and used if
non-empty, and only if it returns an empty string fall back to extracting the
span trace via oteltrace.SpanFromContext(c). Update the traceID assignment in
the block that checks if d is a context.Context to call GetTraceId(c) first,
then only call span.SpanContext().TraceID().String() when GetTraceId returned an
empty/blank value, ensuring trace semantics match SetTraceId/GetTraceId
ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd2d273e-9d57-4050-9ca4-83e61d9ed529
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modnotifier/README.mdnotifier/notifier.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Application-level correlation ID (from SetTraceId/GetTraceId) now takes precedence over the OTEL distributed trace ID in error notifications. Both are captured separately: - traceId: application correlation ID (falls back to OTEL trace ID) - otelTraceId: OTEL distributed trace ID for linking to tracing backends
Summary
Replace OpenTracing span context extraction with native OTEL in the notifier package.
opentracing.SpanFromContext()+BaggageItem("trace")→trace.SpanFromContext()+SpanContext().TraceID().String()opentracing-godependencygo.opentelemetry.io/otel/traceDepends on
Test plan
go test -race ./...— passesmake lint— 0 issuesSummary by CodeRabbit
Documentation
Chores
Bug Fixes / Improvements