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

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

Closed
zkx5xkt opened this issue Feb 26, 2024 · 4 comments · Fixed by #620

Comments

@zkx5xkt
Copy link

zkx5xkt commented Feb 26, 2024

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
Copy link
Contributor

psimk commented Mar 26, 2024

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
Copy link
Contributor

psimk commented Mar 26, 2024

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
Copy link
Collaborator

SimenB commented Mar 26, 2024

Happy to take a PR fixing this 🙂

@SimenB
Copy link
Collaborator

SimenB commented Mar 26, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants