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] Move component status reporting public API to new componentstatus module #10730

Merged

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Jul 26, 2024

Description

Duplicates component status reporting features from component into a separate module, componentstatus. In a future PR, when component.TelemetrySettings.ReportStatus is removed, I'll update Core to depend on componentstatus.

This work isolates component status public API from component and extensions, which will allow us to move forward with their 1.0 work while component status reporting matures.

Link to tracking issue

Related to #10725

Copy link

codecov bot commented Jul 26, 2024

Codecov Report

Attention: Patch coverage is 97.67442% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.12%. Comparing base (524a901) to head (e99f56d).

Files Patch % Lines
component/componentstatus/status.go 97.67% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10730      +/-   ##
==========================================
+ Coverage   92.11%   92.12%   +0.01%     
==========================================
  Files         403      404       +1     
  Lines       18806    18849      +43     
==========================================
+ Hits        17323    17365      +42     
- Misses       1123     1124       +1     
  Partials      360      360              

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

Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM, just a bit dubitative, because this adds a new module to stabilize.

@TylerHelmuth
Copy link
Member Author

@atoulme the current plan is to not stabilize consumerstatus as part of 1.0 work. Component status reporting currently has not direct user configuration and no public api in service. Once ReportStatus and ComponentStatus Watcher are removed from component and extension respectively there won't be any public status reporting APIs in the modules we marked in scope.

@atoulme
Copy link
Contributor

atoulme commented Jul 27, 2024

I see, that’s an elegant way to narrow down what we need to stabilize. I appreciate the rationale. Looks good to me.

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.

LGTM, but we can wait until we have solved the main questions on your draft PR

component/componentstatus/status.go Show resolved Hide resolved
component/componentstatus/status.go Outdated Show resolved Hide resolved
component/componentstatus/instance.go Outdated Show resolved Hide resolved
component/componentstatus/status_test.go Outdated Show resolved Hide resolved
component/componentstatus/status_test.go Outdated Show resolved Hide resolved
@mwear
Copy link
Member

mwear commented Jul 29, 2024

@atoulme the current plan is to not stabilize consumerstatus as part of 1.0 work. Component status reporting currently has not direct user configuration and no public api in service. Once ReportStatus and ComponentStatus Watcher are removed from component and extension respectively there won't be any public status reporting APIs in the modules we marked in scope.

As mentioned here, I have serious reservations about this plan.

TylerHelmuth and others added 3 commits July 30, 2024 02:36
Co-authored-by: Matthew Wear <matthew.wear@gmail.com>
Co-authored-by: Matthew Wear <matthew.wear@gmail.com>
Co-authored-by: Matthew Wear <matthew.wear@gmail.com>
@mx-psi
Copy link
Member

mx-psi commented Jul 30, 2024

I think we should move forward with this. The healtcheck extension is not in our GA roadmap, and in any case doing this unblocks progress in many other modules needed for 1.0.

}

// Timestamp returns the timestamp associated with the Event
func (ev *Event) Timestamp() time.Time {
Copy link
Member

Choose a reason for hiding this comment

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

Timestamp seem to be use-less, because it is always "now", which means consumers (watchers) can also read if they need it.

Copy link
Member

Choose a reason for hiding this comment

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

This is important for the healthcheck extension, which also needs to synthesize events. When synthesizing events the timestamp needs to be settable. We used to do this in core, but have removed it, at least temporarily. I was proposing we add an option to set the timestamp in a followup: #10730 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

What about, if this is needed for that extension, we can have StatusWithTime struct {Status, Time} in the healthcheck and not here. No point in having it here.

Copy link
Member

Choose a reason for hiding this comment

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

Also, until we discuss the next PR, I would rather remove this for the moment.

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 prefer to keep this PR a 1-to-1 move of stuff related to component status reporting into componentstatus. In my opinion we can have API discussions about componentstatus in future issues/PRs. I can update the godocs to state that everything in componentstatus is experimental and currently exempt from our deprecation process if you'd like.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to keep this PR a 1-to-1 move of stuff related to component status reporting into componentstatus. In my opinion we can have API discussions about componentstatus in future issues/PRs. I can update the godocs to state that everything in componentstatus is experimental and currently exempt from our deprecation process if you'd like.

Then update the title to "move code" instead of adding a new module which to me means we can do a better job.

Copy link
Member

Choose a reason for hiding this comment

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

I think a timestamp will be useful for other status watchers and worth considering for the generated status event. Otherwise, if there are multiple watchers that care about the timestamp, each one will have to wrap the event in a custom struct and generate timestamps independently.

Copy link
Member

Choose a reason for hiding this comment

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

I think a timestamp will be useful for other status watchers and worth considering for the generated status event. Otherwise, if there are multiple watchers that care about the timestamp, each one will have to wrap the event in a custom struct and generate timestamps independently.

When in doubt leave it out. I am not convinced yet, and until we have 2-3 use-cases we should not add it, since adding later is backwards compatible, but currently we pay the cost of reading the time as well as the maintenance cost.

Copy link
Member Author

Choose a reason for hiding this comment

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

Followup issue created: #10763

Copy link
Member

Choose a reason for hiding this comment

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

I will still suggest the timestamp has value. I wrote the following on the issue:

I believe that we should keep the timestamp as the observed time is not the same time as the observation. Since status watchers are dispatched in sequence, any status watcher that does a non-trivial amount of work, or something that could block (like writing to an event to a channel) could delay the observed time for subsequent watchers. This would be especially problematic if the delay was variable between invocations. Currently the timestamp is used by the health check extension for time calculations.

// Watcher is an extra interface for Extension hosted by the OpenTelemetry
// Collector that is to be implemented by extensions interested in changes to component
// status.
type Watcher 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 would move this + the InstanceId to the "extension/statuswatcher` or something like that to be consistent with storage and other type of extensions.

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 think you're probably correct, but I'd prefer to encompass all public component status reporting features behind componentstatus for now while it is experimental and subject to lots of breaking API discussions. Once we feel better about its API we could move it into extension/storage as a final location. Likely other aspects of componentstatus will move around in the future as well.

Copy link
Member

Choose a reason for hiding this comment

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

Not suggesting to extension/storage but to extension/statuswatcher :)

Copy link
Member

Choose a reason for hiding this comment

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

I am confused about why we do this then. Is this to just remove unstable code (that is ok)? But still you should at least document all these things as todos (file issues) and then we can do it in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing unstable code from component and extension is the goal. Issue created: #10764

@TylerHelmuth TylerHelmuth changed the title [component] Add componentstatus module [component] Move component status reporting public API to new componentstatus module Jul 31, 2024
@mx-psi mx-psi requested review from bogdandrutu and mwear July 31, 2024 09:35
@mx-psi
Copy link
Member

mx-psi commented Jul 31, 2024

AIUI all remaining issues are about the content of the module and not about the split itself so IMO we can go ahead with this. @bogdandrutu @mwear could you have a final look before I merge?

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

@bogdandrutu bogdandrutu merged commit 6d32c09 into open-telemetry:main Jul 31, 2024
50 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants