-
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
[confmap] Split converters and providers into their own modules #9461
Conversation
d8a925b
to
a70e187
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9461 +/- ##
=======================================
Coverage 90.41% 90.41%
=======================================
Files 347 347
Lines 18184 18184
=======================================
Hits 16441 16441
Misses 1412 1412
Partials 331 331 ☔ 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.
I am in favor of this, but I am not sure this is required for #4759. We already have examples of components that are not in an independent module: https://github.com/open-telemetry/opentelemetry-collector-releases/blob/16bde5d0ce3815acd4d73060c782fcf4c698829e/distributions/otelcol-contrib/manifest.yaml#L31-L32. Would there be any advantage on following that pattern?
Good catch! I missed this capability. You're right, it's not strictly required. I do still think it would be easier for users to use a single module per converter/provider since that matches the pattern with our service components.
That's a good question to ask, but I still don't think so. The only thing I could think of is if we wanted to allow for grouping converters and providers that fall within a certain category, but this would still require special handling and I don't see any obvious or potentially valuable groupings. In general I think we should still move in the direction of a single module per component, it follows our existing patterns and helps keep the resulting dependency tree leaner. |
Okay, makes sense. I think it also helps with stabilizing confmap, since we don't have to think about the behavior of individual providers to stabilize confmap, just about the interfaces. I will put a reminder to merge this by end of week after this week's release has happened |
@evan-bradley can you fix the merge conflicts? |
a70e187
to
3522712
Compare
Resolves #9460
Required for #4759
I made this a single PR to reduce the review load, but I can break the changes into multiple PRs if we would like.