test: add unit tests for telemetry#3033
Closed
omer-topal wants to merge 1 commit into
Closed
Conversation
📝 WalkthroughWalkthroughThis PR adds unit test files for the telemetry package, covering exporter factories for log, meter, and tracer exporters (OTLP, zipkin, jaeger, signoz), as well as core telemetry helpers like resource naming, handler factory, log/tracer providers, and metric instruments. ChangesTelemetry exporter and core test coverage
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/telemetry/meterexporters/factory_test.go (2)
57-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify the exact error message for the unsupported protocol case.
TestExporterFactoryasserts the precise error string for error cases, butTestNewOTLPonly checks that an error occurred for the"unsupported protocol"case. Asserting the message ("unsupported protocol: udp") would make the test consistent and guard against accidental error-format changes.♻️ Proposed fix
{name: "unsupported protocol", protocol: "udp", wantErr: true},func TestNewOTLP(t *testing.T) { tests := []struct { name string insecure bool urlpath string headers map[string]string protocol string wantErr bool + errMessage string }{ {name: "http insecure with options", insecure: true, urlpath: "/v1/metrics", headers: map[string]string{"key": "value"}, protocol: "http"}, {name: "http secure", insecure: false, protocol: "http"}, {name: "grpc insecure", insecure: true, headers: map[string]string{"key": "value"}, protocol: "grpc"}, {name: "grpc secure", insecure: false, headers: map[string]string{"key": "value"}, protocol: "grpc"}, - {name: "unsupported protocol", protocol: "udp", wantErr: true}, + {name: "unsupported protocol", protocol: "udp", wantErr: true, errMessage: "unsupported protocol: udp"}, }exp, err := NewOTLP("localhost:4317", tt.insecure, tt.urlpath, tt.headers, tt.protocol) if tt.wantErr { if err == nil { t.Fatalf("expected error, got nil") } + if tt.errMessage != "" && err.Error() != tt.errMessage { + t.Fatalf("expected error %q, got %q", tt.errMessage, err.Error()) + } return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/telemetry/meterexporters/factory_test.go` at line 57, The unsupported protocol case in TestNewOTLP only checks for an error, but it should assert the exact message to stay consistent with TestExporterFactory. Update the "unsupported protocol" table-driven case in factory_test.go to verify the error string from NewOTLP matches the precise unsupported protocol message for "udp", using the existing TestNewOTLP and unsupported protocol symbols to locate the assertion.
60-75: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winShut down exporters in success cases to avoid resource leaks.
otlpmetrichttp.Newandotlpmetricgrpc.Newcan start background goroutines/connections. The success cases create exporters but never callShutdown, which may leak resources or cause flakiness when the suite runs in CI.♻️ Proposed fix
if exp == nil { t.Fatalf("expected exporter, got nil") } + if err := exp.Shutdown(context.Background()); err != nil { + t.Fatalf("failed to shutdown exporter: %v", err) + } })You'll also need to add
"context"to the import block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/telemetry/meterexporters/factory_test.go` around lines 60 - 75, The successful NewOTLP test cases create exporters but never clean them up, which can leave background goroutines or connections running. Update the table-driven test in factory_test.go so that when NewOTLP returns a non-nil exporter in the non-error path, it calls Shutdown with a context before the subtest exits. Add the context import needed for that cleanup, and keep the existing error assertions intact in the t.Run cases around NewOTLP.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/telemetry/meterexporters/factory_test.go`:
- Line 57: The unsupported protocol case in TestNewOTLP only checks for an
error, but it should assert the exact message to stay consistent with
TestExporterFactory. Update the "unsupported protocol" table-driven case in
factory_test.go to verify the error string from NewOTLP matches the precise
unsupported protocol message for "udp", using the existing TestNewOTLP and
unsupported protocol symbols to locate the assertion.
- Around line 60-75: The successful NewOTLP test cases create exporters but
never clean them up, which can leave background goroutines or connections
running. Update the table-driven test in factory_test.go so that when NewOTLP
returns a non-nil exporter in the non-error path, it calls Shutdown with a
context before the subtest exits. Add the context import needed for that
cleanup, and keep the existing error assertions intact in the t.Run cases around
NewOTLP.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8706542-67e3-4b6e-b182-e72c06e96857
📒 Files selected for processing (4)
pkg/telemetry/logexporters/factory_test.gopkg/telemetry/meterexporters/factory_test.gopkg/telemetry/telemetry_test.gopkg/telemetry/tracerexporters/factory_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (74.24%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #3033 +/- ##
==========================================
- Coverage 74.67% 74.24% -0.42%
==========================================
Files 83 99 +16
Lines 9212 9541 +329
==========================================
+ Hits 6878 7083 +205
- Misses 1798 1910 +112
- Partials 536 548 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit