-
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
Force ECS and JSON logging for libbeat/logp #28573
Conversation
Force the logp package to act as if ECS has been enabled. Attach the "service.name" field to entries only when Config.Beat is not empty. Remove the logp.Config.ECSEnabled field.
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
This pull request does not have a backport label. Could you fix it @michel-laterman? 🙏
NOTE: |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
@michel-laterman will this PR completely resolve #15544? |
when testing please check how many |
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.
@michel-laterman Any chance you could share some example logs from before and after the change? |
@ruflin, I've added log excerpts to the PR description. |
can you also include those events from before/after? |
@ruflin, it should be relatively straightforward to force JSON logging + disable logging metrics in this pr as well |
Agreed, I think that would be nice too. JSON+ECS logs seems non-contentious to me. I don't know who is relying on metrics in logs (there are undoubtedly folks who will rely on that), but I feel like defaulting to off is the right thing. |
@michel-laterman Is the example you provided above for 8.0 without Elastic Agent correct? I would have expected the logs of metricbeat 8.0 with and without elastic-agent to be the same. Lets move forward with ECS + JSON in 8.0 as the default and the only option? @axw This will directly impact apm-server as the config options would be removed. It means in 7.16 we should add a deprecation log message to give users a note in 7.16 to already switch over. For the metrics part, lets do it in a separate PR. |
This pull request is now in conflicts. Could you fix it? 🙏
|
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.
No concerns from my side to change to these defaults, as they are aligned with current apm defaults.
But I'm not certain about removing the logging.ecs
config option while keeping logging.json
. The ecs-logging-go-zap encoder logs everything in json format:
ECS loggers are formatter/encoder plugins for your favorite logging libraries. They make it easy to format your logs into ECS-compatible JSON.
Is there a reason why beats/apm shouldn't always be logging in json
format?
@@ -75,8 +75,3 @@ logging.files: | |||
|
|||
# Set to true to log messages in JSON format. | |||
#logging.json: false |
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.
Should be updated to true
then
@simitt You would remove both config options? I'm on board with this. Like this Beats has only 1 way left on how to write logs which I think is good. The only downside is that json logs are not always too friendly for human consumption but that is something we could solve with tooling. |
I'm not sure if the non-json logs are used/required for anything or not. My main point was that ecs without json doesn't make sense, so if non-json logs are required, removing the config option for ecs is not great; otherwise, yes, also remove the json config option. |
@ruflin yes, I'm on board with removing both. |
/test |
1 similar comment
/test |
/package |
…in-the-package-binareis * upstream/master: allows disable pod events enrichment with deployment name (elastic#28521) Remove Docker input from Filebeat (elastic#28817) [breaking] Make default_field: false the default for all fields (elastic#28596) Osquerybeat: Improve osquery client connect code (elastic#28848) Add crawler metrics into the stats metricset for Enterprise Search (elastic#28790) Remove the now deprecated appsearch module from metricbeat (elastic#28850) Remove Beat generators (elastic#28816) chore: upload files to Google Storage when they exist (elastic#28836) Revert "chore(ci): disable E2E tests in Beats (elastic#28715)" (elastic#28812) Deprecate generating custom Beats (elastic#28814) [Metricbeat] upgrade flatbuffers to 1.12.1 (elastic#28094) Osquerybeat: Fix restart flags after previously bad config (elastic#28827) Force ECS and JSON logging for libbeat/logp (elastic#28573) Filebeat: Error on startup for unconfigured module (elastic#28818) Deprecate log input in favour of filestream (elastic#28623) Fix some spelling mistakes (elastic#28080)
Force the logp package to act as if ECS and JSON has been enabled. Attach the service.name field to entries only when Config.Beat is not empty. Remove the logp.Config.ECSEnabled field. Log entries will now be JSON formatted by default. (cherry picked from commit 987250d)
Force the logp package to act as if ECS and JSON has been enabled. Attach the service.name field to entries only when Config.Beat is not empty. Remove the logp.Config.ECSEnabled field. Log entries will now be JSON formatted by default. (cherry picked from commit 987250d) Co-authored-by: Michel Laterman <[email protected]>
…in-the-package-binareis * upstream/master: allows disable pod events enrichment with deployment name (elastic#28521) Remove Docker input from Filebeat (elastic#28817) [breaking] Make default_field: false the default for all fields (elastic#28596) Osquerybeat: Improve osquery client connect code (elastic#28848) Add crawler metrics into the stats metricset for Enterprise Search (elastic#28790) Remove the now deprecated appsearch module from metricbeat (elastic#28850) Remove Beat generators (elastic#28816) chore: upload files to Google Storage when they exist (elastic#28836) Revert "chore(ci): disable E2E tests in Beats (elastic#28715)" (elastic#28812) Deprecate generating custom Beats (elastic#28814) [Metricbeat] upgrade flatbuffers to 1.12.1 (elastic#28094) Osquerybeat: Fix restart flags after previously bad config (elastic#28827) Force ECS and JSON logging for libbeat/logp (elastic#28573) Filebeat: Error on startup for unconfigured module (elastic#28818) Deprecate log input in favour of filestream (elastic#28623) Fix some spelling mistakes (elastic#28080)
What does this PR do?
Force the logp package to act as if ECS and JSON has been enabled. Attach the
service.name
field to entries only when Config.Beat is not empty.Remove the
logp.Config.ECSEnabled
field. Log entries will now beJSON formatted by default.
Why is it important?
All Elastic stack components will produce ECS conforming logs in 8.0 by default.
This change forces libbeat, and all components using libbeat to produce these logs (including all beats, and the elastic-agent.).
Checklist
I have commented my code, particularly in hard-to-understand areasI have added tests that prove my fix is effective or that my feature worksCHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.How to test this PR locally
Related issues
Logs
Example of logs from metricbeat
7.15.1
(before ECS is forced)Example of logs from metricbeat after ECS is forced on, and JSON is enabled by default an
8.0.0-SNAPSHOT
:Example of metricbeat running under elastic-agent (ECS forced) on an
8.0.0-SNAPSHOT
: