Not all the default collectors respect the timestamps: false configuration

[sam-github]

The workaround for #177 is

To disable metric timestamps set timestamps to false (You can find the list of metrics that support this feature in test/defaultMetricsTest.js):

• https://github.com/siimon/prom-client#default-metrics

But not all metrics support this, these did not:

# HELP process_cpu_user_seconds_total Total user CPU time spent in seconds.
# TYPE process_cpu_user_seconds_total counter
process_cpu_user_seconds_total 4.900423 1578088304120
# HELP process_cpu_system_seconds_total Total system CPU time spent in seconds.
# TYPE process_cpu_system_seconds_total counter
process_cpu_system_seconds_total 0.440623 1578088304120
# HELP process_cpu_seconds_total Total user and system CPU time spent in seconds.
# TYPE process_cpu_seconds_total counter
process_cpu_seconds_total 5.3410459999999995 1578088304120
# HELP process_resident_memory_bytes Resident memory size in bytes.
# TYPE process_resident_memory_bytes gauge
process_resident_memory_bytes 97550336 1578088306184
# HELP process_virtual_memory_bytes Virtual memory size in bytes.
# TYPE process_virtual_memory_bytes gauge
process_virtual_memory_bytes 789307392 1578088306184
# HELP process_heap_bytes Process heap size in bytes.
# TYPE process_heap_bytes gauge
process_heap_bytes 146132992 1578088306184
# HELP process_open_fds Number of open file descriptors.
# TYPE process_open_fds gauge
process_open_fds 310 1578088304595
# HELP nodejs_eventloop_lag_seconds Lag of event loop in seconds.
# TYPE nodejs_eventloop_lag_seconds gauge
nodejs_eventloop_lag_seconds 0.490566822 1578088304611
# HELP nodejs_active_requests_total Total number of active requests.
# TYPE nodejs_active_requests_total gauge
nodejs_active_requests_total 131 1578088304121
# HELP nodejs_heap_space_size_total_bytes Process heap space size total from node.js in bytes.
# TYPE nodejs_heap_space_size_total_bytes gauge
nodejs_heap_space_size_total_bytes{space="read_only"} 262144 1578088304121
nodejs_heap_space_size_total_bytes{space="new"} 33554432 1578088304121
nodejs_heap_space_size_total_bytes{space="old"} 18722816 1578088304121
nodejs_heap_space_size_total_bytes{space="code"} 1474560 1578088304121
nodejs_heap_space_size_total_bytes{space="map"} 790528 1578088304121
nodejs_heap_space_size_total_bytes{space="large_object"} 2813952 1578088304121
nodejs_heap_space_size_total_bytes{space="code_large_object"} 49152 1578088304121
nodejs_heap_space_size_total_bytes{space="new_large_object"} 0 1578088304121
# HELP nodejs_heap_space_size_used_bytes Process heap space size used from node.js in bytes.
# TYPE nodejs_heap_space_size_used_bytes gauge
nodejs_heap_space_size_used_bytes{space="read_only"} 32296 1578088304121
nodejs_heap_space_size_used_bytes{space="new"} 1779952 1578088304121
nodejs_heap_space_size_used_bytes{space="old"} 13502648 1578088304121
nodejs_heap_space_size_used_bytes{space="code"} 1078048 1578088304121
nodejs_heap_space_size_used_bytes{space="map"} 584960 1578088304121
nodejs_heap_space_size_used_bytes{space="large_object"} 2794456 1578088304121
nodejs_heap_space_size_used_bytes{space="code_large_object"} 3552 1578088304121
nodejs_heap_space_size_used_bytes{space="new_large_object"} 0 1578088304121
# HELP nodejs_heap_space_size_available_bytes Process heap space size available from node.js in bytes.
# TYPE nodejs_heap_space_size_available_bytes gauge
nodejs_heap_space_size_available_bytes{space="read_only"} 229576 1578088304121
nodejs_heap_space_size_available_bytes{space="new"} 14979856 1578088304121
nodejs_heap_space_size_available_bytes{space="old"} 5191736 1578088304121
nodejs_heap_space_size_available_bytes{space="code"} 322784 1578088304121
nodejs_heap_space_size_available_bytes{space="map"} 204160 1578088304121
nodejs_heap_space_size_available_bytes{space="large_object"} 0 1578088304121
nodejs_heap_space_size_available_bytes{space="code_large_object"} 0 1578088304121
nodejs_heap_space_size_available_bytes{space="new_large_object"} 16759808 1578088304121

I think this is a known bug, its mentioned in the tests, but I couldn't find an issue for it. Sorry if its a duplicate.

• prom-client/test/defaultMetricsTest.js

  Lines 74 to 79 in cc9ce78

  	it('should omit timestamp in certain metrics', () => {
  	const metricsWithToggableTimestamp = [
  	'nodejs_active_handles_total',
  	'nodejs_external_memory_bytes',
  	'nodejs_heap_size_total_bytes',
  	'nodejs_heap_size_used_bytes'

[SimenB]

I never noticed that when merging #255… @dolchi21 do you remember if there's any reason not all metrics support this?

[sam-github]

The README makes ref to the unit tests to see which ones support stripping timestamps, I suspect that it was just that since it took custom modification of each metric, it never happened.

Perhaps that means it should be implemented differently? That the registry should itself know that the timestamps are not desired, and thus ignore the timestamp passed in from the collector? Just a suggestion, I haven't had time to look closely at that part of the code, though I've read through many of the default collectors by now. Its not completely clear to me that any timestamp support at all is needed, unless its intended for batch processing jobs?

[SimenB]

It's for batch and push gateways, yeah. They're generally discouraged though.

Happy to take a PR disabling timestamps on a registry level, that makes sense to me. @siimon @zbjornson thoughts?

[zbjornson]

Given this from #177

It looks like this client does not support custom collectors as it doesn't support collectors (it's all metric based), so as it stands there's no need for any timestamp-related functionality.

I thought we should just remove all timestamp code from this repo?

[sam-github]

Options are:

1. remove all timestamp code from this repo, cf
2. fix the metrics that don't respect {timestamps: false}
3. rework code so that timestamps are applied or removed at the registry level

I think 1. is probably the right way to go, but don't want to write an unlandable PR.

@SimenB @zbjornson Should I remove the timestamping?

If so, I assume its supported in the low level gauges/histograms/etc. -- should it get removed from there so its not possible for anyone to do anymore, or just remove it from all the built in metrics?

[zbjornson]

#1 is my vote, yeah, and I think we might as well remove it all the way down to the lowest level. No point in having unused/unusable code.

Appreciate all your recent work!

[sam-github]

I've a branch that will fix this, #177 and #289 simultaneously by removing all timestamp support, but I don't want to pile PRs on too deep, I'll wait until #330 and then #229 are merged, then PR the fix for this.

[SimenB]

#333