json_decode_fields processor #2605

suraj-soni · 2016-09-20T21:49:18Z

This pull request captures a feature to enable/disable nested json parsing in filebeat. It's picked up from where the conversations were left off in #2435.

The commit includes :

decode_json_fields processor - initial commit
pulled common json transform code into libbeat/common/json_transform
refactored filebeat/harvester/reader/json.go to take advantage for 2

elasticsearch-release · 2016-09-20T21:52:02Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

elasticsearch-release · 2016-09-20T21:52:04Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

tsg · 2016-10-05T21:23:58Z

@suraj-soni Thanks a lot for the PR!

I'd like to ask for a system test that implements the use case that you have (e.g. log file + configuration).

Also not sure why the PR seems to contain 110 commits, when the diff is actually small, but we can figure this out later.

suraj-soni · 2016-10-07T04:52:44Z

@tsg : For some reason git wouldn't allow me to check in without doing a rebase on my branch. The 110 commits are mostly just rolling forward to the then commit on master branch.

I'll share the log file and configuration used for testing the fix soon.

suraj-soni · 2016-10-16T16:44:31Z

json_harvester_test.zip
@tsg : I've uploaded a sample yml config and a log file for running the test.

andrewkroh · 2016-10-17T14:33:23Z

libbeat/common/mapstr.go

@@ -177,6 +177,25 @@ func (m MapStr) GetValue(key string) (interface{}, error) {
 	return mapp[keyParts[keyPartsLen-1]], nil
 }

+func (m MapStr) AddValue(value MapStr, field string) error {


Can you use the MapStr.Put() method that was recently added?

andrewkroh · 2016-10-17T14:36:58Z

libbeat/processors/actions/json_decode_fields.go

+}
+
+func init() {
+	processors.RegisterPlugin("json_decode_fields",


Naming-wise I'd make it decode_json_fields. The other processors put the verb first.

Makes sense. Will make that change.

andrewkroh · 2016-10-17T14:38:55Z

libbeat/processors/actions/json_decode_fields.go

+
+		data, err := event.GetValue(field)
+		if err != nil {
+			return event, fmt.Errorf("no %s field found: %s", field, err)


Should the processor short-circuit on error or save the error and continue processing the remaining fields? The other processors save the error, continue, then when done they return all the errors that occurred.

I looked at the drop_field processor and looks like it takes the same approach as well.
err := event.Delete(field) if err != nil { return event, fmt.Errorf("Fail to delete key %s: %s", field, err) }

mapstr.GetValue() returns an error when it doesn't find a key. I'm hesitant to use a hasKey + getValue since it will involve parsing the data twice.

Any suggestions on how to fix this ?

You must be looking at a stale copy of the drop_fields.go. Here's the latest version that puts the errors into a slice and continues.

mapstr.GetValue() returns an error when it doesn't find a key. I'm hesitant to use a hasKey + getValue since it will involve parsing the data twice. Any suggestions on how to fix this ?

If you want to ignore errors caused by the key being absent, you can check if the cause is an common.ErrKeyNotFound like the include_fields action does.

suraj-soni · 2016-10-26T21:30:16Z

@andrewkroh : Have incorporated the changes that you suggested. However the build fails with an error :
filebeat/filebeat.full.yml: needs update make: *** [check] Error 1

Not sure how to fix this since I've updated the filebeat.full.yml to include an example of the new processor.

andrewkroh · 2016-10-26T21:33:35Z

filebeat/filebeat.full.yml is a generated file. Make the changes to filebeat/etc/beat.full.yml and then run make update in the filebeat directory.

Also I think you need to rebase your changes. There should only be 1 commit in the PR.

suraj-soni · 2016-10-27T08:09:16Z

@andrewkroh :
Was able to squash all but two commits, not sure how to merge those two.

tsg · 2016-10-27T10:17:26Z

jenkins, test it

ruflin · 2016-10-28T13:42:23Z

CHANGELOG.asciidoc

@@ -9,6 +9,7 @@

 === Beats version HEAD
 https://github.com/elastic/beats/compare/v5.0.0-rc1...master[Check the HEAD diff]
+https://github.com/elastic/beats/compare/v5.0.0-beta1...master[Check the HEAD diff]


That should be removed (merge error probably)

andrewkroh

Can you please add tests for this new processor? I'd like to have tests for a missing key, field value is not a string, JSON is invalid, and JSON is valid.

andrewkroh · 2016-10-28T13:47:34Z

libbeat/processors/actions/decode_json_fields.go

+
+		data, err := event.GetValue(field)
+		if err != nil && errors.Cause(err) != common.ErrKeyNotFound {
+			errs = append(errs, err.Error())


This should continue the for loop after the error.

andrewkroh · 2016-10-28T13:49:57Z

libbeat/processors/actions/decode_json_fields.go

+			err := unmarshal([]byte(text), &output)
+			if err != nil {
+				errs = append(errs, err.Error())
+				event.Put(field, text)


Why Put the value into the event on error? The value is already associated with the key in the event.

andrewkroh · 2016-10-28T13:51:40Z

libbeat/processors/actions/decode_json_fields.go

+}
+
+func (f decodeJSONFields) Run(event common.MapStr) (common.MapStr, error) {
+	errs := []string{}


Use var errs []string. https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices

andrewkroh · 2016-10-28T13:55:29Z

libbeat/processors/actions/decode_json_fields.go

+	errs := []string{}
+
+	for _, field := range f.Fields {
+		var output map[string]interface{}


This doesn't need to be declared until you are actually going to use it. So move it inside of the if ok { unmarshal ... block.

andrewkroh · 2016-10-28T13:57:13Z

libbeat/processors/actions/decode_json_fields.go

+				continue
+			}
+
+			_, addErr := event.Put(field, output)


You can reuse err here. _, err = event.Put(field, output).

andrewkroh · 2016-10-28T13:59:17Z

libbeat/processors/actions/decode_json_fields.go

+
+			_, addErr := event.Put(field, output)
+			if addErr != nil {
+				errs = append(errs, addErr.Error())


To be consistent and prevent future bugs this should also continue after the error. It doesn't really matter in this case, but if in the future some logic gets added after the location where the error occurred it could cause a bug.

suraj-soni · 2016-10-31T21:09:50Z

@andrewkroh ,

Thank you for your feedback. Have made the suggested changes and added test cases as well.

andrewkroh

Thanks for adding the test cases. Can you please fix the CHANGELOG then we can get this merged.

andrewkroh · 2016-10-31T21:46:08Z

libbeat/processors/actions/decode_json_fields_test.go

+
+func TestInvalidJSON(t *testing.T) {
+	input := common.MapStr{
+		"msg":      "{\"log\":\"{\\\"level\\\":\\\"info\\\"}\",\"stream\":\"stderr\",\"count\":3",


If you create a raw string literal by surrounding the string with back ticks (`) then you don't need to use escaping on those quotes. I don't care if you fix this, it's more of a golang tip to keep you sane.

andrewkroh · 2016-10-31T21:50:14Z

libbeat/processors/actions/decode_json_fields_test.go

+		"pipeline": "us1",
+	}
+
+	actual := getActualValue(t, config, input)


These test cases that expect an error should also assert that an error occurred. You can use assert.Error().

andrewkroh · 2016-10-31T21:54:50Z

CHANGELOG.asciidoc

@@ -8,6 +8,7 @@
 // Template, add newest changes here

 === Beats version HEAD
+https://github.com/elastic/beats/compare/v5.0.0-rc1...master[Check the HEAD diff]


It looks like you made no changes to this file, so can you reset this file so that there are no changes in the diff.

Or better yet, reset it and then add a line in the Added / Affecting all Beats section that contains something like:

- Added decode_json_fields processor for decoding fields containing JSON strings. {pull}2605[2605]

suraj-soni · 2016-10-31T23:25:23Z

@andrewkroh Updated the changelog.asciidoc file to incorporate your suggestions.

urso · 2016-11-02T15:53:28Z

libbeat/common/json_transform.go

@@ -0,0 +1,46 @@
+package common


this file moves helpers and exposes the json specific helpers with generic names. I'd rather see some json parsing utility function/module doing the right thing instead of exposing internal helpers.

@urso : Apologies for missing to see this comment.

Does wrapping it into something like this make sense?

package libbeat/common/jsontransform func TransformNumbers(dict MapStr) //exposed to consume func transformValue(value json.Number) //not exposed func transformArray(arr []interface{}) //not exposed

monicasarbu · 2016-11-09T14:59:51Z

@suraj-soni Can you please address the most recent comments, before we merge the PR?

urso · 2016-11-11T18:25:23Z

Sorry for having you wait, but I wanted to give this feature a little more thought. See my comments.

urso · 2016-11-11T18:09:39Z

libbeat/processors/actions/decode_json_fields.go

+		}
+		text, ok := data.(string)
+		if ok {
+			var output map[string]interface{}


we want to restrict ourselves to json objects only here? Using var output interface{} we can decode booleans, number, objects and arrays. Any valid json.

urso · 2016-11-11T18:15:08Z

libbeat/processors/actions/decode_json_fields.go

+
+	jsontransform.TransformNumbers(*fields)
+	return nil
+}


thinking a little more about TransformNumbers and unmarshal, I wonder why wen need to put TransformNumbers at the end. By moving TransformNumber before the deep-parsing stuff, the initial decoding would be exactly similar to the filebeat reader.

func DecodeJson(text []byte, to interface{}) { dec := json.NewDecoder(bytes.NewReader(text)) dec.UseNumber() err := dec.Decode(to) if err != nil { if m, ok := to.(*map[string]interface{}); ok { jsontransform.TransformNumbers(common.MapStr(m)) } return err }

Then (extending unmarshal a little), unmarshal can be generalized a little:

func unmarshal(maxDepth int, text []byte, fields interface{}, processArray bool) error { if err := DecodeJson(text, fields); err != nil { return err } maxDepth-- if maxDepth == 0 { return nil } tryUnmarshal := func(v interface{}) (interface{}, bool) { str, isString := v.(string) if !isString { return v, false } var tmp interface{} err := unmarshal(maxDepth, []byte(str), &tmp, processArray) if err != nil { return v, false } return tmp, true } // try to deep unmarshal fields switch O := fields.(type); { case *map[string]interface{}: for k, v := range O { if decoded, ok := tryUnmarshal(v); ok { O[k] = decoded } } // We want to process arrays here case []interface{}: if !processArray { break } for i, v := range O { if decoded, ok := tryUnmarshal(v); ok { O[i] = decoded } } } }

As you can see, I added a maximum parsing depth. Not sure we really want to parse fully recursively here. I'd use maxDepth=1 by default (Make it configurable). More parsing can be done by configuring a second processor continue parsing on already parsed field.
I also added optional array support, plus lifted the requirement for JSON Objects parsing only.

Having DecodeJson, there is no need for exporting jsontransform as is (but I'd keep jsontransform package for now, as other modules in beats might make use of it).

What do you think?

I like the idea of having a maxDepth setup so that we don't recursively parse till the end.
This does give us the ability to configure the processors for different fields with different levels of depth parsing.

suraj-soni · 2016-11-11T22:04:00Z

I like your suggestions and will work towards incorporating them in the changes and share it for further review.

suraj-soni · 2016-11-16T05:28:04Z

Hi @urso ,

Have made the suggested changes and added a couple of test cases to verify the maxDepth feature as well. Thanks once again for your help in refining this.

urso · 2016-11-18T12:49:38Z

@suraj-soni LGTM. Unfortunately you've got a merge conflict on .gitignore and PR overwrites some vendored file with an old version. Can you rebase based on master, so I can merge?

suraj-soni · 2016-11-19T00:29:12Z

@urso : I've merged the changes from the master and also removed the conflict with .gitignore.

urso · 2016-11-21T23:50:57Z

Merged. Thanks!

suraj-soni · 2016-11-22T00:31:25Z

Thanks for helping to get this in @urso and @andrewkroh !

ruflin · 2016-11-22T09:24:37Z

vendor/github.com/elastic/go-ucfg/CHANGELOG.md

@@ -120,6 +120,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).

 [Unreleased]: https://github.com/elastic/go-ucfg/compare/v0.3.7...HEAD
 [0.3.7]: https://github.com/elastic/go-ucfg/compare/v0.3.6...v0.3.7
+[Unreleased]: https://github.com/elastic/go-ucfg/compare/v0.3.6...HEAD


@urso Now sure how this sneaked in. Can you clean that up in #3045

ruflin · 2016-11-22T09:28:29Z

CHANGELOG.asciidoc

@@ -33,6 +33,7 @@ https://github.com/elastic/beats/compare/v5.0.0...master[Check the HEAD diff]
 ==== Bugfixes

 *Affecting all Beats*


This should probably not be under bug fixes? This is a new feature as far as I understand.

ruflin · 2016-11-22T09:29:18Z

@dedemorton @urso There are no docs for this yet and also no entries yet. As this will probably make it into 5.1 we should definitively add docs to it with examples on how to use it.

@suraj-soni In case you can help here, all docs are in asciidoc and can be found here for general processors: https://github.com/elastic/beats/tree/master/libbeat/docs As this is mostly filebeat related, it probably makes sense to add examples to filebeat docs: https://github.com/elastic/beats/tree/master/filebeat/docs

See elastic#2605

ruflin · 2016-11-22T09:33:03Z

Here is a fix for the two issues mentioned: #3047

See #2605

suraj-soni · 2016-11-28T18:39:31Z

@ruflin : Sure, I'd be happy to help with the documentation for this.

See elastic#2605

monicasarbu added review :Processors labels Oct 7, 2016

andrewkroh requested changes Oct 17, 2016

View reviewed changes

suraj-soni force-pushed the master branch 5 times, most recently from b54fdae to eaff781 Compare October 27, 2016 07:43

ruflin reviewed Oct 28, 2016

View reviewed changes

andrewkroh requested changes Oct 28, 2016

View reviewed changes

suraj-soni force-pushed the master branch 2 times, most recently from e8af13c to 35b174e Compare October 31, 2016 20:10

andrewkroh reviewed Oct 31, 2016

View reviewed changes

decode_json_fields processor

21555b2

suraj-soni force-pushed the master branch from 37cce8f to 21555b2 Compare October 31, 2016 22:33

andrewkroh approved these changes Oct 31, 2016

View reviewed changes

andrewkroh added the v5.1.0 label Nov 2, 2016

andrewkroh mentioned this pull request Nov 2, 2016

Document json_decode_fields processor #2923

Closed

urso reviewed Nov 2, 2016

View reviewed changes

suraj-soni added 2 commits November 9, 2016 13:26

Merge remote-tracking branch 'upstream/master'

842c996

Refactored to pull json_tranform into its own module

91b64a8

urso reviewed Nov 11, 2016

View reviewed changes

Refactored decode_json_fields to accomodate feedback

3e53022

Merge remote-tracking branch 'upstream/master'

c4c3a8c

urso merged commit 50db534 into elastic:master Nov 21, 2016

ruflin reviewed Nov 22, 2016

View reviewed changes

ruflin added a commit to ruflin/beats that referenced this pull request Nov 22, 2016

Cleanup CHANGELOG entry for json_decode

70e902d

See elastic#2605

ruflin mentioned this pull request Nov 22, 2016

Cleanup CHANGELOG entry for json_decode #3047

Merged

andrewkroh pushed a commit that referenced this pull request Nov 22, 2016

Cleanup CHANGELOG entry for json_decode (#3047)

1a553a5

See #2605

dedemorton mentioned this pull request Nov 29, 2016

Beats Doc Updates for 5.1 #3019

Closed

9 tasks

suraj-soni pushed a commit to suraj-soni/beats that referenced this pull request Dec 15, 2016

Cleanup CHANGELOG entry for json_decode (elastic#3047)

b291d7d

See elastic#2605

		@@ -33,6 +33,7 @@ https://github.com/elastic/beats/compare/v5.0.0...master[Check the HEAD diff]
		==== Bugfixes

		Affecting all Beats

json_decode_fields processor #2605

json_decode_fields processor #2605

Conversation

suraj-soni commented Sep 20, 2016 • edited

elasticsearch-release commented Sep 20, 2016

elasticsearch-release commented Sep 20, 2016

tsg commented Oct 5, 2016

suraj-soni commented Oct 7, 2016

suraj-soni commented Oct 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suraj-soni commented Oct 26, 2016

andrewkroh commented Oct 26, 2016 • edited

suraj-soni commented Oct 27, 2016

tsg commented Oct 27, 2016

Choose a reason for hiding this comment

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh Oct 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suraj-soni commented Oct 31, 2016

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suraj-soni commented Oct 31, 2016

Choose a reason for hiding this comment

suraj-soni Nov 9, 2016 • edited

Choose a reason for hiding this comment

monicasarbu commented Nov 9, 2016

urso commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suraj-soni commented Nov 11, 2016

suraj-soni commented Nov 16, 2016

urso commented Nov 18, 2016

suraj-soni commented Nov 19, 2016

urso commented Nov 21, 2016

suraj-soni commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Nov 22, 2016

ruflin commented Nov 22, 2016

suraj-soni commented Nov 28, 2016

suraj-soni commented Sep 20, 2016 •

edited

andrewkroh commented Oct 26, 2016 •

edited

andrewkroh Oct 28, 2016 •

edited

suraj-soni Nov 9, 2016 •

edited