Feature/1363: Additional configuration for JSON parser #4351

maxunt · 2018-06-27T23:16:05Z

closes: #2986

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

glinton · 2018-06-28T16:44:58Z

plugins/parsers/registry.go

@@ -129,11 +131,13 @@ func NewParser(config *Config) (Parser, error) {
 func NewJSONParser(
 	metricName string,
 	tagKeys []string,
+	fieldKeys []string,


Generally we want to avoid changing exported function signatures, as it destabilizes packages. Perhaps what we could do is add a newJSONParser that takes different (or functional) parameters. We could then update those things that call NewJSONParser directly to call NewParser, which will in turn call newJSONParser.

danielnelson · 2018-07-10T00:42:20Z

@maxunt As we discussed, let's have this be string_keys and list flattened paths to values that should be added as string fields.

danielnelson · 2018-07-12T18:04:45Z

plugins/parsers/json/parser.go

+	nTime := time.Now().UTC()
+	if p.JSONTimeKey != "" {
+		if p.JSONTimeFormat == "" {
+			err := fmt.Errorf("E! If 'json_time_key' is specified, there must be a 'json_time_format'")


Since this is an error and not a log message, don't include the E!. As a nitpick, it would probably be good to try to come up with a shorter error string, perhaps: use of 'json_time_key' requires 'json_time_format'?

Remember to do this for the other errors, here is some more info on how "standard" Go error strings should look: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

danielnelson · 2018-07-12T18:09:04Z

plugins/parsers/json/parser.go

@@ -114,6 +137,7 @@ func (p *JSONParser) switchFieldToTag(tags map[string]string, fields map[string]

 func (p *JSONParser) Parse(buf []byte) ([]telegraf.Metric, error) {

+	//if json_query is specified


Remove this comment, it doesn't provide any new information on why and the what can be determined by looking at the code.

danielnelson · 2018-07-12T18:15:40Z

docs/DATA_FORMATS_INPUT.md

+  ## List of field names to extract from JSON and add as string fields
+  # string_fields = []
+
+  ## gjson query path to specify a specific chunk of JSON to be parsed with the above configuration


Wrap comments before 78 chars for readability.

danielnelson · 2018-07-12T18:17:41Z

plugins/parsers/json/parser.go

+			return nil, err
+		}
+
+		nTime, err = time.Parse(p.JSONTimeFormat, f.Fields[p.JSONTimeKey].(string))


Make sure to test the type assertion since it comes from input and could be any type.

danielnelson · 2018-07-12T18:22:01Z

plugins/parsers/json/parser_test.go

+	}
+
+	_, err := parser.Parse([]byte(testString))
+	assert.Error(t, err)


Use require instead of assert (almost) everywhere. Doesn't really matter here but this will prevent confusing chained errors.

danielnelson · 2018-07-12T18:28:02Z

plugins/parsers/json/parser_test.go

+	metrics, err := parser.Parse([]byte(testString))
+	assert.NoError(t, err)
+	assert.Equal(t, metrics[0].Fields()["last"], "Murphy")
+	assert.Equal(t, 3, len(metrics))


Check the length before indexing, this will make the test easier to understand if it fails.

danielnelson · 2018-07-12T18:28:49Z

plugins/parsers/json/parser_test.go

+	}
+	metrics, err := parser.Parse([]byte(testString))
+	assert.NoError(t, err)
+	assert.Equal(t, false, metrics[0].Time() == metrics[1].Time())


Check len(metrics) first, add some more tests to check error cases.

danielnelson · 2018-07-12T18:30:58Z

docs/DATA_FORMATS_INPUT.md

+
+The JSON data format also supports extracting time values through the config "json_time_key" and "json_time_format". 
+If "json_time_key" is set, "json_time_format" must be specified.  The "json_time_key" describes the name of the field containing time information.  The "json_time_format" must be a recognized Go time format.  
+More info on time formats can be found here: https://golang.org/src/time/format.go


Use https://golang.org/pkg/time/#Parse as the link. This other link is still not great but the audience of this documentation is not Go developers.

Try to wrap around 78 chars

glinton · 2018-07-26T15:16:12Z

docs/DATA_FORMATS_INPUT.md

+
+  ## gjson query path to specify a specific chunk of JSON to be parsed with 
+  ## the above configuration. If not specified, the whole file will be parsed. 
+  ## gjson query paths are described here: 


Add link to where gjson query paths are described

glinton · 2018-07-26T15:19:22Z

plugins/parsers/registry.go

 	// MetricName applies to JSON & value. This will be the name of the measurement.
 	MetricName string

+	//holds a gjson path for json parser


When adding comments, add a space before starting comment text (// This is my comment)

glinton · 2018-07-26T15:43:41Z

plugins/parsers/registry.go

+	timeKey string,
+	timeFormat string,
+	defaultTags map[string]string,
+) (Parser, error) {


Returning an error isn't useful here. Where it's an unexported function not matching any interface, I'd remove it, or remove the function altogether and just instantiate a JSONParser on line 120

No error is ever returned by the new_Parser functions, but i thought i'd include it here to match the format of the rest of the parsers

glinton · 2018-07-26T15:47:46Z

plugins/parsers/json/parser.go

@@ -126,7 +177,7 @@ func (p *JSONParser) ParseLine(line string) (telegraf.Metric, error) {
 	}

 	if len(metrics) < 1 {
-		return nil, fmt.Errorf("Can not parse the line: %s, for data format: influx ", line)
+		return nil, fmt.Errorf("can not parse the line: %s, for data format: influx ", line)


Also the copypasta, s/influx/json

could you clarify what you mean here?

Replace "influx" with "json", it appears to be copy paste code

ah i see. good catch thanks

danielnelson · 2018-07-30T18:22:05Z

docs/DATA_FORMATS_INPUT.md

+  json_time_key = "b_time"
+
+  ## holds the format of timestamp to be parsed
+  json_time_format = "02 Jan 06 15:04 MST"


This is an interesting example time format because there it doesn't include a year. This results in a pared time that is in year 0. I think after parsing the time we should do something similar to what we did in logparser and use the current year. While this would be pretty annoying if you actually want to store in a metric from year 0, I think it is much more likely to be what is wanted:

if ts.Year() == 0 { ts = ts.AddDate(timestamp.Year(), 0, 0) }

…ata#4351)

maxunt added 2 commits June 27, 2018 16:09

add "field_tags" to json parser config

420dafd

edit unittests for JSON parser

c50bfc2

glinton reviewed Jun 28, 2018

View reviewed changes

maxunt added 6 commits June 28, 2018 11:45

test case edit

7f2b2a0

newJsonParser function is now unexported, must use NewParser(config)

4db6675

change test files to reflect unexported newJsonParser func

885193b

readability tweak

674b957

add old exported func NewJSONParser

e25fbed

comments reflect deprecated NewJsonParser

9196488

glinton approved these changes Jun 28, 2018

View reviewed changes

maxunt closed this Jul 2, 2018

danielnelson reopened this Jul 10, 2018

maxunt added 4 commits July 10, 2018 14:04

gjson path support for json parser, still needs string_field supp

22e28e0

support for string_fields

6207324

update readme and unit tests

dca5e99

add support for time parsing

5e22ec4

danielnelson reviewed Jul 12, 2018

View reviewed changes

maxunt added 3 commits July 12, 2018 11:33

update readme

867c3a7

address daniel's comments

fa4b2f0

changed some comments

197c474

danielnelson added this to the 1.8.0 milestone Jul 16, 2018

danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jul 16, 2018

maxunt mentioned this pull request Jul 17, 2018

Parser/gjson #4368

Closed

Merge branch 'master' into feature/1363

e2ebce0

glinton reviewed Jul 26, 2018

View reviewed changes

maxunt added 2 commits July 26, 2018 13:59

address some of greg's comments

ab3433a

address more of greg's comments

8445d14

danielnelson suggested changes Jul 30, 2018

View reviewed changes

Merge branch 'master' into feature/1363

aff2697

maxunt added 2 commits August 16, 2018 09:53

unspecified years in timestamp yields current year

cabf688

add json_name_key config

145c5ef

danielnelson approved these changes Aug 16, 2018

View reviewed changes

danielnelson merged commit 2729378 into master Aug 23, 2018

danielnelson deleted the feature/1363 branch August 23, 2018 02:26

danielnelson mentioned this pull request Aug 23, 2018

inputs.exe json plugin can't parse input with strings #4589

Closed

rgitzel pushed a commit to rgitzel/telegraf that referenced this pull request Oct 17, 2018

Add name, time, path and string field options to JSON parser (influxd…

f6c1d43

…ata#4351)

otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019

Add name, time, path and string field options to JSON parser (influxd…

039d2e4

…ata#4351)

otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019

Add name, time, path and string field options to JSON parser (influxd…

9e92fcd

…ata#4351)

dupondje pushed a commit to dupondje/telegraf that referenced this pull request Apr 22, 2019

Add name, time, path and string field options to JSON parser (influxd…

7253979

…ata#4351)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/1363: Additional configuration for JSON parser #4351

Feature/1363: Additional configuration for JSON parser #4351

maxunt commented Jun 27, 2018 •

edited by danielnelson

Loading

glinton Jun 28, 2018

danielnelson commented Jul 10, 2018

danielnelson Jul 12, 2018

danielnelson Jul 12, 2018

danielnelson Jul 12, 2018

danielnelson Jul 12, 2018

danielnelson Jul 12, 2018

danielnelson Jul 12, 2018

danielnelson Jul 12, 2018

danielnelson Jul 12, 2018

glinton Jul 26, 2018

glinton Jul 26, 2018

glinton Jul 26, 2018

maxunt Jul 26, 2018

glinton Jul 26, 2018

maxunt Jul 26, 2018

glinton Jul 27, 2018

maxunt Jul 27, 2018

danielnelson Jul 30, 2018

		@@ -114,6 +137,7 @@ func (p *JSONParser) switchFieldToTag(tags map[string]string, fields map[string]

		func (p *JSONParser) Parse(buf []byte) ([]telegraf.Metric, error) {

		//if json_query is specified

Feature/1363: Additional configuration for JSON parser #4351

Feature/1363: Additional configuration for JSON parser #4351

Conversation

maxunt commented Jun 27, 2018 • edited by danielnelson Loading

Required for all PRs:

Choose a reason for hiding this comment

danielnelson commented Jul 10, 2018

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

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

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

maxunt commented Jun 27, 2018 •

edited by danielnelson

Loading