Calling GET /metrics results in clearing all cached metrics

[MeenaAlfons]

This snippet is taken from reportMetrics() which is called upon receiving a request to GET /metrics.

node-prom-pushgateway/lib/gateway.js

Lines 181 to 196 in f8e56cd

	// TODO: skip too-old metrics (report to process output), it confuses prometheus
	// TODO: hold "too new" samples for the next scrape (ie samples that are from the next cluster)
	// TODO: report metrics with a lag (guard band), to not split clusters of samples
	samples = samples.splice(0);
	// discard samples older than the configured cutoff
	if (this.maxMetricAgeMs >= 0) {
	var now = this.getTimestamp();
	var oldestKeepTs = now - this.maxMetricAgeMs;
	for (i=0; i<samples.length && samples[i].ts < oldestKeepTs; i++) ;
	if (i > 0) {
	Gateway.trace("discarding %d samples older than %d ms, from %d to %d",
	i, this.maxMetricAgeMs, samples[0].ts, samples[i-1].ts);
	samples = samples.slice(i);
	}
	}

In this part, samples.splice(0) results in removing all stored samples in this.samples. I don't get why this is needed. There is already a mechanism to get rid of old samples using maxMetricAgeMs.

Here is more context on why it doesn't make sense to remove cached metrics:

• prom-pushgateway should serve as a caching layer.
• Metrics will be pushed to prom-pushgateway. pushing a new value for metric_1{<label_set>} should overwrite any previous value for that metric and label set. However, pushing a new metric metric_2 should not result in removing metric_1.
• Reporting metrics via GET /metrics should not affect the cached metrics.
• The line samples.splice(0) results in clearing all cached metrics when GET /metrics is called!

I appreciate your inputs on this and I'm happy to open a PR with a solution.

[MeenaAlfons]

After reading the rest of the code, I found out that older values for metrics are kept in this.previousValues which acts as the cache in this case. samples are just the set of samples since the last (scrape) call to reportMetrics. Those samples are averaged and added to the cached values.

In that since, I believe maxMetricAgeMs doesn't actually do what it suggests. The old metrics in this.previousValues are never checked or removed.

[MeenaAlfons]

I think we need to change the following code with the code suggested afterwards.

Current code:

node-prom-pushgateway/lib/gateway.js

Lines 213 to 218 in f8e56cd

	// never omit any known metrics, report the previous value if no change
	// This allows /metrics to be called more frequently than the /push interval.
	var allValues = {};
	for (id in this.previousValues) allValues[id] = this.previousValues[id];
	for (id in map) allValues[id] = map[id];
	this.previousValues = allValues;

Suggested code:

    // never omit any known metrics, report the previous value if no change
    // This allows /metrics to be called more frequently than the /push interval.
    var allValues = {};
    const now = this.getTimestamp();
    const oldestKeepTs = this.maxMetricAgeMs >= 0 ? (now - this.maxMetricAgeMs) : 0;
    const filterOldMetrics = this.maxMetricAgeMs >= 0;
    for (id in this.previousValues) {
        if (!filterOldMetrics || this.previousValues[id].ts >= oldestKeepTs) {
            allValues[id] = this.previousValues[id];
        }
    }
    const discardedMetrics = Object.keys(this.previousValues).length - Object.keys(allValues).length;
    if (discardedMetrics > 0) {
        Gateway.trace("discarding %d metrics older than %d ms", discardedMetrics, this.maxMetricAgeMs);
    }
    for (id in map) allValues[id] = map[id];
    this.previousValues = allValues;