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

Plugins parsers csv unix timestamp #5047

Merged

Conversation

tkanos
Copy link
Contributor

@tkanos tkanos commented Nov 27, 2018

Required for all PRs:

  • [ x ] Signed CLA.
  • [ x ] Associated README.md updated.
  • [ x ] Has appropriate unit tests.

Description.

For now the csv parser does not handle unix timestamp like 1257894000 as csv_timestamp_format in the csv as timestamp.

So to fix that if csv_timestamp_format = Unix, we assume that the timestamp is an epoch ms format.

metricTime = time.Unix(timeUnixValue, 0)

I have also added unit tests, and no others unit test were broken.

=== RUN TestBasicCSV
--- PASS: TestBasicCSV (0.00s)
=== RUN TestHeaderConcatenationCSV
--- PASS: TestHeaderConcatenationCSV (0.00s)
=== RUN TestHeaderOverride
--- PASS: TestHeaderOverride (0.00s)
=== RUN TestTimestamp
--- PASS: TestTimestamp (0.00s)
=== RUN TestTimestampError
--- PASS: TestTimestampError (0.00s)
=== RUN TestTimestampUnixFormat
--- PASS: TestTimestampUnixFormat (0.00s)

=== RUN TestQuotedCharacter
--- PASS: TestQuotedCharacter (0.00s)
=== RUN TestDelimiter
--- PASS: TestDelimiter (0.00s)
=== RUN TestValueConversion
--- PASS: TestValueConversion (0.00s)
=== RUN TestSkipComment
--- PASS: TestSkipComment (0.00s)
=== RUN TestTrimSpace
--- PASS: TestTrimSpace (0.00s)
=== RUN TestSkipRows
--- PASS: TestSkipRows (0.00s)
=== RUN TestSkipColumns
--- PASS: TestSkipColumns (0.00s)
=== RUN TestSkipColumnsWithHeader
--- PASS: TestSkipColumnsWithHeader (0.00s)
=== RUN TestParseStream
--- PASS: TestParseStream (0.00s)
PASS
ok github.com/influxdata/telegraf/plugins/parsers/csv

Copy link
Contributor

@glinton glinton left a comment

Choose a reason for hiding this comment

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

Thanks! While this works, I think it would be nice to eventually centralize the json time format logic into the internal package

Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

Thanks @tkanos, in order to be closer in behavior to the other parsers can you make it so that if the csv_timestamp_format = "unix" then the format is parsed as unix time.

If we try to use unix timestamp in the csv file, we will get an error.
This commit fixes that.

* if the csv_timestamp_format = "unix" we try to parse the timestamp assuming that we will get an epoch in ms.
* Put all the parsing timestamp part in a function (as it deserves it :D)
@tkanos tkanos force-pushed the plugins-parsers-csv-unix-timestamp branch from 5007b2a to f2fdaae Compare November 28, 2018 15:03
@tkanos
Copy link
Contributor Author

tkanos commented Nov 28, 2018

Thanks @danielnelson for the advice. It was done.

@danielnelson danielnelson added this to the 1.10.0 milestone Nov 29, 2018
@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Nov 29, 2018
@@ -75,7 +75,8 @@ document.

The `csv_timestamp_column` option specifies the column name containing the
time value and `csv_timestamp_format` must be set to a Go "reference time"
which is defined to be the specific time: `Mon Jan 2 15:04:05 MST 2006`.
which is defined to be the specific time: `Mon Jan 2 15:04:05 MST 2006`,
it can also be `unix` (for epoch in ms format like 1257894000 )
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually this for seconds, not ms, so worries I'll switch it post merge.

@danielnelson danielnelson merged commit f9113b6 into influxdata:master Nov 29, 2018
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
dupondje pushed a commit to dupondje/telegraf that referenced this pull request Apr 22, 2019
bitcharmer pushed a commit to bitcharmer/telegraf that referenced this pull request Oct 18, 2019
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants