Return noop TracerProvider when no trace processors are defined#15186
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #15186 +/- ##
=======================================
Coverage 91.23% 91.23%
=======================================
Files 701 701
Lines 45838 45838
=======================================
Hits 41822 41822
Misses 2816 2816
Partials 1200 1200 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@hxiao87 thanks for the PR. Tests are failing - please fix those and mark this ready for review when you have everything passing. |
Hi Andrew, thanks for reviewing! I have run the tests locally and confirmed they'd pass. Looks like an approval is needed to kick off the CI, so I'll just open the PR again. While addressing the CI failures from the MissingResource test, I figured |
Yeah that won't happen in practice, a logger will always be provided in a real collector. We could make the test more realistic by passing in a logger (probably a no-op logger), but your change is fine for now. |
Merging this PR will not alter performance
|
|
The spell-check failure doesn't appear to be related to this branch. Should be good to merge |
| return nil, err | ||
| } | ||
|
|
||
| if cfg.Traces.Level == configtelemetry.LevelNone || len(cfg.Traces.Processors) == 0 { |
There was a problem hiding this comment.
Is there a reason to move this if-clause after createFixedResourceConfig()? cfg.Traces.Level and cfg.Traces.Processors shouldn't be changed by createFixedResourceConfig() - are they?
There was a problem hiding this comment.
No, createFixedResourceConfig doesn't modify those fields. With the existing order, this test panics as it passes an empty TracerSettings{} (w/ nil Logger) with no processors - thus moving the resource validation to the front.
Co-authored-by: Florian Lehner <florianl@users.noreply.github.com>
8a98677
…-telemetry#15186) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Returns noop TracerProvider when no processors are defined such that this scenarios is treats the same as when `LevelNone` is set. <!-- Issue number if applicable --> #### Link to tracking issue Fixes open-telemetry#15135 <!--Describe what testing was performed and which tests were added.--> #### Testing - Updated existing tests in `trace_test.go` to make sure there's no behavior change - Added `TestCreateTracerProvider_NoProcessors` to verify that the default config (level=Basic, no processors) returns a noop provider <!--Describe the documentation added.--> #### Documentation Didn't find any relevant doc that needs update. <!--Please delete paragraphs that you did not use before submitting.--> --------- Co-authored-by: Florian Lehner <florianl@users.noreply.github.com> Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Description
Returns noop TracerProvider when no processors are defined such that this scenarios is treats the same as when
LevelNoneis set.Link to tracking issue
Fixes #15135
Testing
trace_test.goto make sure there's no behavior changeTestCreateTracerProvider_NoProcessorsto verify that the default config (level=Basic,no processors) returns a noop provider
Documentation
Didn't find any relevant doc that needs update.