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] Component status reporting rfc #10413

Merged

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Jun 14, 2024

Description

Adds an RFC for component status reporting. The main goal is to define what component status reporting is, our current, implementation, and how such a system interacts with a 1.0 component. When merged, the following issues will be unblocked:

@TylerHelmuth TylerHelmuth added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jun 14, 2024
Copy link

codecov bot commented Jun 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.89%. Comparing base (18d5c02) to head (3adc81e).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10413   +/-   ##
=======================================
  Coverage   91.89%   91.89%           
=======================================
  Files         411      411           
  Lines       19316    19316           
=======================================
  Hits        17750    17750           
  Misses       1217     1217           
  Partials      349      349           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TylerHelmuth TylerHelmuth force-pushed the component-status-reporting-rfc branch from 97a059a to 649dead Compare June 17, 2024 20:42
@TylerHelmuth TylerHelmuth marked this pull request as ready for review June 17, 2024 20:43
@TylerHelmuth TylerHelmuth requested a review from a team June 17, 2024 20:43
docs/rfcs/component-status-reporting.md Show resolved Hide resolved
docs/rfcs/component-status-reporting.md Outdated Show resolved Hide resolved
docs/rfcs/component-status-reporting.md Outdated Show resolved Hide resolved
docs/rfcs/component-status-reporting.md Outdated Show resolved Hide resolved
docs/rfcs/component-status-reporting.md Outdated Show resolved Hide resolved
docs/rfcs/component-status-reporting.md Outdated Show resolved Hide resolved
docs/rfcs/component-status-reporting.md Outdated Show resolved Hide resolved
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
mx-psi added a commit that referenced this pull request Jun 20, 2024
This PR adds documentation for the collector status reporting system. It
describes the current state of the system and has a section for best
practices that we intend to evolve as we develop them. The intended
audience is future users of the system and anyone interested in getting
a deeper look into how the system works without having to read all of
the code. This is intended to be complementary to the [in-progress
RFC](#10413).

[Here is a
preview](https://github.com/open-telemetry/opentelemetry-collector/blob/61abf91b4faec42905b409c352e0e234e5b75ac9/docs/component-status.md)
with the diagrams properly rendered.

---------

Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

When designing the existing component status system, we faced many challenges related to component instances. For example, a single processor configuration may be used in multiple pipelines, in which case multiple instances of the processor will be instantiated. Another example, a receiver or exporter which is used in pipelines of different data types will have one instance per data type. Should these instances share a single status or do they each have their own? If each instance has its own status, how do we uniquely identify these instances and communicate each one's status to users?

In my opinion this is a problem which needs to be addressed in any status system design. Alternatively, I think we should consider ways to remove these problems by making changes to the way components are instanced such that all instances correspond to exactly one configuration in the collector config.

docs/rfcs/component-status-reporting.md Outdated Show resolved Hide resolved
docs/rfcs/component-status-reporting.md Outdated Show resolved Hide resolved
docs/rfcs/component-status-reporting.md Outdated Show resolved Hide resolved
docs/rfcs/component-status-reporting.md Show resolved Hide resolved
docs/rfcs/component-status-reporting.md Outdated Show resolved Hide resolved
mx-psi pushed a commit that referenced this pull request Aug 13, 2024
#### Description
Adds a `Reporter` interface that represents how a `component.Host`
implementation could expose the ability to report a status.

You can see how this interface will be used by looking at Related to
#10777

<!-- Issue number if applicable -->
#### Link to tracking issue
Related to
#10777
Related to
#10413

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Added unit test
mx-psi pushed a commit that referenced this pull request Aug 14, 2024
#### Description
This is a prep PR to reduce the size of
#10777. As
part of the work to make our `component.Host` implementation implement
`componentstatus.Reporter` (see
#10852),
the `host` struct and `graph` logic need to be closer together. This is
because, as part of
#10777
`StartAll` is changed to depend on our specific `Host` type instead of a
`component.Host`. Our host already has a dependency on `graph`, so it
can't be moved into its own module.

<!-- Issue number if applicable -->
#### Link to tracking issue
Related to
#10777
Related to
#10413
mx-psi added a commit that referenced this pull request Aug 16, 2024
)

#### Description

This PR removes `ReportStatus` from `component.TelemetrySettings` and
instead expects components to check if their `component.Host` implements
a new `componentstatus.Reporter` interface.

<!-- Issue number if applicable -->
#### Link to tracking issue
Related to
#10725
Related to
#10413

<!--Describe what testing was performed and which tests were added.-->
#### Testing
unit tests and a sharedinstance e2e test.

The contrib tests will fail because this is a breaking change. If we
merge this I and @mwear can commit to updating contrib before the next
release.

---------

Co-authored-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks @TylerHelmuth

@mx-psi
Copy link
Member

mx-psi commented Aug 22, 2024

I think we are good to merge this once @mwear takes another look :)

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

LGTM. After the refactor, I believe the references to service.Service as a component.Host implementation are no longer accurate, and should be replaced by graph.Host. I've made some suggestions to that effect.

docs/rfcs/component-status-reporting.md Outdated Show resolved Hide resolved
docs/rfcs/component-status-reporting.md Outdated Show resolved Hide resolved
docs/rfcs/component-status-reporting.md Outdated Show resolved Hide resolved
docs/rfcs/component-status-reporting.md Outdated Show resolved Hide resolved
TylerHelmuth and others added 2 commits August 22, 2024 12:46
Co-authored-by: Matthew Wear <matthew.wear@gmail.com>
@TylerHelmuth TylerHelmuth added the ready-to-merge Code review completed; ready to merge by maintainers label Aug 22, 2024
@codeboten
Copy link
Contributor

Reviewed by @mwear, merging now

@codeboten codeboten merged commit dadc331 into open-telemetry:main Aug 22, 2024
49 checks passed
@TylerHelmuth TylerHelmuth deleted the component-status-reporting-rfc branch August 22, 2024 22:20
sfc-gh-sili pushed a commit to sfc-gh-sili/opentelemetry-collector that referenced this pull request Aug 23, 2024
Adds an RFC for component status reporting. The main goal is to define
what component status reporting is, our current, implementation, and how
such a system interacts with a 1.0 component. When merged, the following
issues will be unblocked:
- open-telemetry#9823
- open-telemetry#10058
- open-telemetry#9957
- open-telemetry#9324
- open-telemetry#6506

---------

Co-authored-by: Matthew Wear <matthew.wear@gmail.com>
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Code review completed; ready to merge by maintainers Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants