[cmd/opampsupervisor] Add a healthcheck endpoint for the Supervisor #41565
Conversation
…tor-contrib into supervisor-healthcheck-endpoint
Signed-off-by: Douglas Camata <[email protected]>
Signed-off-by: Douglas Camata <[email protected]>
douglascamata
requested review from
a team,
atoulme and
evan-bradley
as code owners
By default, it's zero and in this case we don't start a healthcheck server.
atoulme
left a comment
atoulme
left a comment
There was a problem hiding this comment.
this can land as is imo - I have a nit about the network interface but I'm ok if we address it separately based on more needs being shored up
…tor-contrib into supervisor-healthcheck-endpoint
|
@atoulme @evan-bradley I finished taken care of the pending comments and did a small update to the respective changelog entry. Please have another look when you have some time. Thank you for your review. 🙇 |
|
There are some flaky test failures on |
atoulme
left a comment
atoulme
left a comment
There was a problem hiding this comment.
LGTM - @evan-bradley please take one more look and feel free to merge, thanks!
evan-bradley
left a comment
evan-bradley
left a comment
There was a problem hiding this comment.
Overall looks good, just a few suggestions. Two additional requests if you don't mind:
- Can we add this config option to one of the YAML files in
testdata/supervisorand verify the config is parsed correctly? This would probably be an E2E test. - Can you document this option in the readme?
|
@evan-bradley all the feedback taken care of, sir! |
|
Oh wait, got some weird test failures. |
|
@evan-bradley now the tests should all pass. |
|
It looks like there's a leaking goroutine in one of the E2E tests, I assume the new one unless something changed on main. I took a quick look and nothing jumped out at me. |
The interactions of the callbacks with the `connectedChan` had the risk to introduce a go-routine leak in the tests if `waitForSupervisorConnection` was not called before the Supervisor was shutdown. Closing the `connectedChan` in the OpAMP server shutdown function allowed me to detect this.
evan-bradley
left a comment
evan-bradley
left a comment
There was a problem hiding this comment.
Thanks for being so responsive and diligent throughout the review process, this one turned out to have a few more considerations than I was expecting at the onset.
4 participants
Description
This PR adds a healthcheck endpoint to the Supervisor. It runs in its own dedicated HTTP server. For now the servers uses port
23233(inspired on the Collector's default healthcheck port:13133). The Supervisor's healthcheck port is not configurable at the moment, but it could be in the future.Link to tracking issue
Fixes #40529.
Testing
Unit test added.