Skip to content
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

Deep merge event fields and metadata maps #17958

Merged
merged 7 commits into from
May 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.next.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
- Fix Elasticsearch license endpoint URL referenced in error message. {issue}17880[17880] {pull}18030[18030]
- Fix panic when assigning a key to a `nil` value in an event. {pull}18143[18143]
- Gives monitoring reporter hosts, if configured, total precedence over corresponding output hosts. {issue}17937[17937] {pull}17991[17991]
- Arbitrary fields and metadata maps are now deep merged into event. {pull}17958[17958]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated CHANGELOG in d27bb07.

- Change `decode_json_fields` processor, to merge parsed json objects with existing objects in the event instead of fully replacing them. {pull}17958[17958]

*Auditbeat*

Expand Down Expand Up @@ -215,6 +217,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
- Add support for fixed length extraction in `dissect` processor. {pull}17191[17191]
- Set `agent.name` to the hostname by default. {issue}16377[16377] {pull}18000[18000]
- Add config example of how to skip the `add_host_metadata` processor when forwarding logs. {issue}13920[13920] {pull}18153[18153]
- When using the `decode_json_fields` processor, decoded fields are now deep-merged into existing event. {pull}17958[17958]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in b7304af16.


*Auditbeat*

Expand Down Expand Up @@ -294,6 +297,8 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
- Added an input option `publisher_pipeline.disable_host` to disable `host.name`
from being added to events by default. {pull}18159[18159]
- Improve ECS categorization field mappings in system module. {issue}16031[16031] {pull}18065[18065]
- When using the `json.*` setting available on some inputs, decoded fields are now deep-merged into existing event. {pull}17958[17958]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's adapt this changelog entry as well :)

Copy link
Contributor Author

@ycombinator ycombinator Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 4b2cd165bc979fdaa7720f7d88d3a2ecd134d143.

- Change the `json.*` input settings implementation to merge parsed json objects with existing objects in the event instead of fully replacing them. {pull}17958[17958]

*Heartbeat*

Expand Down
27 changes: 18 additions & 9 deletions libbeat/common/jsontransform/jsonhelper.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,12 @@ import (
// WriteJSONKeys writes the json keys to the given event based on the overwriteKeys option and the addErrKey
func WriteJSONKeys(event *beat.Event, keys map[string]interface{}, overwriteKeys bool, addErrKey bool) {
if !overwriteKeys {
for k, v := range keys {
if _, exists := event.Fields[k]; !exists && k != "@timestamp" && k != "@metadata" {
event.Fields[k] = v
}
}
// @timestamp and @metadata fields are root-level fields. We remove them so they
// don't become part of event.Fields.
removeKeys(keys, "@timestamp", "@metadata")

// Then, perform deep update without overwriting
event.Fields.DeepUpdateNoOverwrite(keys)
return
}

Expand Down Expand Up @@ -64,7 +65,7 @@ func WriteJSONKeys(event *beat.Event, keys map[string]interface{}, overwriteKeys
}

case map[string]interface{}:
event.Meta.Update(common.MapStr(m))
event.Meta.DeepUpdate(common.MapStr(m))

default:
event.SetErrorWithOption(createJSONError("failed to update @metadata"), addErrKey)
Expand All @@ -83,13 +84,21 @@ func WriteJSONKeys(event *beat.Event, keys map[string]interface{}, overwriteKeys
continue
}
event.Fields[k] = vstr

default:
event.Fields[k] = v
}
}

// 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.
removeKeys(keys, "@timestamp", "@metadata", "type")
event.Fields.DeepUpdate(keys)
}

func createJSONError(message string) common.MapStr {
return common.MapStr{"message": message, "type": "json"}
}

func removeKeys(keys map[string]interface{}, names ...string) {
for _, name := range names {
delete(keys, name)
}
}
136 changes: 136 additions & 0 deletions libbeat/common/jsontransform/jsonhelper_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
// Licensed to Elasticsearch B.V. under one or more contributor
// license agreements. See the NOTICE file distributed with
// this work for additional information regarding copyright
// ownership. Elasticsearch B.V. licenses this file to you under
// the Apache License, Version 2.0 (the "License"); you may
// not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

package jsontransform

import (
"testing"
"time"

"github.com/stretchr/testify/require"

"github.com/elastic/beats/v7/libbeat/beat"
"github.com/elastic/beats/v7/libbeat/common"
)

func TestWriteJSONKeys(t *testing.T) {
now := time.Now()
now = now.Round(time.Second)

eventTimestamp := time.Date(2020, 01, 01, 01, 01, 00, 0, time.UTC)
eventMetadata := common.MapStr{
"foo": "bar",
"baz": common.MapStr{
"qux": 17,
},
}
eventFields := common.MapStr{
"top_a": 23,
"top_b": common.MapStr{
"inner_c": "see",
"inner_d": "dee",
},
}

tests := map[string]struct {
keys map[string]interface{}
overwriteKeys bool
expectedMetadata common.MapStr
expectedTimestamp time.Time
expectedFields common.MapStr
}{
"overwrite_true": {
overwriteKeys: true,
keys: map[string]interface{}{
"@metadata": map[string]interface{}{
"foo": "NEW_bar",
"baz": map[string]interface{}{
"qux": "NEW_qux",
"durrr": "COMPLETELY_NEW",
},
},
"@timestamp": now.Format(time.RFC3339),
"top_b": map[string]interface{}{
"inner_d": "NEW_dee",
"inner_e": "COMPLETELY_NEW_e",
},
"top_c": "COMPLETELY_NEW_c",
},
expectedMetadata: common.MapStr{
"foo": "NEW_bar",
"baz": common.MapStr{
"qux": "NEW_qux",
"durrr": "COMPLETELY_NEW",
},
},
expectedTimestamp: now,
expectedFields: common.MapStr{
"top_a": 23,
"top_b": common.MapStr{
"inner_c": "see",
"inner_d": "NEW_dee",
"inner_e": "COMPLETELY_NEW_e",
},
"top_c": "COMPLETELY_NEW_c",
},
},
"overwrite_false": {
overwriteKeys: false,
keys: map[string]interface{}{
"@metadata": map[string]interface{}{
"foo": "NEW_bar",
"baz": map[string]interface{}{
"qux": "NEW_qux",
"durrr": "COMPLETELY_NEW",
},
},
"@timestamp": now.Format(time.RFC3339),
"top_b": map[string]interface{}{
"inner_d": "NEW_dee",
"inner_e": "COMPLETELY_NEW_e",
},
"top_c": "COMPLETELY_NEW_c",
},
expectedMetadata: eventMetadata.Clone(),
expectedTimestamp: eventTimestamp,
expectedFields: common.MapStr{
"top_a": 23,
"top_b": common.MapStr{
"inner_c": "see",
"inner_d": "dee",
"inner_e": "COMPLETELY_NEW_e",
},
"top_c": "COMPLETELY_NEW_c",
},
},
}

for name, test := range tests {
t.Run(name, func(t *testing.T) {
event := &beat.Event{
Timestamp: eventTimestamp,
Meta: eventMetadata.Clone(),
Fields: eventFields.Clone(),
}

WriteJSONKeys(event, test.keys, test.overwriteKeys, false)
require.Equal(t, test.expectedMetadata, event.Meta)
require.Equal(t, test.expectedTimestamp.UnixNano(), event.Timestamp.UnixNano())
require.Equal(t, test.expectedFields, event.Fields)
})
}
}