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

Adds a 'timestamp_units' configuration parameter #2143

Closed
wants to merge 2 commits into from

Conversation

tjmcs
Copy link

@tjmcs tjmcs commented Dec 10, 2016

Resolves #2124

The changes in this pull request add a new timestamp_units parameter to the agent configuration. This parameter only effects the timestamps output for the json data_format, and take the effect of a duration string (values like 1ns, 1us, 1ms and 1s are all supported). If this parameter is not defined in the configuration file then the current behavior is preserved (the timestamps output as part of the json data_format are truncated to the nearest second).

The following output shows the effect of this parameter when it is unset (the current default) and when it is set to 1us and 1ns (so that the timestamps are output in microseconds and nanoseconds, respectively) for a configuration that is collecting system data to the default file output:

$ telegraf --config telegraf-sys-file.conf
2016-12-10T03:59:30Z I! Starting Telegraf (version dev-41-g4f6087a)
2016-12-10T03:59:30Z I! Loaded outputs: file
2016-12-10T03:59:30Z I! Loaded inputs: inputs.system
2016-12-10T03:59:30Z I! Tags enabled: host=Toms-MBP
2016-12-10T03:59:30Z I! Agent Config: Interval:10s, Quiet:false, Hostname:"Toms-MBP", Flush Interval:10s
{"fields":{"load1":1.8,"load15":1.55,"load5":1.59,"n_cpus":8,"n_users":8},"name":"system","tags":{"host":"Toms-MBP"},"timestamp":1481342380}
{"fields":{"uptime":2838305,"uptime_format":"32 days, 20:25"},"name":"system","tags":{"host":"Toms-MBP"},"timestamp":1481342380}
{"fields":{"load1":1.59,"load15":1.53,"load5":1.55,"n_cpus":8,"n_users":8},"name":"system","tags":{"host":"Toms-MBP"},"timestamp":1481342390}
{"fields":{"uptime":2838315,"uptime_format":"32 days, 20:25"},"name":"system","tags":{"host":"Toms-MBP"},"timestamp":1481342390}
^C2016-12-10T03:59:52Z I! Hang on, flushing any cached metrics before shutdown

$ telegraf --config telegraf-sys-file-1us.conf
2016-12-10T03:59:59Z I! Starting Telegraf (version dev-41-g4f6087a)
2016-12-10T03:59:59Z I! Loaded outputs: file
2016-12-10T03:59:59Z I! Loaded inputs: inputs.system
2016-12-10T03:59:59Z I! Tags enabled: host=Toms-MBP
2016-12-10T03:59:59Z I! Agent Config: Interval:10s, Quiet:false, Hostname:"Toms-MBP", Flush Interval:10s
{"fields":{"load1":1.58,"load15":1.53,"load5":1.55,"n_cpus":8,"n_users":8},"name":"system","tags":{"host":"Toms-MBP"},"timestamp":1481342400052393}
{"fields":{"uptime":2838325,"uptime_format":"32 days, 20:25"},"name":"system","tags":{"host":"Toms-MBP"},"timestamp":1481342400052466}
{"fields":{"load1":1.72,"load15":1.54,"load5":1.58,"n_cpus":8,"n_users":8},"name":"system","tags":{"host":"Toms-MBP"},"timestamp":1481342410057150}
{"fields":{"uptime":2838335,"uptime_format":"32 days, 20:25"},"name":"system","tags":{"host":"Toms-MBP"},"timestamp":1481342410057235}
^C2016-12-10T04:00:11Z I! Hang on, flushing any cached metrics before shutdown

$ telegraf --config telegraf-sys-file-1ns.conf
2016-12-10T04:00:15Z I! Starting Telegraf (version dev-41-g4f6087a)
2016-12-10T04:00:15Z I! Loaded outputs: file
2016-12-10T04:00:15Z I! Loaded inputs: inputs.system
2016-12-10T04:00:15Z I! Tags enabled: host=Toms-MBP
2016-12-10T04:00:15Z I! Agent Config: Interval:10s, Quiet:false, Hostname:"Toms-MBP", Flush Interval:10s
{"fields":{"load1":1.61,"load15":1.54,"load5":1.56,"n_cpus":8,"n_users":8},"name":"system","tags":{"host":"Toms-MBP"},"timestamp":1481342420052140075}
{"fields":{"uptime":2838345,"uptime_format":"32 days, 20:25"},"name":"system","tags":{"host":"Toms-MBP"},"timestamp":1481342420052209922}
{"fields":{"load1":1.6,"load15":1.53,"load5":1.56,"n_cpus":8,"n_users":8},"name":"system","tags":{"host":"Toms-MBP"},"timestamp":1481342430056069459}
{"fields":{"uptime":2838355,"uptime_format":"32 days, 20:25"},"name":"system","tags":{"host":"Toms-MBP"},"timestamp":1481342430056119581}
^C2016-12-10T04:00:31Z I! Hang on, flushing any cached metrics before shutdown

$

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

@phemmer
Copy link
Contributor

phemmer commented Dec 10, 2016

What about calling it 'precision' as this is what it's called everywhere else?

@tjmcs
Copy link
Author

tjmcs commented Dec 10, 2016

I had called the parameter that I'm adding to the configuration file the timestamp_precision in a previous pull request that was closed earlier this week that attempted to add this same functionality in a different way, @phemmer, but @sparrc suggested timestamp_units would be a better name for this parameter than timestamp_precision in his comments on that pull request (I'm assuming that his suggestion for changing the name was because what we were really defining using this parameter were the units of the timestamp, not the precision with which it was measured).

After seeing @sparrc's comments on that pull request I think I would have to agree that the timestamp_units name makes a bit more sense to me than my original choice. With that in mind, I changed the name of the parameter in this pull request, along with the approach that I took in adding this parameter into the configuration file. The differences in the resulting pull request were significant enough that I submitted this set of changes as a second pull request rather than attempting to rebase the feature branch that I'd used for the first pull request and reopening it (@sparrc had closed that pull request after making his comments on it).

So, with that in mind, does the name I've chosen for this parameter work for for you, @phemmer?

@sparrc
Copy link
Contributor

sparrc commented Jan 26, 2017

sorry this PR fell through the cracks, but your change needs to be for the JSON output serializer ONLY.

Do not make any changes to the telegraf agent. You should only need to make changes to the json serializer.

Also please make an attempt to reduce the verbosity of your documentation/comments. This is a simple config option that changes the precision of the timestamp output, you don't need to say much more than that.

@sparrc sparrc closed this Jan 26, 2017
@tjmcs
Copy link
Author

tjmcs commented Jan 26, 2017

I guess I'm a little confused, @sparrc; I thought I'd made a minimal set of changes based on your comments on my last pull request, but evidently not. The issue (as I see it) is that currently the JSON serializer outputs things in seconds, and we would really like the option to have JSON data tagged with timestamps in another unit (we want nanoseconds, but it should support the same units that are supported elsewhere in Telegraf...milliseconds, microseconds, etc.).

Unfortunately, as you pointed out in your comments on my previous pull request related to this issue, the current behavior for any JSON data that is output is to output the data with a timestamp in seconds. As such, I thought I would need to make a change to how the agent is configured so that the user could (if they would like) choose a different units value for the timestamps included when the metrics collected were output in JSON but if that value was not specified in the configuration the current behavior (outputting the timestamps in seconds) would be maintained. So that's why I made what I felt were a minimal set of changes outside of the JSON serializer itself; to preserve the current behavior but allow for finer precision in the timestamps if we wanted it.

As I was testing my changes, however, I ran across another, related issue. If you enter the timestamp units as ns, us, ms, etc. the code that is currently in place in the Telegraf codebase parses that string as if it was prefixed with a 0 (0ns, for example), and if the value is zero then the units that are input are ignored and the timestamp value is output in seconds (regardless of the units string you entered). That was why I made a second commit to the branch that this pull request is based on (to deal with that issue).

If you want me to reduce this to a one-line change that always outputs the JSON data with nanoseconds units for the time string that's a much simpler thing to do, but I think there will have to be changes made outside of the serializer to deal with the two issues I've outlined above. I'm happy to rebase the code on this branch to make changes to it if you want me to, but I'm not sure that I can limit the changes to only the serializer (unless I'm missing something obvious; if so, enlighten me).

@tjmcs tjmcs deleted the tb/add-json-timestamp-units branch March 24, 2017 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants