Deep merge event fields and metadata maps #17958
Merged
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
ycombinator
added
bug
Team:Services
|
Pinging @elastic/integrations-services (Team:Services) |
ycombinator
added
needs_backport
kvch
reviewed
Apr 24, 2020
|
|
||
| // We have accounted for @timestamp, @metadata, type above. So let's remove these keys and | ||
| // deep update the event with the rest of the keys | ||
| delete(keys, "@timestamp") |
There was a problem hiding this comment.
Nit: How about using a for loop instead of calling delete three times?
for _, name := range []string{"@timestamp", "@metadata", "type"} {
delete(keys, name)
}There was a problem hiding this comment.
Addressed in 4f1ee99ec.
kvch
approved these changes
Apr 24, 2020
urso
reviewed
Apr 24, 2020
| @@ -77,6 +77,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d | |||
| - Do not rotate log files on startup when interval is configured and rotateonstartup is disabled. {pull}17613[17613] | |||
| - Fix goroutine leak and Elasticsearch output file descriptor leak when output reloading is in use. {issue}10491[10491] {pull}17381[17381] | |||
| - Fix `setup.dashboards.index` setting not working. {pull}17749[17749] | |||
| - Arbitrary fields and metadata maps are now deep merged into event. {pull}17958[17958] | |||
There was a problem hiding this comment.
nit; this PR looks it is improving/fixing some issue with the JSON parser (most likely json decoder processor only). Reading this changelog makes me think it is a generic bug in Beats :)
There was a problem hiding this comment.
Updated CHANGELOG in d27bb07.
ycombinator
force-pushed
the
lb-jsonhelper-writejsonkeys
branch
2 times, most recently
from
d27bb07
to
cbb13b3
Compare
urso
reviewed
Apr 27, 2020
| @@ -200,6 +200,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d | |||
| - Add optional regex based cid extractor to `add_kubernetes_metadata` processor. {pull}17360[17360] | |||
| - Add `urldecode` processor to for decoding URL-encoded fields. {pull}17505[17505] | |||
| - Add support for AWS IAM `role_arn` in credentials config. {pull}17658[17658] {issue}12464[12464] | |||
| - When using the `decode_json_fields` processor, decoded fields are now deep-merged into existing event. {pull}17958[17958] | |||
There was a problem hiding this comment.
How about: Change decode_json_fields to merge parsed json objects with existing objects in the event instead of fully replacing them..
Would it make sense to treat this as a bug and add the changelog note to the bugfix section?
There was a problem hiding this comment.
Done in b7304af16.
ycombinator
force-pushed
the
lb-jsonhelper-writejsonkeys
branch
from
cbb13b3
to
b7304af
Compare
urso
reviewed
Apr 28, 2020
| @@ -275,6 +276,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d | |||
| - Improve ECS categorization field mappings in postgresql module. {issue}16177[16177] {pull}17914[17914] | |||
| - Improve ECS categorization field mappings in rabbitmq module. {issue}16178[16178] {pull}17916[17916] | |||
| - Improve ECS categorization field mappings in redis module. {issue}16179[16179] {pull}17918[17918] | |||
| - When using the `json.*` setting available on some inputs, decoded fields are now deep-merged into existing event. {pull}17958[17958] | |||
There was a problem hiding this comment.
let's adapt this changelog entry as well :)
There was a problem hiding this comment.
Done in 4b2cd165bc979fdaa7720f7d88d3a2ecd134d143.
urso
approved these changes
Apr 28, 2020
ycombinator
force-pushed
the
lb-jsonhelper-writejsonkeys
branch
2 times, most recently
from
4b2cd16
to
df10f8f
Compare
💔 Build Failed
Expand to view the summary
Build stats
Test stats 🧪
Test errorsExpand to view the tests failures
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
ycombinator
force-pushed
the
lb-jsonhelper-writejsonkeys
branch
from
df10f8f
to
2e10719
Compare
ycombinator
force-pushed
the
lb-jsonhelper-writejsonkeys
branch
from
2e10719
to
2a9821b
Compare
|
Jenkins CI failures are unrelated and Travis CI is green. Merging. |
This was referenced May 5, 2020
ycombinator
added a commit
that referenced
this pull request
May 5, 2020
* Deep merge event fields and metadata maps (#17958) * Deep merge event fields and metadata maps * Add CHANGELOG entry * Use loop to remove keys; extract into function * Relocating CHANGELOG entry * Rewording and moving to bugfix section per review feedback * Adapting other CHANGELOG entry * Fix comparison in test * Fixing up CHANGELOG * Cleaning up CHANGELOG * Fixing up CHANGELOG * Fixing up CHANGELOG
ycombinator
added a commit
that referenced
this pull request
May 5, 2020
* Deep merge event fields and metadata maps (#17958) * Deep merge event fields and metadata maps * Add CHANGELOG entry * Use loop to remove keys; extract into function * Relocating CHANGELOG entry * Rewording and moving to bugfix section per review feedback * Adapting other CHANGELOG entry * Fix comparison in test * Fixing up CHANGELOG * Fixing up CHANGELOG
leweafan
pushed a commit
to leweafan/beats
that referenced
this pull request
Apr 28, 2023
…tic#18231) * Deep merge event fields and metadata maps (elastic#17958) * Deep merge event fields and metadata maps * Add CHANGELOG entry * Use loop to remove keys; extract into function * Relocating CHANGELOG entry * Rewording and moving to bugfix section per review feedback * Adapting other CHANGELOG entry * Fix comparison in test * Fixing up CHANGELOG * Cleaning up CHANGELOG * Fixing up CHANGELOG * Fixing up CHANGELOG
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?
When merging an arbitrary map of metadata or fields into a
beat.Event, we deep merge theevent.Metaandevent.Fields, respectively, while still respecting theoverrideKeysboolean flag.Why is it important?
Before this PR were were shallow-merging
event.Metaandevent.Fields. As a result, when the Filebeatloginput was creatinglog.offsetandlog.file.pathfields, if the the event already contained alogfield, the event'slogfield would take complete precedence. Instead, what we want is for the event'slogfield and Filebeat'slogfield to be deep merged, while respecting the override boolean setting.Checklist
I have made corresponding changes to the documentationI have made corresponding change to the default configuration filesCHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.