Skip to content
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

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

evan-bradley
Copy link
Contributor

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.

Copy link

codecov bot commented Feb 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (de6287d) 90.41% compared to head (3522712) 90.41%.

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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mx-psi mx-psi left a 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?

@evan-bradley
Copy link
Contributor Author

I am not sure this is required for #4759. We already have examples of components that are not in an independent module: [...]

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.

Would there be any advantage on following that pattern?

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.

@mx-psi
Copy link
Member

mx-psi commented Feb 6, 2024

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

@mx-psi
Copy link
Member

mx-psi commented Feb 8, 2024

@evan-bradley can you fix the merge conflicts?

@mx-psi mx-psi merged commit 4407529 into open-telemetry:main Feb 12, 2024
47 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split core into more granular modules: confmap
3 participants