-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Ingest Manager] Use libbeat logger #19211
Conversation
Pinging @elastic/ingest-management (Team:Ingest Management) |
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
type loggingLevel logp.Level | ||
|
||
var loggingLevelMap = map[string]loggingLevel{ | ||
"trace": loggingLevel(logp.DebugLevel), // For backward compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trace was only for us? correct? we can remove that.
# Sets output of logging. Available options are `console`, `file`. | ||
# File output is located at `data/logs/elastic-agent` | ||
# logging.output: console | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reference file should be more complete https://github.com/elastic/beats/blob/master/libbeat/_meta/config/logging.reference.yml.tmpl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what i dont like in to_xxx
configuration options is that it evokes that you can set true to more than one and it will work, having a single option with a single value makes it clear there's no way of having 2 outputs of logging.
i will introduce some more options or i can even make it 1;1 and let libbeat logger handle that without any verification from our side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not like it either but I am into moving responsibility out if possible, @urso is there a plan to change the logging options?
@michalpristas Few changes I see.
|
JSON bool `config:"json"` // Write logs as JSON. | ||
Level Level `config:"level"` // Logging level (error, warning, info, debug). | ||
Selectors []string `config:"selectors"` // Selectors for debug level logging. | ||
ECSEnabled bool `config:"ecs"` // Adds minimal ECS information using ECS conformant keys to every log line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this on by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ECS is on by default here https://github.com/elastic/beats/pull/19211/files#diff-7b5f5c8d014c26b7f6c249cb83fb54b9R101
[Ingest Manager] Use libbeat logger (elastic#19211)
[Ingest Manager] Use libbeat logger (elastic#19211)
What does this PR do?
Changes our dependency on ecs logger and uses libbeat logger by default.
By default we log to file now, so users can share logs easier.
Why is it important?
ECS compliant logging.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Fixes: #15129