siimon / prom-client

Prometheus client for node.js
Apache License 2.0
3.15k stars 378 forks source link

Exemplar still gets updated(to one with empty labels) when providing no exemplar labels with the sample #616

Closed zkx5xkt closed 8 months ago

zkx5xkt commented 9 months ago

I have a setup where I only upload a small percentage of my traces. This means that for majority of my samples in a histogram there won't be a corresponding trace.

Current library implementation:

When presented with a new sample with no exemplar labels the library overrides the bucket's exemplar labels with an empty object essentially purging the existing exemplar

Example of metric after sample is published with an exemplar labels: example_duration_bucket{le="5"} 1 # {traceID="9899898"} 3.0005617 1708951967.457 Example of metric after sample is published with no exemplar labels afterwards: example_duration_bucket{le="5"} 1 # {} 3.0000431 1708975949.123

Desired library implementation:

When presented with a new sample with no exemplar labels the library should retain the existing exemplar so it can be scraped with the metric.

Example of metric after sample is published with an exemplar labels: example_duration_bucket{le="5"} 1 # {traceID="9899898"} 3.0005617 1708951967.457 Example of metric after sample is published with no exemplar labels afterwards: example_duration_bucket{le="5"} 1 # {traceID="9899898"} 3.0005617 1708951967.457

Example

import { Span, TraceFlags } from "@opentelemetry/api";
import { setTimeout } from "node:timers/promises";
import { Histogram } from "prom-client";

const histogram = new Histogram({
    name: "example_duration",
    help: "example duration",
    buckets: [0.5, 5],
    labelNames: [],
    enableExemplars: true,
});

async function example(span: Span) {
    const spanContext = span.spanContext();

    const exemplarLabels = spanContext.traceFlags & TraceFlags.SAMPLED ? {
        traceID: spanContext.traceId,
        spanID: spanContext.spanId
    } : undefined;

    const histogramTimer = histogram.startTimer(undefined, undefined);

    await setTimeout(3_000);

    histogramTimer(undefined, exemplarLabels);
}

currently for prom-client@15.1.0 i'm applying the following hotpatch for Histogram (do not overwrite exemplar values when no exemplar labels given):

--- node_modules/prom-client/lib/histogram.js   2024-02-26 06:50:35.132071300 +0200
+++ node_modules/prom-client/lib/histogram.js   2024-02-26 07:09:28.088071300 +0200
@@ -89,14 +89,16 @@
        const bound = findBound(this.upperBounds, value);
        const { bucketExemplars } = this.hashMap[hash];
        let exemplar = bucketExemplars[bound];
-       if (!isObject(exemplar)) {
-           exemplar = new Exemplar();
-           bucketExemplars[bound] = exemplar;
+       if (Object.keys(exemplarLabels).length) {
+           if (!isObject(exemplar)) {
+               exemplar = new Exemplar();
+               bucketExemplars[bound] = exemplar;
+           }
+           exemplar.validateExemplarLabelSet(exemplarLabels);
+           exemplar.labelSet = exemplarLabels;
+           exemplar.value = value;
+           exemplar.timestamp = nowTimestamp();
        }
-       exemplar.validateExemplarLabelSet(exemplarLabels);
-       exemplar.labelSet = exemplarLabels;
-       exemplar.value = value;
-       exemplar.timestamp = nowTimestamp();
    }

    async get() {

This can probably be done in a backwards compatible manner with an additional flag or something to specifically disable the overriding of exemplars when no exemplar is provided.

# prom-client version: 15.1.0

psimk commented 8 months ago

same issue is present for counters as well, I created a similar patch for anyone wanting this feature:

diff --git a/lib/counter.js b/lib/counter.js
index 22a440eca40ed13d088789903eb4c04cc70e8128..8c5c1b0d9f2c2134460c248fbc181881cbaef063 100644
--- a/lib/counter.js
+++ b/lib/counter.js
@@ -85,9 +85,12 @@ class Counter extends Metric {
    }

    updateExemplar(exemplarLabels, value, hash) {
+       if (Object.keys(exemplarLabels).length === 0) return;
+
        if (!isObject(this.hashMap[hash].exemplar)) {
            this.hashMap[hash].exemplar = new Exemplar();
        }
+
        this.hashMap[hash].exemplar.validateExemplarLabelSet(exemplarLabels);
        this.hashMap[hash].exemplar.labelSet = exemplarLabels;
        this.hashMap[hash].exemplar.value = value ? value : 1;
psimk commented 8 months ago

I also noticed that Go's client lib already does this by default https://github.com/prometheus/client_golang/blob/26e3055e5133a9d64e8e5a07a7cf026875d5f55d/prometheus/counter.go#L172.

SimenB commented 8 months ago

Happy to take a PR fixing this 🙂

SimenB commented 8 months ago

https://github.com/siimon/prom-client/releases/tag/v15.1.1