opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

Does DataType belong to component? Can we make it a type itself?

Open bogdandrutu opened this issue 1 year ago • 2 comments
trafficstars

bogdandrutu avatar Jan 30 '24 17:01 bogdandrutu

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)

mx-psi avatar Apr 09 '24 14:04 mx-psi

I'll try to come up with a solution for this one

ankitpatel96 avatar Apr 12 '24 17:04 ankitpatel96

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.ID we'd have piplelineID component.PipelineID. Using component.Signal is also a clearer term than component.DataType since 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.

TylerHelmuth avatar Aug 28 '24 15:08 TylerHelmuth

I would propose we give one week for voting and that we don't do the refactor in case of a tie

mx-psi avatar Aug 29 '24 07:08 mx-psi

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.

evan-bradley avatar Aug 29 '24 15:08 evan-bradley

The proposal in #10947 doesn't address the initial issue of moving DataType out of component correct?

codeboten avatar Sep 05 '24 18:09 codeboten

@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.

TylerHelmuth avatar Sep 05 '24 18:09 TylerHelmuth

Based on the voting I will move forward with the refactor.

TylerHelmuth avatar Sep 09 '24 14:09 TylerHelmuth

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?

TylerHelmuth avatar Sep 09 '24 15:09 TylerHelmuth