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

[chore] extension/observer: move Endpoints{Lister,Watcher} to a new package #38416

Merged
merged 2 commits into from
Mar 7, 2025

Conversation

axw
Copy link
Contributor

@axw axw commented Mar 6, 2025

Description

Move EndpointsLister and EndpointsWatcher to a new "endpointswatcher" package. The intention here is to more clearly delineates the public API of the observer package -- the bits inside endpointswatcher are intended only for implementations of observers. See #38412 (comment) and #38412 (comment) for further motivation.

I've made the new package non-internal in case there are any observer implementations out of tree.

Link to tracking issue

N/A

Testing

Ran/updated unit tests

Documentation

N/A except for adding package doc explaining that endpointswatcher is intended for implementations only.

@axw axw force-pushed the extension-observer-internal-api branch from ed3fdc7 to a73133c Compare March 6, 2025 04:58
@axw axw marked this pull request as ready for review March 6, 2025 05:20
Copy link
Contributor

@dehaansa dehaansa left a comment

Choose a reason for hiding this comment

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

Looks straightforward, one small question.

"github.com/open-telemetry/opentelemetry-collector-contrib/internal/kafka"
)

var (
_ extension.Extension = (*kafkaTopicsObserver)(nil)
_ observer.EndpointsLister = (*kafkaTopicsObserver)(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a couple of these compile time assertions removed, is there a reason they were removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the observer.EndpointsLister assertions because that's the contract between the implementations and EndpointsWatcher -- it's an internal detail of the observer. It's only the observer.Observable interface that needs to be implemented for consumers (namely receivercreator:

obs, ok := ext.(observer.Observable)
)

@dehaansa dehaansa added the ready to merge Code review completed; ready to merge by maintainers label Mar 7, 2025
@songy23 songy23 merged commit 59f4abc into open-telemetry:main Mar 7, 2025
197 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 7, 2025
@axw axw deleted the extension-observer-internal-api branch March 10, 2025 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension/observer ready to merge Code review completed; ready to merge by maintainers receiver/receivercreator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants