Skip to content

perf: add SetTraceIdWithValue, cache OTEL span lookup#25

Merged
ankurs merged 2 commits intomainfrom
perf/set-trace-id-with-value
Apr 3, 2026
Merged

perf: add SetTraceIdWithValue, cache OTEL span lookup#25
ankurs merged 2 commits intomainfrom
perf/set-trace-id-with-value

Conversation

@ankurs
Copy link
Copy Markdown
Member

@ankurs ankurs commented Apr 3, 2026

Summary

  • Add SetTraceIdWithValue returning (context.Context, string) to avoid a separate GetTraceId call after SetTraceId — eliminates redundant context lookups on the hot path
  • SetTraceId delegates to SetTraceIdWithValue — existing callers are unaffected (public API preserved)
  • Cache oteltrace.SpanFromContext(ctx) result: called once instead of up to 3 times per invocation

Ref: ROADMAP items 1.2, 1.3

Test plan

  • Existing TestSetTraceId_* tests pass unchanged (public API preserved)
  • New TestSetTraceIdWithValue_* tests verify returned trace ID matches GetTraceId
  • OTEL span attribute test for SetTraceIdWithValue
  • go test -race ./notifier/... passes

Add SetTraceIdWithValue that returns (context.Context, string), avoiding
a separate GetTraceId call after SetTraceId. SetTraceId delegates to it
with //go:inline for zero overhead on existing callers.

Cache oteltrace.SpanFromContext result in a local variable — called once
instead of up to 3 times per invocation.
Copilot AI review requested due to automatic review settings April 3, 2026 16:49
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 3, 2026

📝 Walkthrough

Walkthrough

The notifier package's trace ID handling was refactored by introducing SetTraceIdWithValue that resolves trace IDs (from options, gRPC metadata, OTEL span, or UUID) and returns both the updated context and the resolved trace ID string. The original SetTraceId function was reintroduced as a wrapper for backward compatibility. Three test cases verify the new function's behavior with various trace ID scenarios and OTEL span attribute setting.

Changes

Cohort / File(s) Summary
Trace ID Resolution Refactor
notifier/notifier.go
Introduced SetTraceIdWithValue to resolve and cache trace IDs while returning the string value; refactored SetTraceId as a wrapper that discards the returned string, preserving the original API.
Test Coverage
notifier/notifier_test.go
Added three test cases for SetTraceIdWithValue covering non-empty trace ID generation, preservation of existing trace IDs, and verification of OTEL span attribute coldbrew.trace_id setting.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

Poem

🐰 A trace ID returns with grace,
OpenTelemetry's embrace,
Split the logic, keep it light,
Wrapped with backward-compat might,
OTEL attributes shining bright! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main changes: introducing SetTraceIdWithValue function and caching OTEL span lookup for performance improvements.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf/set-trace-id-with-value

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

ankurs added a commit to go-coldbrew/interceptors that referenced this pull request Apr 3, 2026
Replace SetTraceId+GetTraceId+AddToLogContext pattern with a single
SetTraceIdWithValue call in both ServerErrorInterceptor and
ServerErrorStreamInterceptor. SetTraceIdWithValue returns the trace ID
directly and already adds it to the log context, eliminating two
redundant context lookups per request.

Depends on: go-coldbrew/errors#25
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes trace ID resolution on the notifier hot path by adding a new API that returns the resolved trace ID alongside the updated context, and by reducing repeated OpenTelemetry span lookups.

Changes:

  • Add SetTraceIdWithValue(ctx) (context.Context, string) to return the resolved trace ID without requiring a follow-up GetTraceId call.
  • Refactor SetTraceId to delegate to SetTraceIdWithValue.
  • Add unit tests covering the new API and OTEL attribute behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
notifier/notifier.go Introduces SetTraceIdWithValue, caches span lookup, and refactors SetTraceId to delegate.
notifier/notifier_test.go Adds tests for SetTraceIdWithValue (returned value, existing trace ID, OTEL attribute).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread notifier/notifier.go Outdated
Comment thread notifier/notifier.go
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@notifier/notifier.go`:
- Line 580: Remove the invalid compiler directive "//go:inline"—locate the
literal comment "//go:inline" in notifier.go and delete it (do not replace with
any nonstandard directive); if you intended to control inlining behavior use
supported directives like "//go:noinline" or "//go:noescape" instead, but
otherwise just remove the "//go:inline" line so the code relies on the
compiler's automatic inlining.
🪄 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: 6163925c-9912-45be-8679-5d5acdb3eaf5

📥 Commits

Reviewing files that changed from the base of the PR and between d378bec and f3d083f.

📒 Files selected for processing (2)
  • notifier/notifier.go
  • notifier/notifier_test.go

Comment thread notifier/notifier.go Outdated
…e doc

Address review feedback:
- Remove //go:inline (not a valid Go compiler directive)
- Add doc note that callers must use the returned context
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 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.

@ankurs ankurs merged commit e40523f into main Apr 3, 2026
11 checks passed
@ankurs ankurs deleted the perf/set-trace-id-with-value branch April 3, 2026 17:11
ankurs added a commit to go-coldbrew/interceptors that referenced this pull request Apr 3, 2026
…31)

* perf: use SetTraceIdWithValue to eliminate redundant context lookups

Replace SetTraceId+GetTraceId+AddToLogContext pattern with a single
SetTraceIdWithValue call in both ServerErrorInterceptor and
ServerErrorStreamInterceptor. SetTraceIdWithValue returns the trace ID
directly and already adds it to the log context, eliminating two
redundant context lookups per request.

Depends on: go-coldbrew/errors#25

* chore: bump go-coldbrew/errors to v0.2.10

Required for SetTraceIdWithValue API added in errors#25.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants