Skip to content

Conversation

@rogercoll
Copy link
Contributor

Files from elastic/opentelemetry-collector-components#310 so it can be reused in multiple projects.

@gregkalapos
Copy link
Contributor

gregkalapos commented Jan 21, 2025

I'd like to raise the discussion of module vs package which we also have in #141 - #141 (comment)

It seems this lib as a single module gets very big and contains packages with very different concerns. E.g. now it has a new dependency on the elasticsearch client - because that's needed for fetching the config. But all the other packages from this lib don't need that.

Would it make sense to separate things into multiple modules here? And with that, the config fetcher would be a module as well.

@rogercoll
Copy link
Contributor Author

I'd like to raise the discussion of module vs package which we also have in #141 - #141 (comment)

It seems this lib as a single module gets very big and contains packages with very different concerns. E.g. now it has a new dependency on the elasticsearch client - because that's needed for fetching the config. But all the other packages from this lib don't need that.

Would it make sense to separate things into multiple modules here? And with that, the config fetcher would be a module as well.

I like the idea, I prefer the "extra" complexity of maintaining multiple modules than having to include dependencies that might not be used. But I think we will need to create another issue/pr for this as we will need to adapt the whole repository structure to something similar to https://github.com/elastic/opentelemetry-collector-components/ and release each submodule accordingly.

@gregkalapos
Copy link
Contributor

Totally agree on addressing that in a follow up PR - no reason to block this PR for that at this point.

@rogercoll
Copy link
Contributor Author

Tracking issue: #144

@rogercoll rogercoll merged commit 03dfd1f into elastic:main Jan 21, 2025
3 checks passed
@rogercoll rogercoll deleted the add_agentcfg_fetcher branch January 21, 2025 11:15
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.

2 participants