NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

feat(coderd/healthcheck): allow configuring database hc threshold#10623

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

johnstcn merged 10 commits intomainfromcj/configurable-db-hc-threshold

Nov 13, 2023

Merged

feat(coderd/healthcheck): allow configuring database hc threshold#10623

johnstcn merged 10 commits intomainfromcj/configurable-db-hc-threshold

Nov 13, 2023

Conversation

Copy link

Member

johnstcn commentedNov 10, 2023•
edited
Loading

Fixes#9381:

Exposes database healthcheck threshold in configuration

Also takes care of part of#8969:

Exposes healthcheck recheck interval in configuration

feat(coderd/healthcheck): allow configuring database hc threshold

2484951

johnstcn self-assigned this

Nov 10, 2023

johnstcn added4 commits

November 10, 2023 20:55

feat(coderd): add database hc latency, plumb through

9e5c030

gen etc.

42477e3

plumb through healthcheck refresh / timeout

15361a3

make gen etc. again

20e713e

johnstcn commented

Nov 10, 2023

View reviewed changes

coderd/healthcheck/database.go Outdated

Comment on lines 22 to 23

		LatencyMs int64 `json:"latency_ms"`
		ThresholdMs int64 `json:"threshold_ms"`

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

self-review: I can make theseints if preferred, but int64 is slightly less annoying to use here.

coderd/healthcheck/healthcheck.go

Comment on lines -52 to +50

		DB database.Store
		// TODO: support getting this over HTTP?
		DERPMap *tailcfg.DERPMap
		AccessURL *url.URL
		Client *http.Client
		APIKey string
		AccessURL AccessURLReportOptions
		Database DatabaseReportOptions
		DerpHealth derphealth.ReportOptions
		Websocket WebsocketReportOptions

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

self-review: Refactored this struct to directly pass in the individual checker options.

coderd/coderd.go Outdated

Comment on lines 419 to 423

		if options.HealthcheckTimeout == 0 {
		options.HealthcheckTimeout =30 * time.Second
		options.HealthcheckTimeout =options.DeploymentValues.Healthcheck.Timeout.Value()
		}
		if options.HealthcheckRefresh == 0 {
		options.HealthcheckRefresh =10 * time.Minute
		options.HealthcheckRefresh =options.DeploymentValues.Healthcheck.Refresh.Value()

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

self-review: Also plumbed through the healthcheck timeout and refresh and made them configurable. Addresses part of#8969 but I can break this into a separate PR if preferred.

johnstcn marked this pull request as ready for review

November 10, 2023 17:10

johnstcn requested review fromdeansheather,coadler,mafredri andmtojek and removed request forcoadler anddeansheather

November 10, 2023 17:10

mafredri reviewed

Nov 10, 2023

View reviewed changes

cli/testdata/coder_server_--help.golden OutdatedShow resolvedHide resolved

coderd/healthcheck/database.go OutdatedShow resolvedHide resolved

codersdk/deployment.go OutdatedShow resolvedHide resolved

Copy link

Member

matifali commentedNov 11, 2023

We should also show a hint on the database health check page on how to configure this interval.
Probably expose it somewhere on the deployment page too. We are currently exposing many of the configured options there.

johnstcn added2 commits

November 13, 2023 08:43

renaming

8115ffe

more renaming

bc8e200

Copy link

MemberAuthor

johnstcn commentedNov 13, 2023•
edited
Loading

@matifali We should also show a hint on the database health check page on how to configure this interval. Probably expose it somewhere on the deployment page too. We are currently exposing many of the configured options there.

Right now they'll show up by default under Deployment > Observability. I can move them to a separate option group and move them to a separate page if required.

I might need some design help with adding the hint to the health page though, so I'll defer that to a separate PR.

Copy link

Member

matifali commentedNov 13, 2023

I think this is fine for now. A hint could be added in a separate PR.

Thank you.

mtojek reviewed

Nov 13, 2023

View reviewed changes

cli/testdata/coder_server_--help.golden OutdatedShow resolvedHide resolved

cli/testdata/server-config.yaml.golden OutdatedShow resolvedHide resolved

coderd/healthcheck/database.go

		Reachable bool `json:"reachable"`
		Latency string `json:"latency"`
		LatencyMS int64 `json:"latency_ms"`
		ThresholdMS int64 `json:"threshold_ms"`

Copy link

Member

mtojekNov 13, 2023

Choose a reason for hiding this comment

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

Should we add theThreshold string for consistency? I don't know what is the original though behind string+int64 pairs.

Copy link

MemberAuthor

johnstcnNov 13, 2023•
edited
Loading

Choose a reason for hiding this comment

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

Me either, and the fact that I don't know why this particular fence exists makes me pause before knocking it down. I do know that_ms was added post-hoc for the UI (8ee500c) so I suspect it was simply a case of not renaming a field in the API for compatibility purposes.

Instead of parsing the Go string representation (latency) it's probably better for the UI to just create the durations from millis and then display them however is deemed best. So I would argue in favour of leaving outthreshold entirely.

johnstcn added3 commits

November 13, 2023 13:26

yet more renaming

973a9bb

revert configurability for hc timeout, keep hard-coded at 30s

58361e8

Merge remote-tracking branch 'origin/main' into cj/configurable-db-hc…

1de19c0

…-threshold

mtojek self-requested a review

November 13, 2023 13:45

johnstcn requested a review frommafredri

November 13, 2023 13:45

mtojek approved these changes

Nov 13, 2023

View reviewed changes

Copy link

Member

mtojek left a comment

Choose a reason for hiding this comment

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

This is in good shape to get merged 👍

johnstcn merged commitb69c237 intomain

Nov 13, 2023

johnstcn deleted the cj/configurable-db-hc-threshold branch

November 13, 2023 14:14

github-actionsbot locked and limited conversation to collaborators

Nov 13, 2023

mafredri approved these changes

Nov 13, 2023

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

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

Nice work!

cli/testdata/server-config.yaml.golden

		# database exceeds this threshold over 5 attempts, the database is considered
		# unhealthy. The default value is 15ms.
		# (default: 15ms, type: duration)
		thresholdDatabase: 15ms

Copy link

Member

mafredriNov 13, 2023

Choose a reason for hiding this comment

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

I think this would be smoother asdatabaseThreshold vsthresholdDatabase (same with flags, etc)

Labels

None yet

4 participants

Movatterモバイル変換

feat(coderd/healthcheck): allow configuring database hc threshold#10623

feat(coderd/healthcheck): allow configuring database hc threshold#10623

Uh oh!

Conversation

johnstcn commentedNov 10, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

johnstcnNov 10, 2023

Choose a reason for hiding this comment

Uh oh!

johnstcnNov 10, 2023

Choose a reason for hiding this comment

Uh oh!

johnstcnNov 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matifali commentedNov 11, 2023

Uh oh!

johnstcn commentedNov 13, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

matifali commentedNov 13, 2023

Uh oh!

Uh oh!

Uh oh!

mtojekNov 13, 2023

Choose a reason for hiding this comment

Uh oh!

johnstcnNov 13, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

mafredriNov 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johnstcn commentedNov 10, 2023•
edited
Loading

johnstcn commentedNov 13, 2023•
edited
Loading

johnstcnNov 13, 2023•
edited
Loading