test: verify coldbrew.trace_id OTEL span attribute in SetTraceId#24
test: verify coldbrew.trace_id OTEL span attribute in SetTraceId#24
Conversation
|
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)
📝 WalkthroughWalkthroughUpdated OpenTelemetry module versions and added explicit SDK and indirect logging/auto-instrumentation deps; added an in-memory OTEL test tracer helper and expanded tests to verify Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
Adds unit tests to ensure SetTraceId() links the resolved ColdBrew trace ID to the active OpenTelemetry span via the coldbrew.trace_id attribute, including early-return and no-span cases.
Changes:
- Add OTEL test tracer setup and span-attribute assertions for
SetTraceId()behavior. - Add a no-span regression test to ensure
SetTraceId()does not panic without an active span. - Update Go module dependencies to include the OTEL SDK test utilities used by the new tests.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| notifier/notifier_test.go | Adds OTEL-backed unit tests covering span attribute behavior and no-span safety. |
| go.mod | Bumps OTEL to v1.43.0 and adds OTEL SDK dependency for tests. |
| go.sum | Updates sums for the OTEL version bump and new transitive 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: 2
🧹 Nitpick comments (1)
go.mod (1)
12-14: Movego.opentelemetry.io/otel/sdkto indirect dependencies.The SDK package is only imported in
notifier/notifier_test.go(test-only code). In Go modules, test-only dependencies should be marked as indirect. Rungo mod tidyto automatically handle this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 12 - 14, The go.mod currently lists go.opentelemetry.io/otel/sdk as a direct dependency but it is only used in tests (notifier/notifier_test.go); run `go mod tidy` to move go.opentelemetry.io/otel/sdk to an indirect dependency (or manually add the // indirect marker to that require line) so test-only packages are not treated as direct module requirements, ensuring the module list reflects actual non-test imports.
🤖 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_test.go`:
- Around line 118-141: TestSetTraceId_EarlyReturn_SetsOTELAttribute has a
duplicate span.End() invocation: the span is ended twice via defer span.End()
and a later explicit span.End(); remove the redundant call by deleting the
explicit span.End() (or the deferred one) so the span is ended exactly once.
Locate the TestSetTraceId_EarlyReturn_SetsOTELAttribute test and remove the
extra span.End() to ensure the tracer span is closed only once.
- Around line 95-116: In TestSetTraceId_SetsOTELAttribute remove the redundant
deferred end of the OTEL span: delete the `defer span.End()` line and keep the
explicit `span.End()` call (in the TestSetTraceId_SetsOTELAttribute test) so the
span is ended just once before calling `exporter.GetSpans()`; this eliminates
the double `span.End()` while retaining deterministic span termination for the
test.
---
Nitpick comments:
In `@go.mod`:
- Around line 12-14: The go.mod currently lists go.opentelemetry.io/otel/sdk as
a direct dependency but it is only used in tests (notifier/notifier_test.go);
run `go mod tidy` to move go.opentelemetry.io/otel/sdk to an indirect dependency
(or manually add the // indirect marker to that require line) so test-only
packages are not treated as direct module requirements, ensuring the module list
reflects actual non-test imports.
🪄 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: ca86c0ae-1b32-477b-b6b5-7d2d7b2de6af
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.modnotifier/notifier_test.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 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.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
TestSetTraceId_SetsOTELAttribute: Verifies attribute is set when SetTraceId resolves a trace IDTestSetTraceId_EarlyReturn_SetsOTELAttribute: Verifies attribute is set even on early return (pre-existing trace ID)TestSetTraceId_NoSpan_NoPanic: Verifies no panic without OTEL spanUses OTEL SDK test tracer (
tracetest.InMemoryExporter) for span capture.Closes #23
Test plan
go test -race ./...passesSummary by CodeRabbit
Chores
Tests