roachprod: redirect CRDB logs utility #132586
Conversation
|
This change is |
herkolategan
force-pushed
the
hbl/roachtest-crdb-log-intercept
branch
from
9c76148
to
89c79c3
Compare
herkolategan
requested review from
nameisbhaskar and
DarrylWong
and removed request for
a team
pkg/roachprod/logger/log_redirect.go
Outdated
| logRedirectInst.configured = true | ||
| } | ||
|
|
||
| // TestingCRDBLogConfig is meant to be used in tests to reset the CRDB log, it's |
There was a problem hiding this comment.
nit (maybe?): this referring to unit tests specifically right?
There was a problem hiding this comment.
Yes good point, I'll update the comment to be clear, since as you mentioned it could be interpreted as useable in a roachtest, which it should not.
| } | ||
| logConf := logconfig.DefaultStderrConfig() | ||
| logConf.Sinks.Stderr.Filter = logpb.Severity_FATAL | ||
| // Disable logging to a file. File sinks write the application arguments to |
There was a problem hiding this comment.
Is this just to be extra sure or will it still log to file even with the intercept below?
There was a problem hiding this comment.
Ah actually it seems like you need it at the very least because InterceptWith itself calls InfofDepth before setting up the intercept.
There was a problem hiding this comment.
Yes, if I remember correctly you have to validate and apply the config, otherwise the default logger kick in and starts printing to stderr/stdout which we want to avoid as well.
| logRedirectInst.Lock() | ||
| defer logRedirectInst.Unlock() | ||
| if logRedirectInst.configured { | ||
| panic("internal error: CRDB log interceptor already configured") |
There was a problem hiding this comment.
I wouldn't expect this to get called twice but is it something we want to panic for vs. returning an error/logging a warning?
There was a problem hiding this comment.
I went with panics mostly because that's how it was previously, but I'm not strongly opinionated against errors here. I think since it's during the init phase a panic is okay and will probably crash the whole process, which I'd expect if for some reason we can't configure the logging it would be good to investigate.
| require.True(t, found, "expected line not found: %s", line) | ||
| } | ||
|
|
||
| func TestLogRedirect(t *testing.T) { |
There was a problem hiding this comment.
If the goal of the PR is to make sure we stop leaking roachtest args, then should we have a test for that? Not entirely sure how to easily do that though, simplest thing I can think of is maybe assert that we only see one line in the writer?
There was a problem hiding this comment.
It's the main goal. Initially I had a goroutine router (scoping log calls for CRDB log to goroutines so it ends up with the test log) as well to maybe make the CRDB log more functional for roachtests. But it ended up feeling like an overly complex solution, so now it's just a barebones interceptor to avoid logging the args. We could technically make args optional in the logger itself, but I'd rather not mess with prod code for this.
herkolategan
force-pushed
the
hbl/roachtest-crdb-log-intercept
branch
from
89c79c3
to
d18d184
Compare
132594: roachtest: remove CRDB log usages r=DarrylWong a=herkolategan This PR removes all references to CRDB log usage in roachtest. There are still a few transitive dependencies that end up calling CRDB log. These are small enough and get redirected to a separate file if they need to be inspected. Ultimately, we want to ensure that the appropriate loggers get used in roachtests, that are supplied by the test framework. After this PR, a linter can be introduced to ban the top-level import of CRDB log from roachtest. The only remaining direct usage in roachtest is to configure CRDB log to use a file sink, but this will be updated by another PR #132586 that moves the redirect functionality alongside the roachprod logger implementation. Informs: #131412 Epic: None Release note: None Co-authored-by: Herko Lategan <[email protected]>
|
TFTR! bors r=DarrylWong |
132586: roachprod: redirect CRDB logs utility r=DarrylWong a=herkolategan Currently, CRDB log is configured, by roachtest, to log to a file to catch any logs written to it during a roachtest run. This is usually from a shared test util that uses the CRDB log. The file sink on the CRDB logger will log program arguments by default, but this can leak sensitive information. This PR introduces a log redirect that uses the CRDB log interceptor functionality instead of using a file sink. This way we can avoid logging the program arguments. Epic: None Release note: None Co-authored-by: Herko Lategan <[email protected]>
|
Build failed: |
Currently, CRDB log is configured, by roachtest, to log to a file to catch any logs written to it during a roachtest run. This is usually from a shared test util that uses the CRDB log. The file sink on the CRDB logger will log program arguments by default, but this can leak sensitive information. This PR introduces a log redirect that uses the CRDB log interceptor functionality instead of using a file sink. This way we can avoid logging the program arguments. Epic: None Release note: None
This replaces the initiation of the file sink based CRDB log with the new interceptor log redirect. It will log to a file in the artifacts directory. Epic: None Release note: None
herkolategan
force-pushed
the
hbl/roachtest-crdb-log-intercept
branch
from
d18d184
to
666a9cd
Compare
|
bors retry |
Currently, CRDB log is configured, by roachtest, to log to a file to catch any
logs written to it during a roachtest run. This is usually from a shared test
util that uses the CRDB log. The file sink on the CRDB logger will log program
arguments by default, but this can leak sensitive information.
This PR introduces a log redirect that uses the CRDB log interceptor
functionality instead of using a file sink. This way we can avoid logging the
program arguments.
Epic: None
Release note: None