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

agent/accumulator: expose GetTime to give input plugins a chance to r… #4078

Merged
merged 5 commits into from
Apr 27, 2018

Conversation

zerodeux
Copy link
Contributor

…euse a timestamp for several Add* calls + use this on plugins/system to fix telegraf-sample/influxdb-point mismatch pb

I stumbled upon curious single system measures spanning over two distinct timestamps, on a second boundary (this bug happening a few times in a day over 300 servers collecting system measures every 10s) :

> SELECT * FROM system WHERE time > now() - 5m and host='xxxxxxxxx-xxxxxxx'
name: system
time                host              load1 load15 load5 n_cpus n_users uptime  uptime_format
----                ----              ----- ------ ----- ------ ------- ------  -------------
...
1524726770000000000 xxxxxxxxx-xxxxxxx 0.79  0.64   0.6   4      0       6685039 77 days,  8:57 
1524726780000000000 xxxxxxxxx-xxxxxxx 1.14  0.67   0.68  4      0     
1524726781000000000 xxxxxxxxx-xxxxxxx                                   6685049 77 days,  8:57 
1524726790000000000 xxxxxxxxx-xxxxxxx 1.41  0.69   0.76  4      0       6685059 77 days,  8:57 
...

I realized that the input/system plugins call three accumulator.Add* methods in a row, each fetching a distinct timestamp. This is cumbersome since its much more useful to gather all those fields as a single InfluxDB point and hence with the same timestamp.

In order to to this, I had to expose the accumulator.getTime() function. This should not break any plugin assumption and behaviour.

…euse a timestamp for several Add* calls + use this on plugins/system to fix telegraf-sample/influxdb-point mismatch pb
@danielnelson
Copy link
Contributor

This is a good fix but I think we should just create one time and use it in both calls, no need to add the GetTime function.

now := time.Now()
acc.AddGauge(..., now)
acc.AddCounter(..., now)

@zerodeux
Copy link
Contributor Author

OK, and I missed the point that other plugins are alreay doing this. I thought that Telegraf was somewhat in control of the time notion (especially wrt. precision) and didn't dare to go to time.Now() directly. I'll push a simpler fix for plugins/inputs/system ASAP.

@danielnelson danielnelson added this to the 1.6.2 milestone Apr 27, 2018
@danielnelson danielnelson added the fix pr to fix corresponding bug label Apr 27, 2018
@danielnelson danielnelson merged commit ec47cab into influxdata:master Apr 27, 2018
danielnelson pushed a commit that referenced this pull request Apr 27, 2018
@danielnelson
Copy link
Contributor

Thanks! I include this in the next patch release (1.6.2)

@zerodeux
Copy link
Contributor Author

Thanks for your quick review and integration !

arkady-emelyanov pushed a commit to arkady-emelyanov/telegraf that referenced this pull request May 18, 2018
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants