Skip to content

[cmd/opampsupervisor] Allow setting healthcheck host #39863

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

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented May 5, 2025

Description

Currently the opampsupervisor hardcode's the healthcheck extension's host to be localhost. This makes the extension unusable on kubernetes. This PR adds a new agent config setting to allow setting the health check host so that on k8s you can set the host to something like 0.0.0.0 or the pod IP.

Testing

Tested locally in kind using a local opampsupervisor image.

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.

Please add a changelog and update the readme as well to include the new option

@TylerHelmuth
Copy link
Member Author

@codeboten changelog added, but where should I update the README? Currently the README doesn't document the agent's health check configuration.

Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

I think we should consider a different approach for this. This property predates the OpAMP extension offering meaningful health reporting information, and now that it does, we no longer need the health check extension to determine the Collector is healthy from within the Supervisor. As a result, I think it would probably make more sense to not have the Supervisor configure a health check extension, but but instead to suggest the user directly configures one themselves, either through remote or local config.

Comment on lines +188 to 189
HealthCheckHost string `mapstructure:"health_check_host"`
HealthCheckPort int `mapstructure:"health_check_port"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should probably just remove this property entirely, but if we keep it, can we make it an endpoint instead of just a port?

@evan-bradley
Copy link
Contributor

where should I update the README? Currently the README doesn't document the agent's health check configuration.

There is a README in the Supervisor specification folder that we keep up-to-date. If we make a change here, we should update that as well.

That said, I think we should likely look at moving that section to the Supervisor's main README file.

@TylerHelmuth
Copy link
Member Author

I think it would probably make more sense to not have the Supervisor configure a health check extension, but but instead to suggest the user directly configures one themselves, either through remote or local config.

I can submit a different PR to implement this strategy if you want.

@TylerHelmuth
Copy link
Member Author

@evan-bradley what is the alternative strategy the opampsupervisor is already using to set health if not the health check extension?

@evan-bradley
Copy link
Contributor

The Supervisor receives health updates in a format that should be functionally-equivalent to what's provided by the health check extension here: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/cmd/opampsupervisor/supervisor/supervisor.go#L796.

I think we should switch to using these messages to determine the Collector's health status instead of the health check extension.

@TylerHelmuth
Copy link
Member Author

Ok I'll look into this switch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants