opentelemetry-collector
opentelemetry-collector copied to clipboard
Deprecate component.Host.GetExporters
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:
- 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.
- 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.
- 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:
- [ ]
routingprocessorroutes data directly to exporters. This component would be much more naturally implemented as a connector. - [x]
spanmetricsprocessorgenerates data and emits it directly to an exporter. Already ported to a connector. - [x]
datadogprocessorgenerates data and emits it directly to an exporter. Could easily be ported to a connector. - [x]
servicegraphprocessorgenerates data and emits it directly to an exporter. Already ported to a connector. - [ ]
k8sclusterreceiversends metadata directly to exporters. This is the only usage that is not replaced by connectors. However, the implementation is quite unusual and seems like it should be implemented as an extension anyways. - [x] No uses in core, other than self-references and examples.
- [x] A couple other receivers use the function when mocking a
component.Host. Wouldn't be necessary at all. - [x]
servicegraphconnectorstill useshost.GetExportersdue being copied from the processor.
I support this, if there are no objections, should we create issues on contrib for the individual components that need some work?
@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.
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.
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.
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
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.
@TylerHelmuth Should we remove this from the Collector v1 board based on #10643?
@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.