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

feat: implement local service health check #240

Merged
merged 23 commits into from
Oct 23, 2023
Merged

feat: implement local service health check #240

merged 23 commits into from
Oct 23, 2023

Conversation

winstxnhdw
Copy link
Contributor

@winstxnhdw winstxnhdw commented Sep 4, 2023

@winstxnhdw
Copy link
Contributor Author

winstxnhdw commented Sep 4, 2023

Still unsure how to test if a service is critical or warning. I am also having trouble trying to deserialise the AggregatedStatus enum. Is it even possible to deserialise pure strings with ConsulClient?

Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

Still unsure how to test if a service is critical or warning.

It is going to be tricky. I've implemented a test showing how to do it but it doesn't work.
The reason is that local health API returns an unsuccessful status code when at least one check is in a warning or critical state (https://developer.hashicorp.com/consul/api-docs/agent/service#get-local-service-health). Therefore we get an exception instead of a regular response. Not sure how to overcome this API inconsistency.

I am also having trouble trying to deserialise the AggregatedStatus enum. Is it even possible to deserialise pure strings with ConsulClient?

Just use HealthStatus class. I think that a simple enum would work just fine, but we don't need another enum class as the AggregatedStatus has the same possible values as health status, or doesn't it?

Consul/Agent.cs Outdated Show resolved Hide resolved
Consul/Agent.cs Outdated Show resolved Hide resolved
Consul.Test/AgentTest.cs Outdated Show resolved Hide resolved
winstxnhdw and others added 5 commits September 5, 2023 17:08
Co-authored-by: Marcin Krystianc <marcin.krystianc@gmail.com>
Co-authored-by: Marcin Krystianc <marcin.krystianc@gmail.com>
Co-authored-by: Marcin Krystianc <marcin.krystianc@gmail.com>
@winstxnhdw
Copy link
Contributor Author

The reason is that local health API returns an unsuccessful status code when at least one check is in a warning or critical state (https://developer.hashicorp.com/consul/api-docs/agent/service#get-local-service-health). Therefore we get an exception instead of a regular response. Not sure how to overcome this API inconsistency.

Haha, try-catch maybe?

@marcin-krystianc
Copy link
Contributor

Haha, try-catch maybe?

Well, it is better to avoid exceptions in the regular execution path. I think we have already an example of how to implement a special case for unsuccessful status codes, see

if (!response.IsSuccessStatusCode && (
(response.StatusCode != HttpStatusCode.NotFound && typeof(TOut) != typeof(TxnResponse)) ||
(response.StatusCode != HttpStatusCode.Conflict && typeof(TOut) == typeof(TxnResponse))))

You could implement a similar special case in the https://github.com/G-Research/consuldotnet/blob/master/Consul/Client_GetRequests.cs#L82

@winstxnhdw winstxnhdw marked this pull request as ready for review September 8, 2023 12:52
@winstxnhdw
Copy link
Contributor Author

winstxnhdw commented Sep 8, 2023

Whoops, marked this for review on accident.

@winstxnhdw
Copy link
Contributor Author

Sorry for the long delay! I was really busy with school and exams. Finally got around to working on it. @marcin-krystianc do let me know if I need to make any more changes.

Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

Hi @winstxnhdw, I've left some comments. I think there was an actual bug in the GetLocalServiceHealthByID method.

Consul.Test/AgentTest.cs Outdated Show resolved Hide resolved
Consul.Test/AgentTest.cs Outdated Show resolved Hide resolved
Consul/Agent.cs Outdated Show resolved Hide resolved
winstxnhdw and others added 3 commits October 19, 2023 18:42
Co-authored-by: Marcin Krystianc <marcin.krystianc@gmail.com>
Co-authored-by: Marcin Krystianc <marcin.krystianc@gmail.com>
@winstxnhdw
Copy link
Contributor Author

winstxnhdw commented Oct 19, 2023

Seems like I misunderstood the docs. I have removed GetWorstLocalHealthByID because querying by ID will query the status of the specific service with that ID. This means that GetLocalHealthByID is already equivalent. GetWorstLocalHealth, however, will find the worst health status among all services with the same name.

Copy link
Contributor

@marcin-krystianc marcin-krystianc 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, just one simple suggestion to rename variable in a test to avoid confusion.

Consul.Test/AgentTest.cs Outdated Show resolved Hide resolved
Co-authored-by: Marcin Krystianc <marcin.krystianc@gmail.com>
Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

👍

@marcin-krystianc marcin-krystianc merged commit f0af036 into G-Research:master Oct 23, 2023
148 checks passed
@winstxnhdw winstxnhdw deleted the health branch October 23, 2023 09:13
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.

Support for "/agent/health/service/*" API methods.
2 participants