Changes configuration logging to be reflect-based. #563
Conversation
structlog/structlog.go
Outdated
| case reflect.Struct: | ||
| logStructWithLogger(v, prefix, logger) | ||
| case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: | ||
| logInt(v, prefix, logger) |
There was a problem hiding this comment.
It doesn't seem like logInt does anything that requires the reflect.Value, could this just be:
logger("%s: %d", prefix, v.Int())along with the remaining cases?
There was a problem hiding this comment.
I did it that way to resemble the struct case and be more easily extensible if we want further functionality in the future. I was initially expecting to need more than just one short line though, so if you really think it would be better inlined, I don't have a problem with that.
structlog/structlog.go
Outdated
| @@ -0,0 +1,104 @@ | |||
| package structlog | |||
There was a problem hiding this comment.
I know this isn't exactly config, but... I'd still recommend keeping it in the config package for now.
Motivations being:
- We have no other use-cases on the radar in the near future.
- By using
mapstructure, its usage is basically tied to Viper structs anyway. - If we really generalized it enough to be reusable, it should probably be its own repo/project. However, it's tough to see what's "too specialized" in it until you have more use-csaes.
So... IMO, better to keep it in config until we have some more use-cases.
structlog/structlog.go
Outdated
| var mapregex *regexp.Regexp | ||
| var blacklistregexp []*regexp.Regexp | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
Let's not (ever, if we can avoid it) use init() functions... they complicate the code's lifecycle, and tbh I have no idea what value they actually add. I'm not alone here either.
In this case, you can easily define them inline:
var mapregex *regexp.Regexp = regexp.MustCompile(`mapstructure:"([^"]+)"`)
var blacklistregexp []*regexp.Regexp = []*regexp.Regexp{
regexp.MustCompile("password"),
}
(ironically, I understand that the article also recommends "no package-level variables" too. I think he's right about that in general, except that immutable/threadsafe ones like these are ok. If all package-level vars were bad, then we'd never declare consts either... which I think is a little ridiculous)
structlog/structlog.go
Outdated
| return fmt.Sprintf("%s.%s", prefix, field) | ||
| } | ||
|
|
||
| func logInt(v reflect.Value, prefix string, logger func(msg string, args ...interface{})) { |
There was a problem hiding this comment.
I'm very sympathetic to small functions, but... these are only called once, aren't they? Subjective, but... I think the code reads better if they're inlined.
config/structlog.go
Outdated
| // LogGeneral will log nearly any sort of value, but requires the name of the root object to be in the | ||
| // prefix if you want that name to be logged. Structs will append .<fieldname> recursively to the prefix | ||
| // to document deeper structure. | ||
| func LogGeneral(v reflect.Value, prefix string) { |
There was a problem hiding this comment.
I did it the other way around. :) Oh, yes, don't even need logGeneral as public.
* 'master' of https://github.com/prebid/prebid-server: (93 commits) Separated the bidder info from the endpoint (prebid#585) More granular adapter error metrics (prebid#550) Deleted the validate endpoint, and dead code files. (prebid#541) Deleted the /getuids endpoint. (prebid#531) Moved the cookie parsing into the actual ParseCookie function. (prebid#584) [adform adapter] Pass gdpr & digitrust to bid request (prebid#581) Adding endpoint exposing prebid-server version (prebid#577) Infer imp.secure state (prebid#572) Changing custom parameter name (prebid#576) Adds map support to config logging. (prebid#573) Moves the metrics config under pbsmetrics where it should be rather than in the general config module. (prebid#571) Reorganizes the metrics code to support metrics submodules the same way we do in other sections. (prebid#568) Consolidates 2 metrics engine functions and fixes missing metric from… (prebid#558) Changes configuration logging to be reflect-based. (prebid#563) Cookie sync URL gdpr support for Brightroll adapter (prebid#562) Improve config logging & validation (prebid#560) Use the cache type to determine if no cache was available. (prebid#557) Fixed the validation logic on unbounded caches, and added tests. (prebid#556) Include binaries in releases (prebid#554) Defines defaults for all config options and removes pointers from the… (prebid#539) ...
* Changes configuration logging to be reflect-based. * Impliments various small improvements for clarity. * Makes LogGeneral() private
* Changes configuration logging to be reflect-based. * Impliments various small improvements for clarity. * Makes LogGeneral() private
* Changes configuration logging to be reflect-based. * Impliments various small improvements for clarity. * Makes LogGeneral() private
Fixes #561