Fix V logging to also filter out secrets #12079
Conversation
Changelog[uncommitted] (2023-02-07)Bug Fixes
|
sdk/go/common/util/logging/log.go
Outdated
| @@ -43,8 +43,28 @@ var LogFlow = false // true to flow logging settings to child processes. | |||
| var rwLock sync.RWMutex | |||
| var filters []Filter | |||
|
|
|||
| func V(level glog.Level) glog.Verbose { | |||
| return glog.V(level) | |||
| type VerboseShim glog.Verbose | |||
There was a problem hiding this comment.
RE: name: How about one of "VerboseLogger", "VerboseLog", "VLog", or "Chatty"?
Naming-wise, logging.*Log does stutter a bit, but it also doesn't look like the type is intended to be used directly much so it's not a problem.
There was a problem hiding this comment.
Yeh I'll probably go with VerboseLogger is the most clear, and yes most uses of this means no one actually uses the type name.
Frassle
force-pushed
the
fraser/glogging
branch
from
9d2653d
to
8cf781a
Compare
That ends up being the case for a lot of stuff in sdk/go/common, it's this really odd bag of bits some used only by the engine, some used internally by the engine and sdk but not really expected to be used by users, and some used by providers (which at the moment is mostly just us). In this case I think it's safe to take this change. This should only effect provider authors if anyone, and I think it's very unlikely they'll have references to
I've been thinking about that. The current logging system has a number of flaws and it would be good to replace it with a modern structured logging solution. Nice thing about this change (which I think is very low risk) is it would allow us to facade to a new system from this module making it easier to migrate to something more modern. I was going to add a point to ideation this week to talk logging systems. |
We use a small logging wrapper around glog for most of our logging. The global Errorf/Warningf/Infof methods in that module format the string and args passed in then filter out any secrets before calling into glog with the final string. The wrapper also exposed a `V` method to only log if verbosity was set, but that directly returned a glog.Verbose which mean the Infof calls to that didn't run our secret filtering code. This changes the logging wrapper to return a `VerboseShim` (Not a great name, but `Verbose` was already taken in this module) which is simply a new type over `glog.Verbose` with the same methods but it calls `FilterString` on the inputs. Technically a breaking change, but I think the uses of this will be source compatible.
Frassle
force-pushed
the
fraser/glogging
branch
from
8cf781a
to
8d7e8c4
Compare
|
bors merge |
|
🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
|
Build succeeded: |
Description
We use a small logging wrapper around glog for most of our logging. The global Errorf/Warningf/Infof methods in that module format the string and args passed in then filter out any secrets before calling into glog with the final string.
The wrapper also exposed a
Vmethod to only log if verbosity was set, but that directly returned a glog.Verbose which mean the Infof calls to that didn't run our secret filtering code.This changes the logging wrapper to return a
VerboseShim(Not a great name, butVerbosewas already taken in this module) which is simply a new type overglog.Verbosewith the same methods but it callsFilterStringon the inputs.Technically a breaking change, but I think the uses of this will be source compatible.
Checklist
make changelogand committed thechangelog/pending/<file>documenting my change