-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Description
Component(s)
No response
Describe the issue you're reporting
While working with configgrpc and confighttp recently, I've found the host parameter in the ToClient and ToServer methods on the config corresponding structs in each module to be more cumbersome than helpful, and I think we should consider changing it before stabilizing these modules.
I think the following points are in favor of changing the host component.Host parameter to middlewares map[component.ID]component.Component (or similar):
- For Collector-adjacent components that want to keep a similar configuration structure for gRPC/HTTP-related tasks and use configgrpc/confighttp for this, a host isn't necessarily already present and requires extra setup to use the methods. We recently encountered this situation in this Supervisor PR. This would also relieve tests of having to create a mock host; they could just create an extensions map.
- The
component.Hostinterface is stable and only has aGetExtensionsmethod, so passing in the host doesn't do much in the way of encapsulating behavior: the ToClient/ToServer methods will only callGetExtensions.
The main point against this would be that these modules could take advantage of other host capabilities in the future through optional interfaces. At present I don't see this happening, but would be open to the opinions of others if we think it may. We could probably utilize the existing variadic options parameter in the future if something comes up, even if it would be a slightly unusual way to use it.
This would also be a fairly large breaking change, albeit with an easy work around: you just pass host.GetExtensions() instead of host to the config methods.
Tip
React with 👍 to help prioritize this issue. Please use comments to provide useful context, avoiding +1 or me too, to help us triage it. Learn more here.
Metadata
Metadata
Assignees
Labels
Type
Projects
Status