[exporter/debug] add ability to set output_paths #14164
Conversation
andrzej-stencel
changed the title
andrzej-stencel
left a comment
andrzej-stencel
left a comment
There was a problem hiding this comment.
Thanks for the contribution @ishaish103! I added a couple suggestions for the docs and the tests.
| return cfg | ||
| }(), | ||
| }, | ||
| { |
There was a problem hiding this comment.
Please add a test case for when user sets output_paths but does not set use_internal_logger to false.
What should happen then? Shouldn't such config be invalid?
There was a problem hiding this comment.
basically the same thing:
throw an error if out_paths is null and set_intenral_logger is false
Co-authored-by: Andrzej Stencel <andrzej.stencel@elastic.co>
|
@andrzej-stencel Hey :) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14164 +/- ##
=======================================
Coverage 91.85% 91.86%
=======================================
Files 677 677
Lines 42699 42706 +7
=======================================
+ Hits 39222 39232 +10
+ Misses 2423 2421 -2
+ Partials 1054 1053 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…tor into 10472-debug-exporter-output-paths-ishai
…ishaish103/opentelemetry-collector into 10472-debug-exporter-output-paths-ishai
CodSpeed Performance ReportMerging this PR will not alter performanceComparing
|
- Add validation to error when output_paths is specified but use_internal_logger is true - Remove redundant fallback in createCustomLogger since validation ensures output_paths is set - Update default config to have empty OutputPaths when UseInternalLogger is true - Update tests to reflect the new validation rules
- Updated QueueConfig to use configoptional.Optional[QueueBatchConfig] - Changed from queueCfg.Enabled = false to configoptional.None() to disable queue - Updated all test cases to use the new Optional API - Preserved OutputPaths feature and validation logic
- Remove trailing whitespace in factory_test.go - Remove extra blank line in factory_test.go - Regenerate all generated code files
There was a problem hiding this comment.
so i ended up removing this validation and added comment on the config param that in such case the output path will be ignored
I'm worried that this will lead to multiple users of the Debug exporter setting output_paths in their configuration and pulling their hair wondering why it doesn't work. We should design software so that it's hard to make such mistakes.
How about not setting the OutputPaths field in createDefaultConfig() and only setting it in createCustomLogger() to ["stdout"], if it is unset (nil)? Would that work?
… handling - Updated error messages for clarity regarding output_paths when use_internal_logger is false. - Adjusted validation logic to handle nil output_paths correctly. - Modified default behavior to set output_paths to nil when use_internal_logger is true. - Updated README to reflect changes in output_paths behavior.
…ishaish103/opentelemetry-collector into 10472-debug-exporter-output-paths-ishai
make sense, that worked! |
|
@andrzej-stencel can take another look please? |
- Split expectedErr into expectedUnmarshalErr/expectedValidateErr in config_test - Update README: use Wikipedia links for stdout/stderr instead of os.Stdout/os.Stderr - Simplify factory_test: remove time.Sleep, Windows workarounds, redundant code - Remove unintended trailing blank line from metadata.yaml Assisted-by: Claude Opus 4.5
- Add nolint:gosec for os.ReadFile in test (matches codebase pattern) - Skip file output test on Windows due to t.TempDir() cleanup race Assisted-by: Claude Opus 4.5
|
Hey @andrzej-stencel |
Reverts the QueueConfig default back to configoptional.Default() as it was before - this change was unrelated to the output_paths feature. Assisted-by: Claude Opus 4.5
andrzej-stencel
removed
the
release:risky-change
2 participants
Link to tracking issue
Fixes #10472