-
Notifications
You must be signed in to change notification settings - Fork 44
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
logp: don't write to files by default if running in a container environment #236
logp: don't write to files by default if running in a container environment #236
Conversation
ce685aa
to
0f7f948
Compare
logp/config.go
Outdated
toFiles := true | ||
|
||
// If running in a container environment, don't write to files by default. | ||
if environment == ContainerEnvironment { |
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 elastic-agent container always logs to files in the file system by default, even if logging to stdout+stderr is disabled. These are the logs that are included in diagnostics from the container.
Let's make sure we don't break agent diagnostics when this merges into elastic-agent.
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.
ACK. This is reverting to the previous behavior. When we silently made this change earlier, it didn’t break elastic-agent, nor was it broken before the change. I don’t believe it's affected at all, but I could be wrong.
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.
So looking at changes in elastic-agent, there is only two occurrences of logp.DefaultConfig
and both use logp.DefaultEnvironment
which always gets ToFiles = true
.
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 was thinking a bit more about it and I believe the problem is not just because we always set toFiles
to true
, it's also because DefaultConfig
sets an output, so when logp.createLogOutput
elastic-agent-libs/logp/core.go
Lines 250 to 272 in 196dace
func createLogOutput(cfg Config, enab zapcore.LevelEnabler) (zapcore.Core, error) { | |
switch { | |
case cfg.toIODiscard: | |
return makeDiscardOutput(cfg, enab) | |
case cfg.ToStderr: | |
return makeStderrOutput(cfg, enab) | |
case cfg.ToSyslog: | |
return makeSyslogOutput(cfg, enab) | |
case cfg.ToEventLog: | |
return makeEventLogOutput(cfg, enab) | |
case cfg.ToFiles: | |
return makeFileOutput(cfg, enab) | |
} | |
switch cfg.environment { | |
case SystemdEnvironment, ContainerEnvironment: | |
return makeStderrOutput(cfg, enab) | |
case MacOSServiceEnvironment, WindowsServiceEnvironment: | |
return makeFileOutput(cfg, enab) | |
default: | |
return zapcore.NewNopCore(), nil | |
} | |
} |
gets called, the environment is not taken into consideration because an output has already been explicitly chosen.
I believe the systemd case (Filebeat running as a service) is also affected. Installing from the deb
should trigger this case, I'm not sure if there is an option to "manually" install Filebeat as a service.
Could you also test the deb/systemd flow to make sure we fixed it in all cases?
Sure, I was able to build a deb package with |
@belimawr From my testing with systemd, filebeat creates /var/log/filebeat/*.ndjson files and |
💚 Build Succeeded
History
cc @mauri870 |
@belimawr This should be working properly now both for systemd and container environments. I tested the deb package of v8.15.0 and it does not log to syslog by default. I managed to package filebeat (main) with this fix included and it seems to log to syslog properly when running via systemd as well as docker, see attached logs. Please let me know if there is any additional concerns on your end.
|
Yes, I believe this is the correct behaviour: when running under systemd, the logs go to stderr and can be seen using journalctl. The best approach to be sure is to test an older version 😅 I also saw your last comment, the behaviour looks correct. |
// For container and systemd environments, we don't write to files by default. | ||
switch environment { | ||
case ContainerEnvironment, SystemdEnvironment: |
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 fact that we have this logic in two different places, bugs me... Because the FF for 8.15.3 was Tue, I'd get this PR in as it is and backport it if we still have time to make into the release. Then create an issue to re-visit it at some point in the future.
I believe that's the correct function to define the default behaviour, createLogOutput
should not need the environment switch.
Thanks, I did test with 8.14.3 and the behavior seems consistent with with filebeat main + this fix. |
What does this PR do?
The PR #208 introduced a bug that made the
-environment container|systemd
flag stop writting to stderr on the default logger. This PR restores the old behavior as well as adding a test that observes that logs are properly written to stderr.Why is it important?
Checklist
Related issues