opentelemetry-collector
opentelemetry-collector copied to clipboard
Does DataType belong to component? Can we make it a type itself?
On April 3rd we discussed creating a new configsignal module that would contain this type as an enum. We should make sure that we can use DataType inside a component.ID or change the service config (we use a component.ID on this configuration to support naming pipelines something like traces/2)
I'll try to come up with a solution for this one
With a plausible implementation example via https://github.com/open-telemetry/opentelemetry-collector/pull/10947, I'd like to take a vote on whether @open-telemetry/collector-contrib-approvers @open-telemetry/collector-approvers view this change as valueable.
Benefits of the change:
- Cleaner code to maintain. Instead of doing
pipelineID component.IDwe'd havepiplelineID component.PipelineID. Usingcomponent.Signalis also a clearer term thancomponent.DataTypesince Signal is an OTel term and DataType is not. - Ability to validate pipeline names separately from component names.
Downsides of this change:
- It is almost purely an API change - end users (users running a Collector) do not care about this change.
- Breaking changes late in the life of
component. List of breaking changes can be found in https://github.com/open-telemetry/opentelemetry-collector/pull/10947. I think it would be possible to follow a 3-release cycle deprecation strategy, but the interface changes make it harder.
If we do not do this refactor before 1.0, I do not believe it could be done as an enhancement in 1.X - it would have to be added via a 2.0 breaking change.
Vote 👍 if you think we should do this refactor. Vote 👎 if you think we should not do this refactor.
I would propose we give one week for voting and that we don't do the refactor in case of a tie
I'm voting yes on the refactor with the expectation that this is a straightforward change. We'll have to live with this for a while, and I think renames like this can help improve the contributor experience, particularly for new contributors.
The proposal in #10947 doesn't address the initial issue of moving DataType out of component correct?
@codeboten no, but it could. For simplicity in that PR I left the component.DataType -> component.Signal in component, but component.Signal and component.PipelineID can be moved somewhere else. componentstatus depends on PipelineID so it cannot be moved to service. component.Signal is used in connector stuff, so it also can't live in service. The final place must be a pretty "low level" module since a lot of other modules will depend on it.
Based on the voting I will move forward with the refactor.
As for addressing the initial ask of this issue, should we make a new module, pipeline at the root and put Signal and PipelineID in there?