logging: Allow request logging config to determine log level #4956
Conversation
philipgough
force-pushed
the
query-log-impl
branch
2 times, most recently
from
a526e09
to
ae59973
Compare
This change allows the 'level' config in request logging 'options' to determine the output level for those logs. Prior to this, without setting --log.level flag on the component to debug, no logs were emitted. Signed-off-by: Philip Gough <philip.p.gough@gmail.com>
philipgough
force-pushed
the
query-log-impl
branch
from
ae59973
to
8183612
Compare
| @@ -24,17 +24,17 @@ type HTTPServerMiddleware struct { | |||
| } | |||
|
|
|||
| func (m *HTTPServerMiddleware) preCall(name string, start time.Time, r *http.Request) { | |||
| logger := m.opts.filterLog(m.logger) | |||
| level.Debug(logger).Log("http.start_time", start.String(), "http.method", fmt.Sprintf("%s %s", r.Method, r.URL), "http.request_id", r.Header.Get("X-Request-ID"), "thanos.method_name", name, "msg", "started call") | |||
| m.opts.levelledLogger(m.opts.filterLog(m.logger)).Log( | |||
There was a problem hiding this comment.
this logger could probably be configured once in the constructor?
There was a problem hiding this comment.
thinking about this some more I am also not sure that the filter is even needed considering we have setup a logger via cmd/main.go that is filtered based on --log.level and passed in to this middleware
There was a problem hiding this comment.
I think if we use the leveledLogger here to achieve the intended effect (i.e. log the requests on the configured level), filterLog should not be necessary as you said - we should have that governed by the global flag.
There was a problem hiding this comment.
For sure, totally agree - maybe @bwplotka could chime in and let us know if there were "bigger plans" for the logging package or do we really need to expose these functional options at all? Maybe we are missing some context
|
|
||
| logger = m.opts.filterLog(logger) | ||
| m.opts.levelFunc(logger, status).Log("msg", "finished call") | ||
| m.opts.levelledLogger(m.opts.filterLog(m.logger)).Log( |
| @@ -69,6 +70,12 @@ func WithFilter(f FilterLogging) Option { | |||
| } | |||
| } | |||
|
|
|||
| func WithLevelledLogger(f LevelledLogging) Option { | |||
There was a problem hiding this comment.
I'm wondering what is actually the reason to have WithLeveledLogger and WithFilter to be functional options and exported? It's not like we support users to provide their own leveled logger or filter, or do we?
There was a problem hiding this comment.
I agree, I am not sure that the functional options are required here. However I am not sure if there were other plans for the logging package to be used elsewhere or what the initial driver for that code was. It does seem excessive for the way the package is currently used.
To answer your question in regards to this LevelledLogger - the input in the config determines the level to log at, which is why I am using this currently. I believe it could all be done in the constructor as I had commented previously
| @@ -125,6 +136,21 @@ func DefaultCodeToLevel(logger log.Logger, code int) log.Logger { | |||
| return level.Error(logger) | |||
| } | |||
|
|
|||
| func DefaultLevelledLogger(logger log.Logger, lvl string) log.Logger { | |||
There was a problem hiding this comment.
Should we allow only Info and Debug in here as you mentioned in the PR description? Or perhaps we could validate level beforehand and only accept those two levels?
There was a problem hiding this comment.
See #4956 (comment) but yes, or we could restrict the request logging config to only those two options while keeping the package more generic.
I didn't want to overhaul to much here without feedback from maintainers first :)
| @@ -24,17 +24,17 @@ type HTTPServerMiddleware struct { | |||
| } | |||
|
|
|||
| func (m *HTTPServerMiddleware) preCall(name string, start time.Time, r *http.Request) { | |||
| logger := m.opts.filterLog(m.logger) | |||
| level.Debug(logger).Log("http.start_time", start.String(), "http.method", fmt.Sprintf("%s %s", r.Method, r.URL), "http.request_id", r.Header.Get("X-Request-ID"), "thanos.method_name", name, "msg", "started call") | |||
| m.opts.levelledLogger(m.opts.filterLog(m.logger)).Log( | |||
There was a problem hiding this comment.
I think if we use the leveledLogger here to achieve the intended effect (i.e. log the requests on the configured level), filterLog should not be necessary as you said - we should have that governed by the global flag.
Perhaps I am misunderstanding the original intentions, but the request logging configuration seems overly complicated.
I started working on documenting the config in #4934 and there are some comments there, but overall I find the config is unclear.
This PR changes the implementation slightly, since my take is that if I have gone to the trouble of configuring request logging, all I want to do is specify at what level I believe that should be output and have that determined by the
--log.levelThis change allows the 'level' config in request logging 'options'
to determine the output level for those logs.
Prior to this, without setting --log.level flag on the component
to debug, no logs were emitted.
With this implementation, I think it would make sense to restrict the logging options to one of
debug,infoIf this approach seems reasonable to the maintainers we would need to tackle gRPC in similar fashion.
Changes
Verification