Add support for json logging to file #4523
Conversation
|
I like the approach, but note that we default to logging to syslog. If we decide to recommend people to use the file logger, we'll need some improvements to it (e.g. reopen files on signal, to support external rotating strategies). |
|
Should we enable json logging also for syslog? Should we make it apply to all logging methods? |
ruflin
force-pushed
the
json-logging
branch
2 times, most recently
from
1d702f9
to
f473e55
Compare
|
You'll need a rebase on this, but you did it to yourself :) |
libbeat/logp/log.go
Outdated
| if _log.JSON { | ||
| _log.syslog[level].Output(calldepth, string(bytes)) | ||
| } else { | ||
| _log.syslog[level].Output(calldepth, fmt.Sprintf("%s", message)) |
There was a problem hiding this comment.
The Sprintf here seems superfluous. string(bytes)?
There was a problem hiding this comment.
agree, was probably there because there was more before the refactoring ...
| } else { | ||
| // Makes sure all prefixes have the same length | ||
| prefix = prefix + strings.Repeat(" ", 4-len(prefix)) | ||
| bytes = []byte(fmt.Sprintf("%s %s %s", timestamp, prefix, message)) |
There was a problem hiding this comment.
I wonder if the Sprintf, which is basically a Join, could be implemented more efficient. A benchmark between this and the strings.Join version would be interesting.
If it can be optimized, it's probably worth it so that we don't affect the performance in the debug logging case.
There was a problem hiding this comment.
@urso probably knows from the top of his head? :-D
ruflin
force-pushed
the
json-logging
branch
2 times, most recently
from
9186fc0
to
2ab1353
Compare
CHANGELOG.asciidoc
Outdated
| @@ -60,6 +60,8 @@ https://github.com/elastic/beats/compare/v6.0.0-alpha2...master[Check the HEAD d | |||
| - New cli subcommands interface. {pull}4420[4420] | |||
| - Allow source path matching in `add_docker_metadata` processor. {pull}4495[4495] | |||
| - Add support for analyzers and multifields in fields.yml. {pull}4574[4574] | |||
| - Allow source path matching in `add_docker_metadata` processor {pull}4495[4495] | |||
There was a problem hiding this comment.
A changlog entry for add_docker_metadata, is that a merge problem?
The current support is very minimal and only logs the existing log structure in json instead of plain text. In the future further support for being able to log structs and more complex events could be added. JSON logging is disabled by default.
|
Merged in the current form. |
|
Removing "needs_docs" label because the docs are added in #4931 |
The current support is very minimal and only logs the existing log structure in json instead of plain text. In the future further support for being able to log structs and more complex events could be added.
JSON logging is disabled by default and only available for file logging.
Changelog and docs are still missing. I want to discuss first if we should move forward with this.