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

chore: reduce log volume on server startup#16608

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

hugodutka merged 7 commits intomainfromhugodutka/reduce-log-volume-on-startup

Feb 20, 2025

Merged

chore: reduce log volume on server startup#16608

hugodutka merged 7 commits intomainfromhugodutka/reduce-log-volume-on-startup

Feb 20, 2025

Conversation

Copy link

Contributor

hugodutka commentedFeb 18, 2025•
edited
Loading

Addresses#16231.

This PR reduces the volume of logs we print after server startup in order to surface the web UI URL better.

Here are the logs after the changes a couple of seconds after starting the server:

The warning is due to running a development site-less build. It wouldn't show in a release build.

github-actionsbot assignedhugodutka

Feb 18, 2025

hugodutka force-pushed thehugodutka/reduce-log-volume-on-startup branch 4 times, most recently frombfaffb7 toe18ef6fCompare

February 18, 2025 17:13

hugodutka marked this pull request as ready for review

February 18, 2025 17:18

hugodutka requested a review frommafredri

February 18, 2025 17:18

hugodutka force-pushed thehugodutka/reduce-log-volume-on-startup branch 2 times, most recently from5c7de02 tob1b217bCompare

February 18, 2025 18:02

mafredri reviewed

Feb 18, 2025

View reviewed changes

Copy link

Member

mafredri left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Some nice changes in here, thanks. Had a few comments/concerns and CC'd@johnstcn and@spikecurtis on the ones I was uncertain about.

Hmm, looks like something went awry with the review. ccs for Cian on crypto, Spike on tailnet seem to be gone 😅 .

provisioner/terraform/serve.go OutdatedShow resolvedHide resolved

provisionerd/provisionerd.go OutdatedShow resolvedHide resolved

site/site.go OutdatedShow resolvedHide resolved

johnstcn reviewed

Feb 18, 2025

View reviewed changes

coderd/database/dbpurge/dbpurge.goShow resolvedHide resolved

johnstcn reviewed

Feb 18, 2025

View reviewed changes

coderd/cryptokeys/rotate.goShow resolvedHide resolved

spikecurtis reviewed

Feb 19, 2025

View reviewed changes

Copy link

Contributor

spikecurtis left a comment

Choose a reason for hiding this comment

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

Ok from my perspective

hugodutka force-pushed thehugodutka/reduce-log-volume-on-startup branch 3 times, most recently from54b3e03 to2f31144Compare

February 19, 2025 14:15

change notifications disabled message level to debug

56f1535

hugodutka force-pushed thehugodutka/reduce-log-volume-on-startup branch from2f31144 toc9033d8Compare

February 20, 2025 10:45

mafredri reviewed

Feb 20, 2025

View reviewed changes

cli/server_test.go OutdatedShow resolvedHide resolved

provisionerd/provisionerd.go OutdatedShow resolvedHide resolved

provisioner/terraform/serve.go Outdated

		slog.F("max_version", maxTerraformVersion.String()))
		// When running in-memory provisioners, each one reaches this point.
		// It's not useful to print this to the user multiple times, so we ensure to log it once.
		if !loggedVersion.Swap(true) {

Copy link

Member

mafredriFeb 20, 2025

Choose a reason for hiding this comment

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

It's possible now that we end up logging on a minor terraform version mis-match, in which case we end up downloading a newer one and this log ends up reporting incorrect information.

Perhaps it's better moved to Serve?

PS. If we can avoid the package level state, that'd be great (for instance, can be a source of flaky tests).