feat: show health check warnings #10620

mtojek · 2023-11-10T11:19:10Z

This PR extends the /api/v2/debug/health API response to include warning bool field. Coder will not report "unhealthy" status if there are troubles with Websockets.

johnstcn · 2023-11-10T14:31:14Z

site/src/testHelpers/entities.ts

  },
  database: {
    healthy: true,
    reachable: true,
-    latency: 92570,
-    error: null,
+    latency: "92570",


Can we get rid of this field? (not in this PR)

Many places in this API response are discussable. I second the question.

johnstcn · 2023-11-10T14:45:03Z

coderd/healthcheck/healthcheck.go

 	// Healthy is true if the report returns no errors.
 	Healthy bool `json:"healthy"`
+	// Warning is true when Coder is operational but its performance might be impacted.
+	Warning bool `json:"warning"`


What if we get a report that has both { Healthy: false, Warning: true }?
As far as I understand, that's an invalid state in our current model, which should not be representible.

You're right, but I'm afraid that it will not be achievable without breaking API? I introduced the "warning" field to be perceived as "it is healthy, but...".

To solve it properly, most likely we need to replace "Healthy" with enum, for instance: status: healthy, status: degraded, status: error, etc.

I'm happy to chat about this as it is the right moment.

The more I think about it, it could be useful to have both health and warn information, as they are not necessarily completely separate.

For example, if you're using embedded database and access URL just points straight to the VM, and you get a failed healthcheck for database taking too long and warnings for access URL taking too long to respond, that can already give you an indication that perhaps the server is overloaded.

Maybe we could add

Warnings []string or map[string]string

at the top level, and any warnings from component reports get added in?

I agree with this concept if only we present these warnings on the "Health" page 👍 Otherwise, it is pointless. It also means that the Health page will become more informative than just Healthy/Unhealthy + JSON dump.

mtojek

This PR touches few important aspects we may want to discuss:

Warning bool vs. Warning []string (Cian) vs "status enum". I don't have any preference, for Warning bool seems to be sufficient, but I don't know where we would like to go with the design. I wouldn't like to drive a new RFC just because of that.
Can we move Healthcheck API response to codersdk?
So, can "site" use API responses from the codersdk? Right now, it uses its own custom implementation. Unfortunately, it seems to be incorrect/not fully implemented.

I will ping a couple of folks so that we have an opinionated design.

mtojek · 2023-11-10T15:26:58Z

site/src/testHelpers/entities.ts

@@ -2456,7 +2459,8 @@ export const MockHealth = {
              TokenBucketBytesBurst: 0,
            },
            can_exchange_messages: true,
-            round_trip_ping: 7674330,
+            round_trip_ping: "7674330",


Same question as Cian posted for "latency". Is it backward compatibility?

mtojek · 2023-11-10T15:27:29Z

site/src/testHelpers/entities.ts

@@ -2467,18 +2471,16 @@ export const MockHealth = {
              ],
            ],
            client_errs: [[], []],
-            error: null,


Does UI/CLI/something use "error" fields?

mtojek · 2023-11-10T15:29:04Z

coderd/healthcheck/derphealth/derp.go

+	// A node may use websockets because `Upgrade: DERP` may be blocked on
+	// the load balancer. This is unhealthy because websockets are slower
+	// than the regular DERP protocol.
+	r.Warning = r.UsesWebsocket


This is the most important logical change. Using WebSockets won't be considered to be a failure anymore.

mtojek · 2023-11-10T15:31:28Z

site/src/api/api.ts

 export const getHealth = async () => {
-  const response = await axios.get<Health>("/api/v2/debug/health");
+  const response = await axios.get<TypesGen.HealthcheckReport>(


@BrunoQuaresma Do you know why it uses Health instead of TypesGen.HealthcheckReport? Is there any strong reason why it does not use api/api?

Extracted it here: #10650

I don't remember but this is a good improvement for sure.

mtojek · 2023-11-10T15:33:37Z

site/src/api/api.ts

 export const getHealth = async () => {
-  const response = await axios.get<Health>("/api/v2/debug/health");
+  const response = await axios.get<TypesGen.HealthcheckReport>(


@BrunoQuaresma It seems that make gen does not generate DerphealthcheckReport even though it is marked to be generated. Do you have any idea why it happens?

@@ -2097,7 +2097,9 @@ export interface HealthcheckReport { readonly healthy: boolean; readonly warning: boolean; readonly failing_sections: string[]; - readonly derp: DerphealthReport; + // Named type "github.com/coder/coder/v2/coderd/healthcheck/derphealth.Report" unknown, using "any" + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- External type + readonly derp: any; readonly access_url: HealthcheckAccessURLReport; readonly websocket: HealthcheckWebsocketReport; readonly database: HealthcheckDatabaseReport

Extracted it here: #10650

BrunoQuaresma

From the FE perspective, it looks good

mtojek · 2023-11-15T12:47:08Z

I'm going to redo this according to the redesign notion doc.

feat: show health warnings

6f4c14d

mtojek self-assigned this Nov 10, 2023

mtojek changed the title ~~feat: show health warnings~~ feat: show health check warnings Nov 10, 2023

mtojek added 6 commits November 10, 2023 12:26

make gen

059e9fa

make site/src/api/typesGenerated.ts

ee8bd1b

WIP

99893ff

WIP

5eba101

WIP

b0dace2

fmt

f97a359

johnstcn reviewed Nov 10, 2023

View reviewed changes

mtojek added 4 commits November 10, 2023 15:57

Fix

7f82c1b

fix

6fed35b

Remove comment

e9497ee

Fix: test

fa7224f

mtojek commented Nov 10, 2023

View reviewed changes

mtojek requested review from BrunoQuaresma, deansheather and spikecurtis November 10, 2023 15:41

mtojek mentioned this pull request Nov 13, 2023

refactor(site): use generated Healthcheck API entities #10650

Merged

BrunoQuaresma approved these changes Nov 13, 2023

View reviewed changes

mtojek added 2 commits November 14, 2023 12:50

Merge branch 'main' into 9470-fallback-is-healthy

6495bb0

Merge branch 'main' into 9470-fallback-is-healthy

75f271e

mtojek closed this Nov 15, 2023

github-actions bot locked and limited conversation to collaborators Nov 15, 2023

github-actions bot deleted the 9470-fallback-is-healthy branch May 16, 2024 00:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: show health check warnings #10620

feat: show health check warnings #10620

mtojek commented Nov 10, 2023 •

edited

Loading

johnstcn Nov 10, 2023 •

edited

Loading

mtojek Nov 10, 2023

johnstcn Nov 10, 2023

mtojek Nov 10, 2023

johnstcn Nov 10, 2023 •

edited

Loading

mtojek Nov 10, 2023

mtojek left a comment •

edited

Loading

mtojek Nov 10, 2023

mtojek Nov 10, 2023

mtojek Nov 10, 2023

mtojek Nov 10, 2023

mtojek Nov 13, 2023

BrunoQuaresma Nov 13, 2023

mtojek Nov 10, 2023

mtojek Nov 13, 2023

BrunoQuaresma left a comment

mtojek commented Nov 15, 2023

feat: show health check warnings #10620

feat: show health check warnings #10620

Conversation

mtojek commented Nov 10, 2023 • edited Loading

johnstcn Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrunoQuaresma left a comment

Choose a reason for hiding this comment

mtojek commented Nov 15, 2023

mtojek commented Nov 10, 2023 •

edited

Loading

johnstcn Nov 10, 2023 •

edited

Loading

johnstcn Nov 10, 2023 •

edited

Loading

mtojek left a comment •

edited

Loading