Add more ECS fields to logs #25998
Merged
Add more ECS fields to logs #25998
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
botelastic
bot
added
the
needs_team
botelastic
bot
removed
the
needs_team
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪Test errors
Expand to view the tests failures
|
| Test | Results |
|---|---|
| Failed | 1 |
| Passed | 47332 |
| Skipped | 5244 |
| Total | 52577 |
Genuine test errors 
💔 There are test failures but not known flaky tests, most likely a genuine test failure.
- Name:
Extended / heartbeat-windows-7-32-windows-7-32-bit / TestQueueRunsInOrder – github.com/elastic/beats/v7/heartbeat/scheduler/timerqueue
axw
requested changes
May 31, 2021
libbeat/logp/core.go
Outdated
| @@ -196,6 +196,13 @@ func makeOptions(cfg Config) []zap.Option { | |||
| if cfg.development { | |||
| options = append(options, zap.Development()) | |||
| } | |||
| if cfg.ECSEnabled { | |||
| fields := []zap.Field{zap.String("service.name", cfg.Beat)} | |||
| if cfg.Files.Name != "" { | |||
There was a problem hiding this comment.
I see why this works, but it looks wrong to me since we should add event.dataset for non-file logging output. How about we introduce a new function in this package like
// logFilename returns the base filename to which logs will be written for
// the "files" log output. If another log output is used, or `logging.files.name`
// is unspecified, then the beat name will be returned.
func logFilename(cfg Config) string {
name := cfg.Beat
if cfg.Files.Name != "" {
name = cfg.Files.Name
}
return name
}Then you can use that here and in makeFileOutput, and revert the configure.Logging.
There was a problem hiding this comment.
we should add event.dataset for non-file logging output
I don't get that. If the output is eg. stderr what should be the value of event.dataset?
There was a problem hiding this comment.
See #25997 and https://github.com/elastic/ecs-logging/blob/9507c12f7523741cc06f84a2686d580e50f3465f/spec/spec.json#L79-L95
If the user manually configures the service name, the logging library should set
event.dataset=${service.name}.logif not explicitly configured otherwise.
axw
approved these changes
Jun 1, 2021
+1 There are unfortunately not many tests on logp, but as we start to add fields dynamically on user config it would be nice to introduce tests that check expected fields to be set. |
|
jenkins retest this please |
|
@urso any idea how can i merge this? the metricbeat-windows-10 tests have been stuck for +10 hours 2 times... or do you think is related to these changes? |
|
jenkins retest this please |
|
jenkins retest this please |
|
@urso this failed 6 times with 5 different flaky tests :/ |
Sorry, no. We never manually push to master or other release branches. Teams regularly working on Beats can still overwrite the checks in the UI and merge if there is need. The heartbeat one that failed is new. I'm not aware of it failing and the flaky test checker doesn't seem to know about the failed test either. Maybe some really bad timing or infra problems with windows 7, or a genuine back introduced lately... Regarding Heartbeat there is no real change besides Beats moving to 1.16. But during the migration to 1.16 we already have had to skip some other tests in metricbeat and heartbeat. See #25859 I'm okay to merge this PR for you, but can you please create an issue with the stack trace and skip the test that fails. Maybe @andrewvc might need to have a look. |
urso
approved these changes
Jun 4, 2021
|
Merged. The backport PR will be assigned to you. Feel free to ping me if you have any problems with the backport. |
mergify bot
pushed a commit
that referenced
this pull request
Jun 11, 2021
(cherry picked from commit bfbadaa)
urso
pushed a commit
that referenced
this pull request
Jun 16, 2021
(cherry picked from commit bfbadaa) Co-authored-by: Juan Álvarez <juan.alvarez@elastic.co>
mdelapenya
added a commit
to mdelapenya/beats
that referenced
this pull request
Jun 21, 2021
* master: (26 commits) Report total and free CPU for vSphere virtual machines (elastic#26167) [filebeat] Add preserve_original_event option to o365audit input (elastic#26273) Change xml processor names in script processor to match convention (elastic#26263) [Oracle] Fixing default values for paths in config template (elastic#26276) Add more ECS fields to logs (elastic#25998) [Heartbeat] Fix broken invocation of synth package (elastic#26228) rename sqs file name (elastic#26227) Populate the agent action result if there is no matching action handlers (elastic#26152) Add ISO8601 as supported timestamp type (elastic#25564) Move Filebeat azure module to GA (elastic#26168) Filebeat azure module pipeline fixes and changes (elastic#26148) libbeat: monitor version (elastic#26214) Add new parser to filestream input: container (elastic#26115) [Metricbeat] Add state_statefulset replicas.ready (elastic#26088) Disable test processors system test for windows 10 (elastic#26216) Fix startup with failing configuration (elastic#26126) Remove 32 bits version of Elastic Agent. (elastic#25708) Chane fleetmode detection to ony use management.enabled (elastic#26180) Make `filestream` input GA (elastic#26127) libbeat/idxmgmt/ilm: fix alias creation (elastic#26146) ...
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds service.name and event.dataset fields to ECS logs
Checklist
- [ ] I have commented my code, particularly in hard-to-understand areas- [ ] I have made corresponding changes to the documentation- [ ] I have made corresponding change to the default configuration files- [ ] I have added tests that prove my fix is effective or that my feature works- [ ] I have added an entry inCHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.How to test this PR locally
Start a beat with default settings, both with and without
-e, and see the implemented fields in the logs.Related issues
Closes #25997