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

Rename the health check endpoint to /healthz #467

Merged
merged 1 commit into from
Nov 2, 2023
Merged

Conversation

davidfowl
Copy link
Member

Fixes #447

@davidfowl
Copy link
Member Author

After this poll, I'm sure everyone will change this endpoint 😄

@brendandburns
Copy link
Contributor

brendandburns commented Oct 26, 2023

As I discussed w/ David, I'm team /healthy and /alive

@mitchdenny
Copy link
Member

+1 for healthz given the broader cloud native ecosystem is invested in this convention.

Copy link
Member

@IEvangelist IEvangelist left a comment

Choose a reason for hiding this comment

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

🤣 at the /healthz...

@@ -100,7 +100,7 @@ public static WebApplication MapDefaultEndpoints(this WebApplication app)
// app.MapPrometheusScrapingEndpoint();

// All health checks must pass for app to be considered ready to accept traffic after starting
app.MapHealthChecks("/readiness");
app.MapHealthChecks("/healthz");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
app.MapHealthChecks("/healthz");
app.MapHealthChecks("/healthy");

@@ -100,7 +100,7 @@ public static WebApplication MapDefaultEndpoints(this WebApplication app)
// app.MapPrometheusScrapingEndpoint();

// All health checks must pass for app to be considered ready to accept traffic after starting
app.MapHealthChecks("/readiness");
app.MapHealthChecks("/healthz");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
app.MapHealthChecks("/healthz");
app.MapHealthChecks("/healthy");

@@ -100,7 +100,7 @@ public static WebApplication MapDefaultEndpoints(this WebApplication app)
// app.MapPrometheusScrapingEndpoint();

// All health checks must pass for app to be considered ready to accept traffic after starting
app.MapHealthChecks("/readiness");
app.MapHealthChecks("/healthz");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
app.MapHealthChecks("/healthz");
app.MapHealthChecks("/healthy");

@DamianEdwards
Copy link
Member

@davidfowl are we doing this for preview.1 still?

@davidfowl
Copy link
Member Author

Yep, I will go with /health and /alive

@IEvangelist
Copy link
Member

Yep, I will go with /health and /alive

Why not /healthy? It's more aligned with /alive if that's a preference, but /health feels disjointed from /alive. If you're healthy, you're alive, but they're not dually synonymous. You could be alive, but unwell... Whereas, /health could be anything, what does this actually report? It is a bit, or some status object? Is it an enum with three possible values; Healthy, Degraded, and Unhealthy? If the latter is the case, then I'd assume that we wouldn't want /alive as I'd expect that to be either true or false.

When you say "and", are you using both, as in multiple routes that resolve with the same check? Just curious about this naming convention. I agree with @brendandburns on this one, but still a bit conflicted.

@davidfowl
Copy link
Member Author

davidfowl commented Oct 31, 2023

/health does a deep health check and checks all dependencies. It'll eventually also return detailed health status (it'll return a single result today, healthy, unhealthy, degraded)
/alive does a self-check without checking dependencies.

@davidfowl davidfowl enabled auto-merge (squash) November 2, 2023 18:39
@davidfowl davidfowl merged commit f9eb4b7 into main Nov 2, 2023
5 checks passed
@davidfowl davidfowl deleted the davidfowl/healthz branch November 2, 2023 19:04
@davidfowl
Copy link
Member Author

/backport to release/8.0-preview1

Copy link
Contributor

github-actions bot commented Nov 2, 2023

Started backporting to release/8.0-preview1: https://github.com/dotnet/aspire/actions/runs/6737243070

Copy link
Contributor

github-actions bot commented Nov 2, 2023

@davidfowl backporting to release/8.0-preview1 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Patch format detection failed.
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

github-actions bot commented Nov 2, 2023

@davidfowl an error occurred while backporting to release/8.0-preview1, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

davidfowl added a commit that referenced this pull request Nov 2, 2023
davidfowl added a commit that referenced this pull request Nov 2, 2023
joperezr added a commit that referenced this pull request Nov 14, 2023
@danmoseley danmoseley added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Nov 16, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider changing the health check endpoint to /healthz
6 participants