feat(slog): add support for structured and leveled logger#1756
feat(slog): add support for structured and leveled logger#1756
Conversation
1. Uses standard lib's log/slog as a main logger. 2. Deprecates the old way of logging via Debug flag and DebugF 3. Provide adapter for legacy way of logging with no-op logging 4. Backward compatible with Debug and DebugF apporach of logging Signed-off-by: Kaviraj <[email protected]>
1. No more un-structured `debug` calls 2. Examples on how to use proper structured logging on both transport Signed-off-by: Kaviraj <[email protected]>
Signed-off-by: Kaviraj <[email protected]>
Signed-off-by: Kaviraj <[email protected]>
Signed-off-by: Kaviraj <[email protected]>
| @@ -0,0 +1,125 @@ | |||
| package clickhouse | |||
There was a problem hiding this comment.
Main logger implementation.
| isReleased() bool | ||
| setReleased(released bool) | ||
| debugf(format string, v ...any) | ||
| getLogger() *slog.Logger |
There was a problem hiding this comment.
This is needed to provide connection contexts to connection polling. Without this, the connection pooling doesn't know what's the connection-id, remote-addr, etc. and logs will be very generic like "acquired connection", "releasing connection"
| // | ||
| // For backward compatibility, if Debug=true and Debugf is set, those will be used | ||
| // instead of Logger. | ||
| Logger *slog.Logger |
There was a problem hiding this comment.
This is the main UX from the user POV. That accepts logger as separate dependency.
| // 1. If Debug=true and Debugf is set, use legacy Debugf (backward compatibility) | ||
| // 2. If Logger is set, use the provided logger | ||
| // 3. Otherwise, use a noop logger (no logging) | ||
| func (o *Options) logger() *slog.Logger { |
There was a problem hiding this comment.
Provides "sane" default values for logger. Also take into the legacy Debug flag way of doing logging.
clickhouse.go
Outdated
| return nil, err | ||
| } | ||
| conn.debugf("[query] \"%s\"", query) | ||
| conn.getLogger().Info("executing query", slog.String("sql", query)) |
There was a problem hiding this comment.
- there is a high security risk when logging SQL statements - they may contain sensitive information (SSN, credit card number etc). Usually this is done on debug/trace level only and by dedicated logger (ex.:
clickhouse.client.sql_logger) - Logging with info level is too annoying in most case and can be quite expensive for customers. SQL in this case can be quite large so it will be a problem. Previously it was on debug level. I'd put it on trace level because client seems do very little with SQL itself .
There is more useful bit of information that can be logged- queryID
There was a problem hiding this comment.
Agree. logging query shouldn't be info. I'm putting back to Debug (trace level is not a norm in Go).
because client seems do very little with SQL itself .
There is more useful bit of information that can be logged- queryID
Sometimes I would like to see the full SQL (in Debug of course) with all the prepare statement get expanded and format applied. Just the actual query sent to the server. Also unfortunately we don't have access to queryID at this level.
clickhouse_std.go
Outdated
| } | ||
| if err := s.batch.Append(values...); err != nil { | ||
| s.debugf("[batch][exec] append error: %v", err) | ||
| s.logger.Error("batch append error", slog.Any("error", err)) |
There was a problem hiding this comment.
is it really required to log error on this level?
User should be able to log it if needed - anyway error is returned as result.
I'd remove logging here completely or leave it on debug level.
There was a problem hiding this comment.
I think removing is better. 👍 thanks
|
I suggest keep log level as before (ex.: previously errors were logged on debug level) |
Signed-off-by: Kaviraj <[email protected]>
2 participants
Summary
closes: #1698
Notes (for the reviewers):
FAQ:
Loggerinterface - slog is from standard library, and has all the helpers and log levels already.Checklist
Delete items not relevant to your PR: