Plugin/reader each interval by MrMaxBuilds · Pull Request #4332 · influxdata/telegraf

MrMaxBuilds · 2018-06-21T18:53:00Z

closes #3479
closes #3883

Required for all PRs:

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

glinton

Reviewing specifically for the plugin dev framework

glinton · 2018-06-26T20:10:36Z

plugins/inputs/reader/dev/telegraf.conf

@@ -0,0 +1,106 @@
+


Slim this file down by removing comments and default values. See other example.

glinton · 2018-06-26T20:11:33Z

telegraf.conf

@@ -0,0 +1,104 @@
+


Remove this file entirely, no need to have a global telegraf.conf

glinton · 2018-06-26T20:13:14Z

plugins/inputs/reader/dev/docker-compose.yml

+      entrypoint:
+        - /telegraf
+        - --config
+        - /telegraf.conf


Nitpick: add newline at the end of this and json_a.log

glinton · 2018-06-26T20:14:15Z

Makefile

 	ragel -Z -G2 $^ -o $@

-.PHONY: deps telegraf install test test-windows lint vet test-all package clean docker-image fmtcheck uint64
+static:


This may cause a merge conflict when we merge #4324 unless you merged that branch into yours.

What I do when I need to have parts from another commit/branch is either cherry-pick that commit or make the change to the shared file, and just not commit the changes to that file from my new branch.

glinton · 2018-06-27T01:59:26Z

Resolves #3479
Resolves #3883

glinton · 2018-06-28T17:45:13Z

plugins/parsers/registry.go

 	return parser, err
 }

+func NewGrokParser(metricName string,


Similar to a comment on 4351, lets not export this function.

danielnelson

In addition to the comments, we should also update logparser to use the new grok parser internally.

danielnelson · 2018-07-02T23:37:42Z

internal/config/config.go

+	delete(tbl.Fields, "patterns")
+	delete(tbl.Fields, "custom_patterns")
+	delete(tbl.Fields, "custom_pattern_files")
+	delete(tbl.Fields, "timezone")


Since these all are at plugin level now, lets prefix them with grok_

danielnelson · 2018-07-02T23:39:54Z

plugins/inputs/reader/reader.go

+## Each data format has its own unique set of configuration options, read
+## more about them here:
+## https://github.com/influxdata/telegraf/blob/master/docs/DATA_FORMATS_INPUT.md
+data_format = ""


Use 2 space indention in the sample config, make sure to update the README as well. You can generate the config for the readme with telegraf -usage reader.

danielnelson · 2018-07-02T23:43:47Z

plugins/inputs/reader/reader.go

+func (r *Reader) refreshFilePaths() {
+	var allFiles []string
+	for _, filepath := range r.Filepaths {
+		g, err := globpath.Compile(filepath)


I would compile only once and store the result on the Reader struct. Since we don't have a constructor function yet, I would probably check if the value on the struct is nil and if so compile.

My thought was that if people want to specify a directory via some globpath, then this will check for any new files added there during runtime

danielnelson · 2018-07-02T23:44:29Z

plugins/inputs/reader/reader.go

+	for _, filepath := range r.Filepaths {
+		g, err := globpath.Compile(filepath)
+		if err != nil {
+			log.Printf("E! Error Glob %s failed to compile, %s", filepath, err)


This error should probably stop the input since it indicates a user error, if any glob fails return the error up and out of Gather.

danielnelson · 2018-07-02T23:48:38Z

plugins/inputs/reader/reader.go

+func (r *Reader) readMetric(filename string) ([]telegraf.Metric, error) {
+	fileContents, err := ioutil.ReadFile(filename)
+	if err != nil {
+		log.Printf("E! File could not be opened: %v", filename)


Error should be along the lines of "File could not be read" for accuracy, but also this error should be returned instead of logged. In general it is not safe to do anything with the return value if an error occurs.

danielnelson · 2018-07-02T23:52:44Z

plugins/inputs/reader/reader.go

+				log.Printf("E! Metric could not be parsed from: %v, on line %v", k, i)
+				continue
+			}
+			acc.AddFields(m.Name(), m.Fields(), m.Tags())


Also add the m.Time().

danielnelson · 2018-07-03T00:06:56Z

plugins/inputs/reader/reader_test.go

+	assert.Equal(t, 2, len(acc.Metrics))
+}
+
+func getPluginDir() string {


I think the cwd is actually guaranteed by go test to be the directory containing the test file.

Check if this is required and if not remove.

danielnelson · 2018-07-03T00:11:48Z

plugins/parsers/grok/influx-patterns

@@ -0,0 +1,73 @@
+# Captures are a slightly modified version of logstash "grok" patterns, with


I feel like I should know this, but... any idea why we need this file when we have influx_patterns.go?

I wasn't sure if it was something relevant. I was under the impression that it was simply the text from influx_patterns.go. I think it can be removed.

Go ahead and remove this.

danielnelson · 2018-07-03T00:15:19Z

plugins/parsers/grok/parser.go

+	NamedPatterns      []string
+	CustomPatterns     string
+	CustomPatternFiles []string
+	Measurement        string


Let's remove Measurement, we already have name_override which does the same thing.

danielnelson · 2018-07-03T00:29:52Z

plugins/parsers/grok/parser_test.go

@@ -0,0 +1,19 @@
+package grok


Needs more tests, probably easiest to just bring over all tests from grok_test.go

danielnelson · 2018-07-12T18:36:33Z

docs/DATA_FORMATS_INPUT.md

+  grok_custom_pattern_files = []
+
+  ## Custom patterns can also be defined here. Put one pattern per line.
+  grok_custom_patterns = '''


I think we need a closing ''' for this below.

danielnelson · 2018-07-12T18:39:56Z

plugins/inputs/logparser/logparser.go

-			return err
-		}
-	}
-


It looks like we are removing l.parsers, I think this is good but also remove the reflect bit above 156:175 and the field from the LogParserPlugin struct.

danielnelson · 2018-07-12T18:44:36Z

plugins/inputs/reader/reader.go

+)
+
+type Reader struct {
+	Filepaths     []string `toml:"files"`


Call this Files, if its good enough for the config its good enough for the struct, and it will be less confusing.

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

plugins/inputs/reader/reader.go

+	FromBeginning bool
+	parser        parsers.Parser
+
+	Filenames []string


Make this a unexported field with a lowercase letter, otherwise you can set this from the config file which is undesired.

danielnelson · 2018-07-12T18:47:05Z

plugins/inputs/reader/reader.go

+	for _, filepath := range r.Filepaths {
+		g, err := globpath.Compile(filepath)
+		if err != nil {
+			return fmt.Errorf("E! Error Glob: %v could not be compiled, %s", filepath, err)


Similar to my comment on the json parser, since this is an error don't include E! and try to shorten: could not compile glob %q: %v

danielnelson · 2018-07-12T18:51:00Z

plugins/parsers/grok/parser.go

+}
+
+func (p *Parser) Parse(buf []byte) ([]telegraf.Metric, error) {
+	lines := strings.Split(string(buf), "\n")


Use bufio.Scanner so that line splitting will work on files with DOS line endings.

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

plugins/parsers/grok/parser.go

 	CustomPatterns     string
 	CustomPatternFiles []string
-	Measurement        string
+	MetricName         string


Keep this named Measurement.

danielnelson · 2018-07-12T18:54:36Z

plugins/parsers/grok/influx-patterns

@@ -0,0 +1,73 @@
+# Captures are a slightly modified version of logstash "grok" patterns, with


Go ahead and remove this.

danielnelson · 2018-07-12T18:55:05Z

plugins/inputs/reader/testfiles/json_a.log

+			"ignored_string": "hello, world!"
+		},
+		"another_list": [4]
+	}


Add end of line for this file, also fix the indention.

danielnelson · 2018-07-12T18:55:27Z

plugins/inputs/reader/reader_test.go

+	assert.Equal(t, 2, len(acc.Metrics))
+}
+
+func getPluginDir() string {


Check if this is required and if not remove.

MrMaxBuilds added 17 commits June 21, 2018 11:26

input plugin that reads files each interval

e12eced

change config file

08a11d7

tweak metric output

9c4b522

add grok as a top level parser

4e24a1b

add more test files

ec7f131

clean up some test cases

504d978

knock more errors from test files

542c030

add setparser to reader

554b960

Merge branch 'master' into plugin/reader

36a23ea

add init function to reader

f40371e

add grok as a top level parser, still need README

9c84595

allow for import from plugins/all

cc40629

add docker-image spin up for reader

79d9ea4

docker will spin up

bbd68b3

add test file to docker spin up

bf7220d

update DATA_FORMATS_INPUT.MD to include grok

a931eb1

remove comments

e450b26

glinton suggested changes Jun 26, 2018

View reviewed changes

MrMaxBuilds added 2 commits June 26, 2018 13:18

condense telegraf.conf

001658a

more condensing

7fa27f4

glinton force-pushed the plugin/reader branch 2 times, most recently from 513ec70 to 7fa27f4 Compare June 26, 2018 23:06

Formatting and revert Makefile

1be2a8e

glinton approved these changes Jun 26, 2018

View reviewed changes

MrMaxBuilds added 2 commits June 27, 2018 10:13

add reader README.md

aa750ec

update readmes

892c95a

glinton reviewed Jun 28, 2018

View reviewed changes

grok parser func unexported

04f09d6

danielnelson reviewed Jul 3, 2018

View reviewed changes

MrMaxBuilds added 7 commits July 6, 2018 11:22

logparser is linked to grok parser

8a9da28

logparser no longer uses seperate grok

cafa95e

add more unit tests to grok parser

c6087ab

fix unit tests for grok parser

e4b6f23

change logparser unit tests

d224673

test files added for logparser

f52ceeb

Merge branch 'master' into plugin/reader

285cf0b

danielnelson reviewed Jul 12, 2018

View reviewed changes

MrMaxBuilds added 5 commits July 12, 2018 13:27

addresses daniel's comments

0c3ac29

change parser config names

74900ed

allow for original config and functionality of logparser

d0f5389

finish daniel's changes

dd778a9

small change to config

5449eb7

danielnelson added this to the 1.8.0 milestone Jul 14, 2018

danielnelson added the new plugin label Jul 14, 2018

danielnelson added 8 commits July 13, 2018 18:30

Rename reader to file input

1f58dd7

Attempt linking to another plugin

2a18ca2

Adjust link to data format docs

50f49fe

Rename Reader struct to File

08d1397

Move measurement option to logparser grok config

63da4e6

Set default data_format to influx

88a85a7

Add deprecation messages to logparser input

2638186

Fix dev config for file input

1fe3adb

danielnelson merged commit 774a9f0 into master Jul 14, 2018

danielnelson deleted the plugin/reader branch July 14, 2018 06:23

glinton mentioned this pull request Jul 23, 2018

tail errors with malformed log line for Apache2.2 access log file #4442

Closed

xor-gate mentioned this pull request Jul 26, 2018

DATA_FORMATS_INPUT.md github HTML preview is broken #4473

Closed

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

Add file input plugin and grok parser (influxdata#4332)

e121a5d

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

Add file input plugin and grok parser (influxdata#4332)

1b4ea4c

github-actions bot mentioned this pull request Aug 3, 2025

Link Checker Report WAYlOn16348/DVeNtBr4Y#6

Open

		@@ -0,0 +1,73 @@
		# Captures are a slightly modified version of logstash "grok" patterns, with

Conversation

MrMaxBuilds commented Jun 21, 2018 • edited by danielnelson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Required for all PRs:

Uh oh!

glinton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glinton commented Jun 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielnelson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

MrMaxBuilds commented Jun 21, 2018 •

edited by danielnelson

Loading