Introduce component status reporting #6550
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@open-telemetry/collector-approvers @open-telemetry/collector-contrib-approvers please take a look at the draft and tell me if you like the direction. |
Codecov ReportBase: 91.33% // Head: 91.35% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #6550 +/- ##
==========================================
+ Coverage 91.33% 91.35% +0.02%
==========================================
Files 242 246 +4
Lines 13888 14055 +167
==========================================
+ Hits 12684 12840 +156
- Misses 959 968 +9
- Partials 245 247 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
bogdandrutu
reviewed
Nov 15, 2022
There was a problem hiding this comment.
Super hard to review, are you ok to only keep for the moment changes in "component/*" in the PR for the moment?
Overall feedback:
- Direction is fine.
- Need to better understand if the pipeline status should be also part of this mechanism (maybe postpone for later that part).
- Do we go on a model where components register for updates, or we define an optional interface that Components can implement and we check for that instead of exposing the API on the Host?
- In general I want to make components "less" aware about their ID. For that I think service has full control on the Host instance we pass to each component (and knows/can guarantee the right ID is set in the wrapper).
| } | ||
|
|
||
| // ComponentEventOption applies options to a ComponentEvent. | ||
| type ComponentEventOption func(*ComponentEvent) error |
There was a problem hiding this comment.
either make the arg of the func private, or use an interface with a private func.
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package status |
There was a problem hiding this comment.
Reading this package, makes me think that we want this in component, because all of the structs start with "component".
There was a problem hiding this comment.
I would rename component.StatusEvent the main struct of this package and get rid of it.
There was a problem hiding this comment.
That was my initial approach, but it pollutes the component package with many new public symbols. I think it is a bit better this way to isolate status-related stuff in a separate sub-package. No strong opinion, I can merge if we think that's better.
Comment on lines
+58
to
+61
| // Source is the component that is reporting its status. | ||
| func (ev *ComponentEvent) Source() Source { | ||
| return ev.source | ||
| } |
There was a problem hiding this comment.
Not sure about this, I think we should not make the component that aware of the ID. I think we already "wrap" the host, so we know which component reports the Event to us.
There was a problem hiding this comment.
Yes, I can hide the ID on the "generating" side. But we still need to keep it on the "listening" side. See my other comment.
There was a problem hiding this comment.
Yes, pass it as separate arg.
| } | ||
|
|
||
| // ListenerOption applies options to a Listener. | ||
| type ListenerOption func(*Listener) |
There was a problem hiding this comment.
Same comment here, right now func(*Listener) is public for no good reason.
| // the following events: | ||
| // - Events reported by via the ReportComponentStatus function, | ||
| // - Notifications about pipeline status changes (ready, not ready). | ||
| RegisterStatusListener(options ...status.ListenerOption) StatusListenerUnregisterFunc |
There was a problem hiding this comment.
Looks like status.Listener is not used anywhere.
There was a problem hiding this comment.
It is used in notifications.go.
tigrannajaryan
force-pushed
the
feature/tigran/status_reporting
branch
from
5285937 to
d73c804
Compare
Try now, I only kept component/* and service/* for now. |
## Summary of changes - Add component/status package. This introduces the concepts of component status event and pipeline readiness status. - Introduce Host.RegisterStatusListener() to allow components to listen for status changes. - Deprecate Host.ReportFatalError() in favour of Host.ReportComponentStatus(). - Move component ID and Type to a separate component/id package. This is necessary to avoid dependency cycle with the new component/status package. Large part of the diff in this commit is because of this particular change. If this is too large and difficult to review we can split this commit into 2 parts where this particular change is a separate commit. - Deprecated component.ID and component.Type in favour of id.ID and id.Type. ## TODO after this is merged - healthcheck extension must register and listen to component statuses. - Replace all ReportFatalError() calls by ReportComponentStatus() calls in core and contrib. ## Open Questions - Do we want to name component/id package component/componentid instead? - The pipelines readiness status is also implemented in the component/status package. We can split the status, componentstatus and pipelinestatus into separate packages but not sure if it warrants creation of a new top-level package just to be able to stay pure in the component package. We can try this if think it is a better approach. - Listeners need to be able to tell if all current components are healthy. It is assumed that the listeners need to maintain a map of components and track the status of each component. This works only if we assume that the set of components cannot change during the lifetime of the listener. This assumption is true today but can change in the future if we introduce partial pipeline restarts where only modified/added/removed components are recreated (this will break listener's assumption and the map will become invalid). Should we instead keep track of this entire status map in the Host and broadcast the entire status to the listeners as a whole instead of (or in addition to) individual component events?
tigrannajaryan
force-pushed
the
feature/tigran/status_reporting
branch
from
d73c804 to
0de249e
Compare
This is the current approach we use with PipelineWatcher interface. If prefer this approach then we can do that. An important difference is that with Host some other bit of code can register to listen (not sure why we would need it), while with this approach only components can. |
Yes, with current approach the source component isn't aware of its ID. The Source field in the event is filled by hostWrapper. The listener MUST be aware of component IDs in order to keep a full status of all components. The full status is necessary so that the healthcheck can aggregate it into one boolean "Healthy" value. To keep the full status the listener needs a map where the key is source component ID. If we want to hide the component IDs from status reporting mechanism then we have to keep track of the full status in the Host (hostWrapper/service) and only notify listeners about full status and not individual component statuses. |
If the source of the "Source" is |
tigrannajaryan
added a commit
to tigrannajaryan/opentelemetry-collector
that referenced
this pull request
Nov 15, 2022
This is an alternate to open-telemetry#6550 ## Summary of changes - Add component status concept. Components can report their status via Host.ReportComponentStatus(). Interested extensions can watch status events if they implement StatusWatcher interface. This is similar to how PipelineWatcher works today. - Deprecate Host.ReportFatalError() in favour of Host.ReportComponentStatus(). ## TODO after this is merged - healthcheck extension must implement StatusWatcher. - Replace all ReportFatalError() calls by ReportComponentStatus() calls in core and contrib. ## Open Questions - StatusWatchers need to be able to tell if all current components are healthy. It is assumed that the listeners need to maintain a map of components and track the status of each component. This works only if we assume that the set of components cannot change during the lifetime of the listener. This assumption is true today but can change in the future if we introduce partial pipeline restarts where only modified/added/removed components are recreated (this will break listener's assumption and the map will become invalid). Should we instead keep track of this entire status map in the Host and broadcast the entire status to the listeners as a whole instead of (or in addition to) individual component events?
tigrannajaryan
added a commit
to tigrannajaryan/opentelemetry-collector
that referenced
this pull request
Nov 15, 2022
This is an alternate to open-telemetry#6550 ## Summary of changes - Add component status concept. Components can report their status via Host.ReportComponentStatus(). Interested extensions can watch status events if they implement StatusWatcher interface. This is similar to how PipelineWatcher works today. - Deprecate Host.ReportFatalError() in favour of Host.ReportComponentStatus(). ## TODO after this is merged - healthcheck extension must implement StatusWatcher. - Replace all ReportFatalError() calls by ReportComponentStatus() calls in core and contrib. ## Open Questions - StatusWatchers need to be able to tell if all current components are healthy. It is assumed that the listeners need to maintain a map of components and track the status of each component. This works only if we assume that the set of components cannot change during the lifetime of the listener. This assumption is true today but can change in the future if we introduce partial pipeline restarts where only modified/added/removed components are recreated (this will break listener's assumption and the map will become invalid). Should we instead keep track of this entire status map in the Host and broadcast the entire status to the listeners as a whole instead of (or in addition to) individual component events?
|
An alternate approach here: #6560 |
tigrannajaryan
added a commit
to tigrannajaryan/opentelemetry-collector
that referenced
this pull request
Nov 16, 2022
This is an alternate to open-telemetry#6550 ## Summary of changes - Add component status concept. Components can report their status via Host.ReportComponentStatus(). Interested extensions can watch status events if they implement StatusWatcher interface. This is similar to how PipelineWatcher works today. - Deprecate Host.ReportFatalError() in favour of Host.ReportComponentStatus(). ## TODO after this is merged - healthcheck extension must implement StatusWatcher. - Replace all ReportFatalError() calls by ReportComponentStatus() calls in core and contrib. ## Open Questions - StatusWatchers need to be able to tell if all current components are healthy. It is assumed that the listeners need to maintain a map of components and track the status of each component. This works only if we assume that the set of components cannot change during the lifetime of the listener. This assumption is true today but can change in the future if we introduce partial pipeline restarts where only modified/added/removed components are recreated (this will break listener's assumption and the map will become invalid). Should we instead keep track of this entire status map in the Host and broadcast the entire status to the listeners as a whole instead of (or in addition to) individual component events?
tigrannajaryan
added a commit
to tigrannajaryan/opentelemetry-collector
that referenced
this pull request
Nov 16, 2022
This is an alternate to open-telemetry#6550 ## Summary of changes - Add component status concept. Components can report their status via Host.ReportComponentStatus(). Interested extensions can watch status events if they implement StatusWatcher interface. This is similar to how PipelineWatcher works today. - Deprecate Host.ReportFatalError() in favour of Host.ReportComponentStatus(). ## TODO after this is merged - healthcheck extension must implement StatusWatcher. - Replace all ReportFatalError() calls by ReportComponentStatus() calls in core and contrib. ## Open Questions - StatusWatchers need to be able to tell if all current components are healthy. It is assumed that the listeners need to maintain a map of components and track the status of each component. This works only if we assume that the set of components cannot change during the lifetime of the listener. This assumption is true today but can change in the future if we introduce partial pipeline restarts where only modified/added/removed components are recreated (this will break listener's assumption and the map will become invalid). Should we instead keep track of this entire status map in the Host and broadcast the entire status to the listeners as a whole instead of (or in addition to) individual component events?
tigrannajaryan
added a commit
to tigrannajaryan/opentelemetry-collector
that referenced
this pull request
Nov 16, 2022
This is an alternate to open-telemetry#6550 ## Summary of changes - Add component status concept. Components can report their status via Host.ReportComponentStatus(). Interested extensions can watch status events if they implement StatusWatcher interface. This is similar to how PipelineWatcher works today. - Deprecate Host.ReportFatalError() in favour of Host.ReportComponentStatus(). ## TODO after this is merged - healthcheck extension must implement StatusWatcher. - Replace all ReportFatalError() calls by ReportComponentStatus() calls in core and contrib. ## Open Questions - StatusWatchers need to be able to tell if all current components are healthy. It is assumed that the listeners need to maintain a map of components and track the status of each component. This works only if we assume that the set of components cannot change during the lifetime of the listener. This assumption is true today but can change in the future if we introduce partial pipeline restarts where only modified/added/removed components are recreated (this will break listener's assumption and the map will become invalid). Should we instead keep track of this entire status map in the Host and broadcast the entire status to the listeners as a whole instead of (or in addition to) individual component events?
|
Closing in favour of #6560 |
tigrannajaryan
added a commit
to tigrannajaryan/opentelemetry-collector
that referenced
this pull request
Nov 21, 2022
This is an alternate to open-telemetry#6550 ## Summary of changes - Add component status concept. Components can report their status via Host.ReportComponentStatus(). Interested extensions can watch status events if they implement StatusWatcher interface. This is similar to how PipelineWatcher works today. - Deprecate Host.ReportFatalError() in favour of Host.ReportComponentStatus(). ## TODO after this is merged - healthcheck extension must implement StatusWatcher. - Replace all ReportFatalError() calls by ReportComponentStatus() calls in core and contrib. ## Open Questions - StatusWatchers need to be able to tell if all current components are healthy. It is assumed that the listeners need to maintain a map of components and track the status of each component. This works only if we assume that the set of components cannot change during the lifetime of the listener. This assumption is true today but can change in the future if we introduce partial pipeline restarts where only modified/added/removed components are recreated (this will break listener's assumption and the map will become invalid). Should we instead keep track of this entire status map in the Host and broadcast the entire status to the listeners as a whole instead of (or in addition to) individual component events?
mwear
pushed a commit
to mwear/opentelemetry-collector
that referenced
this pull request
Aug 2, 2023
This is an alternate to open-telemetry#6550 - Add component status concept. Components can report their status via Host.ReportComponentStatus(). Interested extensions can watch status events if they implement StatusWatcher interface. This is similar to how PipelineWatcher works today. - Deprecate Host.ReportFatalError() in favour of Host.ReportComponentStatus(). - healthcheck extension must implement StatusWatcher. - Replace all ReportFatalError() calls by ReportComponentStatus() calls in core and contrib. - StatusWatchers need to be able to tell if all current components are healthy. It is assumed that the listeners need to maintain a map of components and track the status of each component. This works only if we assume that the set of components cannot change during the lifetime of the listener. This assumption is true today but can change in the future if we introduce partial pipeline restarts where only modified/added/removed components are recreated (this will break listener's assumption and the map will become invalid). Should we instead keep track of this entire status map in the Host and broadcast the entire status to the listeners as a whole instead of (or in addition to) individual component events?
mwear
pushed a commit
to mwear/opentelemetry-collector
that referenced
this pull request
Aug 23, 2023
This is an alternate to open-telemetry#6550 - Add component status concept. Components can report their status via Host.ReportComponentStatus(). Interested extensions can watch status events if they implement StatusWatcher interface. This is similar to how PipelineWatcher works today. - Deprecate Host.ReportFatalError() in favour of Host.ReportComponentStatus(). - healthcheck extension must implement StatusWatcher. - Replace all ReportFatalError() calls by ReportComponentStatus() calls in core and contrib. - StatusWatchers need to be able to tell if all current components are healthy. It is assumed that the listeners need to maintain a map of components and track the status of each component. This works only if we assume that the set of components cannot change during the lifetime of the listener. This assumption is true today but can change in the future if we introduce partial pipeline restarts where only modified/added/removed components are recreated (this will break listener's assumption and the map will become invalid). Should we instead keep track of this entire status map in the Host and broadcast the entire status to the listeners as a whole instead of (or in addition to) individual component events?
mwear
pushed a commit
to mwear/opentelemetry-collector
that referenced
this pull request
Sep 10, 2023
This is an alternate to open-telemetry#6550 - Add component status concept. Components can report their status via Host.ReportComponentStatus(). Interested extensions can watch status events if they implement StatusWatcher interface. This is similar to how PipelineWatcher works today. - Deprecate Host.ReportFatalError() in favour of Host.ReportComponentStatus(). - healthcheck extension must implement StatusWatcher. - Replace all ReportFatalError() calls by ReportComponentStatus() calls in core and contrib. - StatusWatchers need to be able to tell if all current components are healthy. It is assumed that the listeners need to maintain a map of components and track the status of each component. This works only if we assume that the set of components cannot change during the lifetime of the listener. This assumption is true today but can change in the future if we introduce partial pipeline restarts where only modified/added/removed components are recreated (this will break listener's assumption and the map will become invalid). Should we instead keep track of this entire status map in the Host and broadcast the entire status to the listeners as a whole instead of (or in addition to) individual component events?
mwear
pushed a commit
to mwear/opentelemetry-collector
that referenced
this pull request
Sep 26, 2023
This is an alternate to open-telemetry#6550 - Add component status concept. Components can report their status via Host.ReportComponentStatus(). Interested extensions can watch status events if they implement StatusWatcher interface. This is similar to how PipelineWatcher works today. - Deprecate Host.ReportFatalError() in favour of Host.ReportComponentStatus(). - healthcheck extension must implement StatusWatcher. - Replace all ReportFatalError() calls by ReportComponentStatus() calls in core and contrib. - StatusWatchers need to be able to tell if all current components are healthy. It is assumed that the listeners need to maintain a map of components and track the status of each component. This works only if we assume that the set of components cannot change during the lifetime of the listener. This assumption is true today but can change in the future if we introduce partial pipeline restarts where only modified/added/removed components are recreated (this will break listener's assumption and the map will become invalid). Should we instead keep track of this entire status map in the Host and broadcast the entire status to the listeners as a whole instead of (or in addition to) individual component events?
mwear
pushed a commit
to mwear/opentelemetry-collector
that referenced
this pull request
Sep 29, 2023
This is an alternate to open-telemetry#6550 - Add component status concept. Components can report their status via Host.ReportComponentStatus(). Interested extensions can watch status events if they implement StatusWatcher interface. This is similar to how PipelineWatcher works today. - Deprecate Host.ReportFatalError() in favour of Host.ReportComponentStatus(). - healthcheck extension must implement StatusWatcher. - Replace all ReportFatalError() calls by ReportComponentStatus() calls in core and contrib. - StatusWatchers need to be able to tell if all current components are healthy. It is assumed that the listeners need to maintain a map of components and track the status of each component. This works only if we assume that the set of components cannot change during the lifetime of the listener. This assumption is true today but can change in the future if we introduce partial pipeline restarts where only modified/added/removed components are recreated (this will break listener's assumption and the map will become invalid). Should we instead keep track of this entire status map in the Host and broadcast the entire status to the listeners as a whole instead of (or in addition to) individual component events?
mwear
pushed a commit
to mwear/opentelemetry-collector
that referenced
this pull request
Oct 6, 2023
This is an alternate to open-telemetry#6550 - Add component status concept. Components can report their status via Host.ReportComponentStatus(). Interested extensions can watch status events if they implement StatusWatcher interface. This is similar to how PipelineWatcher works today. - Deprecate Host.ReportFatalError() in favour of Host.ReportComponentStatus(). - healthcheck extension must implement StatusWatcher. - Replace all ReportFatalError() calls by ReportComponentStatus() calls in core and contrib. - StatusWatchers need to be able to tell if all current components are healthy. It is assumed that the listeners need to maintain a map of components and track the status of each component. This works only if we assume that the set of components cannot change during the lifetime of the listener. This assumption is true today but can change in the future if we introduce partial pipeline restarts where only modified/added/removed components are recreated (this will break listener's assumption and the map will become invalid). Should we instead keep track of this entire status map in the Host and broadcast the entire status to the listeners as a whole instead of (or in addition to) individual component events?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
TODO after this is merged
Open Questions