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

[receiver/receivercreator] Make receivercreator expose its required interface #34234

Conversation

TylerHelmuth
Copy link
Member

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

Link to tracking Issue:
Related to open-telemetry/opentelemetry-collector#9511

Testing:
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.

@TylerHelmuth TylerHelmuth added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 23, 2024
@TylerHelmuth TylerHelmuth requested review from a team, codeboten and mx-psi July 23, 2024 21:49
@github-actions github-actions bot requested a review from rmfitzpatrick July 23, 2024 21:49
@@ -34,16 +34,27 @@ func newReceiverCreator(params receiver.Settings, cfg *Config) receiver.Metrics
}
}

// Host is an interface that the component.Host passed to receivercreator's Start function must implement
type Host interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it need to be exported?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's good to have the interface so that the component explicitly says what it needs from the component.Host. If it's controversial though, maybe we can make it private for now, enshrine it on some documentation on how to deal with this case, and make it public then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can hide it for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am asking if it can be unexported because in general we are trying to keep the public API of any component down to its config struct if possible.

Do we see any code import this module and use this host interface?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we see any code import this module and use this host interface?

No, it doesn't need to be public at this time.

@@ -34,16 +34,27 @@ func newReceiverCreator(params receiver.Settings, cfg *Config) receiver.Metrics
}
}

// Host is an interface that the component.Host passed to receivercreator's Start function must implement
type Host interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's good to have the interface so that the component explicitly says what it needs from the component.Host. If it's controversial though, maybe we can make it private for now, enshrine it on some documentation on how to deal with this case, and make it public then?

@mx-psi mx-psi requested a review from atoulme July 25, 2024 17:35
mx-psi added a commit to open-telemetry/opentelemetry-collector that referenced this pull request Jul 25, 2024
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

<!-- Issue number if applicable -->

Adds a note to the `component.Host` interface documenting that
components may require additional interfaces.
I think this is the only sustainable pattern post 1.0, since we won't be
able to add any new methods to the `component.Host` interface.

This pattern is also used today by:
- The zpages extension
(https://github.com/open-telemetry/opentelemetry-collector/blob/91f13c309d00eef5b997a2e810effb2a4ccd95d4/extension/zpagesextension/zpagesextension.go#L58-L66)
- The receiver creator (after
open-telemetry/opentelemetry-collector-contrib/pull/34234)
- Some contrib components
(open-telemetry/opentelemetry-collector-contrib/pull/32498)

#### Link to tracking issue

Fixes #10181
@TylerHelmuth TylerHelmuth merged commit 79c0bf1 into open-telemetry:main Jul 26, 2024
154 checks passed
@TylerHelmuth TylerHelmuth deleted the receiver-creator-use-own-interface branch July 26, 2024 16:37
@github-actions github-actions bot added this to the next release milestone Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
receiver/receivercreator Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants