-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[chore] extension/observer: move Endpoints{Lister,Watcher} to a new package #38416
Conversation
ed3fdc7
to
a73133c
Compare
There was a problem hiding this 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) |
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.