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

[component] Review component.Host.GetFactory #9511

Closed
mx-psi opened this issue Feb 7, 2024 · 5 comments · Fixed by #10771
Closed

[component] Review component.Host.GetFactory #9511

mx-psi opened this issue Feb 7, 2024 · 5 comments · Fixed by #10771
Assignees
Labels
area:component help wanted Good issue for contributors to OpenTelemetry Service to pick up release:required-for-ga Must be resolved before GA release

Comments

@mx-psi
Copy link
Member

mx-psi commented Feb 7, 2024

component.Host.GetFactory is used by a single component: the receivercreator. While it may not be possible it would be great to consider if we can remove this or maybe make it optional (by having an optional interface that the service can implement). This would simplify the Host interface

@mx-psi mx-psi added release:required-for-ga Must be resolved before GA release area:component labels Feb 7, 2024
@mx-psi
Copy link
Member Author

mx-psi commented May 20, 2024

One example where we do the optional interface thingie

hostZPages, ok := host.(interface {
RegisterZPages(mux *http.ServeMux, pathPrefix string)
})
if ok {
hostZPages.RegisterZPages(zPagesMux, "/debug")
zpe.telemetry.Logger.Info("Registered Host's zPages")
} else {
zpe.telemetry.Logger.Warn("Host's zPages not available")
}

@mx-psi mx-psi added the help wanted Good issue for contributors to OpenTelemetry Service to pick up label Jul 17, 2024
@TylerHelmuth
Copy link
Member

Removing it from the component.Host interface and leaving it implemented on our implementation of component.Host seems fine to me.

On the other hand, the functionality is interesting and simple for any component.Host to implement, since it should already have all the component factories it is managing. Unlike #7370 where connectors were the clear solution to components needing to call GetExporters, this method is the only way for 1 component to create another.

Since we only have 1 use case that utilizes this feature, I am in favor of the optional interface approach, but would be ok if we choose to keep it on the interface.

@atoulme
Copy link
Contributor

atoulme commented Jul 20, 2024

I think we could change the behavior so that the GetFactory method is available as a func as a receiver factory option. I'll try and draft something for review.

@atoulme
Copy link
Contributor

atoulme commented Jul 20, 2024

Here is the draft PR with a design experiment: #10679

I changed the design a bit so you can only get access to receiver factories, hopefully that's fine given the only use case we have is about creating receivers.

@mx-psi
Copy link
Member Author

mx-psi commented Jul 22, 2024

I think #10679 is interesting, but it also seems like something we can do after 1.0. If we are okay with removing this from component.Host and keeping on service, I think that's the bare minimum we can do now and we can look into something like #10679 afterwards

@TylerHelmuth TylerHelmuth self-assigned this Jul 22, 2024
TylerHelmuth added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this issue Jul 26, 2024
…nterface (#34234)

**Description:**
Updates receivercreator in preparation for `component.Host.GetFactory`
to be removed.

**Link to tracking Issue:** <Issue number if applicable>
Related to
open-telemetry/opentelemetry-collector#9511

**Testing:** <Describe what testing was performed and which tests were
added.>
Unit tests.

I cant add a unit test yet that fails the interface check since being
compliant with `component.Host` still requires the `GetFactory` method.
codeboten pushed a commit that referenced this issue Jul 26, 2024
#### Description
This PR deprecates the `component.Host.GetFactory` interface. This has
the benefit of keeping the `component.Host` interface as simple as
possible. Components that were relying on this method can instead check
if the underlying `component.Host` implementation supports the
interface. An example of this pattern can be found here:
https://github.com/open-telemetry/opentelemetry-collector/blob/91f13c309d00eef5b997a2e810effb2a4ccd95d4/extension/zpagesextension/zpagesextension.go#L58-L66

#### Link to tracking issue
Related to
#9511
@mx-psi mx-psi closed this as completed in 2bb2613 Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:component help wanted Good issue for contributors to OpenTelemetry Service to pick up release:required-for-ga Must be resolved before GA release
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants