feat: add query-log-path configuration
#25710
Conversation
This PR adds the ability for an admin to define 'query-log-path' as a configuration item field.
A new configuration option is created
```toml
[data]
query-log-path = "/var/influx/query.log"
```
This will enable query logging.
Logged queries example:
```
{"level":"info","ts":1735248393.084461,"msg":"Executing query","query":"SHOW DATABASES"}
{"level":"info","ts":1735248395.092188,"msg":"Executing query","query":"SHOW DATABASES"}
{"level":"info","ts":1735248398.58039,"msg":"Executing query","query":"SELECT * FROM stress.autogen.m0 LIMIT 20"}
```
devanbenz
force-pushed
the
db/459/query-loggin
branch
from
a440193 to
410241e
Compare
| } | ||
|
|
||
| // Test to ensure that watcher creates new file on file rename | ||
| func TestQueryExecutor_WriteQueryToLog_WatcherRemoveFile(t *testing.T) { |
There was a problem hiding this comment.
It appears that CI doesn't like these tests where on my local machine they pass no problem. I may need to adjust how I'm doing this 🤔
| @@ -66,6 +66,7 @@ | |||
| # Whether queries should be logged before execution. Very useful for troubleshooting, but will | |||
| # log any sensitive data contained within a query. | |||
| # query-log-enabled = true | |||
| # query-log-path = "/var/log/influxdb/query.log" | |||
There was a problem hiding this comment.
Shouldn't the default be the empty string?
query/executor.go
Outdated
| func initQueryLogWriter(log *zap.Logger, e *Executor, path string) (*os.File, error) { | ||
| logFile, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR|os.O_APPEND, 0644) | ||
| if err != nil { | ||
| e.Logger.Error("failed to open log file", zap.Error(err)) |
There was a problem hiding this comment.
fmt.Errorf to put the file path into the error. It can help to pinpoint the problem
| encoderConfig := zap.NewProductionEncoderConfig() | ||
|
|
||
| fileCore := zapcore.NewCore( | ||
| zapcore.NewJSONEncoder(encoderConfig), |
There was a problem hiding this comment.
Should we respect the configuration setting for log format?
query/executor.go
Outdated
|
|
||
| file, err = initQueryLogWriter(log, e, path) | ||
| if err != nil { | ||
| e.Logger.Error("failed to open log file", zap.Error(err)) |
There was a problem hiding this comment.
Isn't this error logged in initQueryLogWriter?
query/executor.go
Outdated
| closeQueryLogWriter(file.Fd(), e) | ||
| file, err = initQueryLogWriter(log, e, path) | ||
| if err != nil { | ||
| e.Logger.Error("failed to create log file watcher", zap.Error(err)) |
query/executor.go
Outdated
| } | ||
|
|
||
| func (e *Executor) WithLogWriter(log *zap.Logger, path string) { | ||
| var file *os.File |
There was a problem hiding this comment.
Should the closeQueryLogWriter be in a defer call? There's only one open at a time.
query/executor.go
Outdated
| defer watcher.Close() | ||
|
|
||
| for { | ||
| select { |
There was a problem hiding this comment.
I think we need an exit case here; a context that is cancelled on influxd shutdown, or some similar mechanism that closes and cleans up the watcher and the log.
There was a problem hiding this comment.
Otherwise we may not get the Sync() and Close() on the file, I believe.
query/executor.go
Outdated
| return logFile, nil | ||
| } | ||
|
|
||
| func closeQueryLogWriter(fd uintptr, e *Executor) { |
There was a problem hiding this comment.
I wouldn't operate on the fd here. Stick with the higher level libraries, like os.File.Sync and os.File.Close
There was a problem hiding this comment.
But I might call our utility function SyncDir on the containing directory to ensure the log files are saved in case of bad crashes.
query/executor.go
Outdated
|
|
||
| err = watcher.Add(file.Name()) | ||
| if err != nil { | ||
| e.Logger.Error("failed to watch log file", zap.Error(err)) |
There was a problem hiding this comment.
Maybe add file name to error message?
This PR adds the ability for an admin to define 'query-log-path' as a configuration item field.
A new configuration option is created
This will enable query logging.
Logged queries example: