Migrate healthcheckextension to internal/healthcheck implementation #42791
Conversation
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
d33d815 to
677e024
Compare
1316790 to
4f59c0b
Compare
Signed-off-by: Israel Blancas <[email protected]>
There was a problem hiding this comment.
Thanks again @iblancasa. Looking a bit more closely, I think we have some opportunities to simplify how we do this a little. Let me know what you think.
| ### Feature Gate: `extension.healthcheck.useCompatibilityWrapper` | ||
|
|
||
| - **Default**: Enabled (true) | ||
| - **Purpose**: Preserves v1 Ready/NotReady behavior when using the shared healthcheck implementation | ||
| - **When enabled**: Ready/NotReady calls directly control health endpoint status (original behavior) | ||
| - **When disabled**: Health status is determined by component status events (v2 behavior) |
There was a problem hiding this comment.
| ### Feature Gate: `extension.healthcheck.useCompatibilityWrapper` | |
| - **Default**: Enabled (true) | |
| - **Purpose**: Preserves v1 Ready/NotReady behavior when using the shared healthcheck implementation | |
| - **When enabled**: Ready/NotReady calls directly control health endpoint status (original behavior) | |
| - **When disabled**: Health status is determined by component status events (v2 behavior) | |
| ### Feature Gate: `extension.healthcheck.disableCompatibilityWrapper` | |
| - **Default**: Disabled (false) | |
| - **Purpose**: Transitions v1 Ready/NotReady behavior when using the shared healthcheck implementation | |
| - **When enabled**: Health status is determined by component status events (v2 behavior) | |
| - **When disabled**: Ready/NotReady calls directly control health endpoint status (original behavior) |
|
|
||
| const defaultPort = 13133 | ||
|
|
||
| // Feature gate to enable the compatibility wrapper that preserves v1 Ready/NotReady behavior |
There was a problem hiding this comment.
| // Feature gate to enable the compatibility wrapper that preserves v1 Ready/NotReady behavior | |
| // Feature gate to disable the compatibility wrapper that preserves v1 Ready/NotReady behavior |
| featuregate.WithRegisterFromVersion("v0.137.0"), | ||
| featuregate.WithRegisterToVersion("v0.142.0"), |
There was a problem hiding this comment.
| featuregate.WithRegisterFromVersion("v0.137.0"), | |
| featuregate.WithRegisterToVersion("v0.142.0"), | |
| featuregate.WithRegisterFromVersion("v0.138.0"), | |
| featuregate.WithRegisterToVersion("v0.143.0"), |
|
|
||
| 1. **Current**: Compatibility wrapper enabled by default - no breaking changes. | ||
| 2. **Future**: Feature gate will be removed, compatibility wrapper will be permanently disabled. | ||
| 3. **Recommended**: Test your setup with the feature gate disabled to prepare for future versions. |
There was a problem hiding this comment.
| 3. **Recommended**: Test your setup with the feature gate disabled to prepare for future versions. | |
| 3. **Recommended**: Test your setup with the feature gate enabled to prepare for future versions. |
| Interval: "5m", | ||
| ExporterFailureThreshold: 5, | ||
| }, | ||
| UseV2: false, |
There was a problem hiding this comment.
I feel like there's a lot of overlap between the intent of the UseV2 option and the feature gate we're adding in this PR. I think we should look at using the feature gate as a way of removing this legacy config section by doing something like the following:
- Create an aliased config struct in this package like in the v2 extension
- Implement a
Validatefunction that throws an error if any legacy config option is set while the feature gate is set. - If the feature gate isn't set, ensure
UseV2is false, and use the compatibility wrapper.
I think the UseV2 option essentially becomes pointless, but I suppose it could allow users to directly swap-in their v2 configs if they use it.
| ln, err := hc.config.ToListener(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to bind to address %s: %w", hc.config.Endpoint, err) | ||
| func newCompatibilityWrapper(sharedExt *healthcheck.HealthCheckExtension) *compatibilityWrapper { |
There was a problem hiding this comment.
Looking at this again, instead of the wrapper, couldn't we just keep the existing implementation when the feature gate is disabled and use the internal library when it's enabled? I think we will need to bridge the config and we could leave it as-is without worrying about mapping functionality between the two.
| // Then trigger a component status change to make the health endpoint report OK | ||
| // This bridges the old Ready() behavior to the new event-driven system | ||
| w.ComponentStatusChanged( | ||
| w.internalComponentID, | ||
| componentstatus.NewEvent(componentstatus.StatusOK), | ||
| ) |
There was a problem hiding this comment.
How does this work with the internal aggregation? If another component reports it encountered an error, that will trump this behavior and cause the extension to report the Collector is unhealthy, right?
Follow up for #42256.