-
Notifications
You must be signed in to change notification settings - Fork 622
Move Postgres log rotation parameters to the postgres package #4215
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
Conversation
- pkg: math/rand$ | ||
desc: Use the "math/rand/v2" package instead. See https://go.dev/doc/go1.22#math_rand_v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason behind the switch to v2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason to stay away from v1, really. The v2 rand.N
is generic, so it can do time.Duration
without casts; that was nice.
The behavior of these parameters is independent of OpenTelemetry. This refactor is part of a larger series to make the Postgres log directory configurable.
prefix := strings.ReplaceAll(filePrefix, "%", "%%") | ||
suffix := strings.ReplaceAll(fileSuffix, "%", "%%") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏️ we're doubling %
to escape in case the prefix/suffix have %
-- but ours don't, right?
and if someone forgot/missed that we're handling this and doubled the %
originally, we'd quadruple the %
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ours don't, right?
Right. Escaping occurred to me as I was writing the test. I expect the postgres
package to know about these details, while other packages don't.
if someone forgot/missed that we're handling this and doubled the
%
originally, we'd quadruple the%
?
Yep. If someone thinks to escape before calling, their test would show the wacky number of escapes.
Checklist:
Type of Changes:
This moves the
log_filename
andlog_rotation
parameters intointernal/postgres
, documents how they're related, and adds some tests. 📝 Theinternal/collector
tests don't change.Issue: PGO-2558