Keep input timestamp for prometheus metrics

[bozaro]

This change passes the metrics collection time to prometheus.
Prior to this change, prometheus can see one sample as two different samples, which generates dips/spikes on the charts.

For example:

• prometheus scrape interval = 7 sec;
• collection interval = 5 sec;
• expression: irate(system_uptime[30s])
  

[danielnelson]

Looks good, but based on my understanding of prometheus best-practice we should add a new option to the output that must be enabled to add timestamps: export_timestamp = true

[bozaro]

I added export_timestamp option.
Looks like timestamp was supported since 0.4 protocol version and there should be no problems from this option enabled by default (https://prometheus.io/docs/instrumenting/exposition_formats/).

[danielnelson]

Can you flip the default to false, we need to do this for backwards compatibility.

Separately, and we don't have to know now, it would be good to know if we should change it to true when Telegraf 2.0 comes out. Timestamps seem to be somewhat frowned upon in prometheus, prometheus/client_golang#187, but perhaps ours is the right use case. I also found https://github.com/prometheus/pushgateway#about-timestamps, I would think this is a fairly similar use and they have chosen not to export timestamps.

[bozaro]

I flipped the default value to false for backward compatibility.

But I'm sure, that in future export_timestamp should be true by default.
As I understand, prometheus guideline recommends some integration ways:

1. Client expose metrics and calculates metrics on demand. In this case client don't send timestamp, because prometheus use scribe time as metric timestamp.
2. Client send metric to prometheus pushgateway and push gateway use current time as metric timestamp.

Telegraf can't calculates metrics on demand, so it should use push gateway. Logically telegraf work with embedded push gateway for exposing metrics. And this embedded push gateway should store metrics timestamp for collected metrics.

[glinton]

Also, please refrain from force pushing once a review has been started. We squash merge so there won't be messy history anyways

[bozaro]

Only last commit was force pushed: I lost configuration parameter name from commit message.

[danielnelson]

I still feel like I'm missing some understanding. It seems to me almost obvious that Telegraf should recommend including the timestamp. At the same time it also seems that Telegraf operates very similarly to the pushgateway, which doesn't use timestamps. What happens when prometheus scapes a metric that is over 5m old?

[bozaro]

I did not check. I think he will add a backdating metric. But I admit that he will ignore her.
In any case, a metric with fixed timestamp looks like a point.

[mitchellrj]

You've added the export_timestamp option, but by my reading, the configuration option is always ignored and set to true, is that right @bozaro @danielnelson?

https://github.com/bozaro/telegraf/blob/17c65971c39979246e8beebd3ff0ce95a41c69c6/plugins/outputs/prometheus_client/prometheus_client.go#L514

[danielnelson]

You are right about the default being true, I will fix that, but the I believe the export_timestamp option is working and can be disabled.

[danielnelson]

Default option to false: ab1a1b0

[neeles83]

@danielnelson Can I ask an question in here about the fix/update? I have updated our existing Telegraf implementation with the new v1.14.1 and enabled the timestamp as shown below. But I don't see any timestamp information in my metrics.

Configuration:
[[outputs.prometheus_client]]
listen = ":9126"
metric_version = 2
export_timestamp = true
expiration_interval = 120s

Startup log:
2020-04-26T18:44:22Z I! Starting Telegraf 1.14.1
2020-04-26T18:44:22Z I! Loaded inputs: mem cpu net kernel disk diskio processes swap system http_listener_v2 cisco_telemetry_mdt snmp snmp snmp snmp snmp snmp snmp snmp_trap
2020-04-26T18:44:22Z I! Loaded aggregators:
2020-04-26T18:44:22Z I! Loaded processors:
2020-04-26T18:44:22Z I! Loaded outputs: prometheus_client
2020-04-26T18:44:22Z I! Tags enabled: host=9803f90fed0a
2020-04-26T18:44:22Z I! [agent] Config: Interval:3m0s, Quiet:false, Hostname:"9803f90fed0a", Flush Interval:1m0s
2020-04-26T18:44:22Z I! [outputs.prometheus_client] Listening on http://[::]:9126/metrics
2020-04-26T18:44:22Z I! [inputs.http_listener_v2] Listening on [::]:8080
2020-04-26T18:44:22Z I! [inputs.snmp_trap] Listening on udp://:162
[root@rott99-app2003 telegraf]#

Exposed metrics: > (with missing timestamp)
CheckPoint_interface_ifAdminStatus{agent_host="10.2x.x.4",host="9803f90fed0a",ifDescr="eth1-01",ifIndex="6",ifName="eth1-01",ifPhysAddress="00:1c:7f:3f:40:eb",ifSpecific=".0.0",sysName="firewall-vsx1-wp-int-test"} 1 1587927480000
CheckPoint_interface_ifAdminStatus{agent_host="10.x.x.24",host="9803f90fed0a",ifDescr="eth1-02",ifIndex="8",ifName="eth1-02",ifPhysAddress="00:1c:7f:3f:40:eb",ifSpecific=".0.0",sysName="firewall-vsx1-wp-int-test"} 1 1587927480000

[danielnelson]

It looks like your example output has timestamps, it's the last value of each line: 1587927480000

[neeles83]

@danielnelson Sorry for the miss conception on my part. You are right, I overlooked that there was a space between the 1 value of the metric and the timestamp. So it's working and you can ignore my comments.