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

Deprecate component.Host.GetExporters

Open djaglowski opened this issue 2 years ago • 6 comments

Is your feature request related to a problem? Please describe.

component.Host.GetExporters has a comment that states This is an experimental function that may change or even be removed completely. With the addition of connectors we should further question whether this function should exist.

Describe the solution you'd like I'm proposing that we deprecate the function once the service.connectors featuregate is marked stable. (Currently proposed to become beta in #7369).

This function is not only unnecessary, but potentially problematic for at least the following reasons:

  1. It provides any component with the ability to start and stop any exporter. While it's not expected that this capability would be misused, there doesn't appear to be any valid reason why this should be possible, nor is it clear that this would ever be safe.
  2. It allows any component to emit data directly to any exporter. This is not the intention of the collector's pipeline model. Rather, it was a necessary workaround to solve various use cases that can now be solved with connectors.
  3. Components that currently rely on this functionality may violate subtle concerns, such as data mutability, which would otherwise not be a concern of any component. The same concerns should be handled by the service itself, which is necessarily the case with an implementation that requires data to flow through pipelines.

The following components currently use this function:

djaglowski avatar Mar 14 '23 19:03 djaglowski

I support this, if there are no objections, should we create issues on contrib for the individual components that need some work?

mx-psi avatar Mar 15 '23 16:03 mx-psi

@mx-psi, I've created tracking issues in contrib and updated my proposal to include them in a checklist.

In my opinion, we are better off deprecating the function sooner rather than later. This will prevent new usages and give any external users as much notification as possible. I will also create a PR for this, but of course this can be merged if/when more approvers support it.

djaglowski avatar Mar 15 '23 20:03 djaglowski

I agree on the depreciation. We need to make sure all the contrib dependencies are resolved, should be ok to do that after the depreciation, but before the removal.

dmitryax avatar Mar 17 '23 19:03 dmitryax

From Datadog's side, the datadog processor was removed on v0.94.0. I am also proposing we remove the spanmetrics processor later this month.

mx-psi avatar Feb 08 '24 12:02 mx-psi

We discussed a way to speed this up that we can tackle as a last resort: we could remove the component.Host.GetExporters method from the component.Host implementation, but still have the component.Host struct passed to components by service implement this function. That way we delay the actual removal of GetExporters and the work on the remaining components until service 1.0

mx-psi avatar Apr 03 '24 15:04 mx-psi

Moving to service 1.0 after #9987. The remaining part is to remove in contrib components and then remove from the service's component.Host.

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

@TylerHelmuth Should we remove this from the Collector v1 board based on #10643?

mx-psi avatar Jul 31 '24 08:07 mx-psi

@mx-psi based on the decision made in https://github.com/open-telemetry/opentelemetry-collector/issues/10181 our component.Host implementation can implement optional interfaces that other components need.

As an aside, I think service should probably expose, somehow, which interfaces it's component.Host implements.

I would say that long term the GetExporters interface probably isn't something we want so leaving the issue open is good, but I agree that we no longer need it for service 1.0.

TylerHelmuth avatar Aug 07 '24 01:08 TylerHelmuth