Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate component.Host.GetExporters #7370

Open
7 of 8 tasks
djaglowski opened this issue Mar 14, 2023 · 8 comments
Open
7 of 8 tasks

Deprecate component.Host.GetExporters #7370

djaglowski opened this issue Mar 14, 2023 · 8 comments
Assignees

Comments

@djaglowski
Copy link
Member

djaglowski commented Mar 14, 2023

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:

@mx-psi
Copy link
Member

mx-psi commented Mar 15, 2023

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

@djaglowski
Copy link
Member Author

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

@dmitryax
Copy link
Member

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.

@mx-psi
Copy link
Member

mx-psi commented Feb 8, 2024

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
Copy link
Member

mx-psi commented Apr 3, 2024

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

@TylerHelmuth TylerHelmuth self-assigned this Apr 17, 2024
@mx-psi mx-psi moved this to In Progress in Collector: v1 Apr 18, 2024
evan-bradley pushed a commit to open-telemetry/opentelemetry-collector-contrib that referenced this issue Apr 18, 2024
**Description:** 
Removes all direct use of the `component.Host.GetExporters` function so
that it can be removed from the interface.

**Link to tracking Issue:** 
Works towards
open-telemetry/opentelemetry-collector#7370
@mx-psi mx-psi moved this from In Progress to Waiting for reviews in Collector: v1 Apr 18, 2024
mx-psi pushed a commit that referenced this issue Apr 22, 2024
**Description:** 
Remove the deprecated `GetExporters` function from `component.Host`

**Link to tracking Issue:** <Issue number if applicable>

Related to
#7370
@mx-psi mx-psi moved this from Waiting for reviews to Todo in Collector: v1 Apr 22, 2024
@mx-psi
Copy link
Member

mx-psi commented Apr 22, 2024

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.

@evan-bradley evan-bradley moved this from Todo to In Progress in Collector: v1 May 2, 2024
@mx-psi
Copy link
Member

mx-psi commented Jul 31, 2024

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

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Aug 7, 2024

@mx-psi based on the decision made in #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants