-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[chore][exporter/debug] refactor code to make independent from Logging exporter #9922
[chore][exporter/debug] refactor code to make independent from Logging exporter #9922
Conversation
…g exporter This is in preparation to implement open-telemetry#7806 (draft PR: open-telemetry#9298).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong opinion here, but an alternative solution could be to have a custom unmarshal function/initialization logic for the logging exporter that overrides verbosity from normal
to set to basic
. That way we would avoid duplicating code. Any thoughts on which one is better?
I feel we might have more changes coming to the Debug exporter that I don't think we want to introduce to the Logging exporter - see e.g. #9372, #9921. I would leave the Logging exporter's code unchanged, waiting to be removed in September 2024, and only work with the Debug exporter's code. In other words, I don't think we will have changes we want to introduce to both Logging and Debug exporter - I'd rather leave the Logging exporter's behavior unchanged until it's removed. Because of this, the code duplication resulting from the split is not a problem. It's actually expected, as the Debug exporter is expected to evolve without the Logging exporter being touched. |
Okay, I am fine with this approach, let me ping @open-telemetry/collector-approvers just in case someone has a different opinion and we can wait a couple days to give people time to voice their opinion |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9922 +/- ##
==========================================
- Coverage 91.79% 91.72% -0.08%
==========================================
Files 358 359 +1
Lines 16576 16634 +58
==========================================
+ Hits 15216 15257 +41
- Misses 1037 1053 +16
- Partials 323 324 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will merge tomorrow unless somebody else objects
Improving coverage after #9922.
Improving coverage after open-telemetry#9922.
This PR is the first part of this draft PR: #9298.
This refactoring is needed to implement [exporter/debug] change behavior of "normal" verbosity to be different from "basic" #7806. I want to change the behavior of the Debug exporter, but leave the behavior of the Logging exporter unchanged.
Link to tracking Issue: