Skip to content

feat(cli): rotate file logs for coderd #15438

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

Merged
merged 4 commits into from
Nov 12, 2024
Merged

feat(cli): rotate file logs for coderd #15438

merged 4 commits into from
Nov 12, 2024

Conversation

defelmnq
Copy link
Contributor

@defelmnq defelmnq commented Nov 8, 2024

Related to #15309

As we already are doing for agent logs - this PR is enabling the logs rotation for coderd logs.

Currently keeping the same logic than we had for agent - with 5MB as the file size for rotation.

@defelmnq defelmnq linked an issue Nov 8, 2024 that may be closed by this pull request
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM, but want to ensure we have like-for-like behaviour with the agent.

}
closers = append(closers, fi.Close)
sinks = append(sinks, sinkFn(fi))
closers = append(closers, logWriter.Close)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this give us the equivalent behaviour of lumberjackWriteCloseFixer in cli/agent.go?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should probably re-use that same fix here, although the main reason for that seems to have been to reduce CI flakes due to racy writes after close. We probably won't be logging to a file in CI in the general case.

Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

See inline comment below; bonus point for a test in ./cli but I'm not going to insist on it for this PR.

@dannykopping
Copy link
Contributor

See inline comment below; bonus point for a test in ./cli but I'm not going to insist on it for this PR.

Agreed on a test; I was thinking about this over the weekend.
We rely on this lumberjack lib, which has its own tests, but if we ever wanted to swap it out for another we'd want the functionality to remain the same. We can at least test the functional aspect.

Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

I smoke-tested this locally and noted our existing test coverage in TestServer/Logging.*. Testing Lumberjack's rotation is probably way too much overhead here.

If we wanted to add a test, we could extract addSinkIfNotProvided() in cli/clilog and test that on its own.

@dannykopping
Copy link
Contributor

I smoke-tested this locally and noted our existing test coverage in TestServer/Logging.*. Testing Lumberjack's rotation is probably way too much overhead here.

I trust your judgment. I'm happy to merge as-is.

@defelmnq defelmnq merged commit d6442db into main Nov 12, 2024
26 checks passed
@defelmnq defelmnq deleted the feat-rotate-coderd-logs branch November 12, 2024 20:48
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.

Rotate server logs
3 participants