-
Notifications
You must be signed in to change notification settings - Fork 378
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
perf: improve performance of rendering metrics to Prometheus string #549
Conversation
I see that we actually ban people from passing in |
Thanks for sending this PR @ngavalas! Could you rebase now Really excited about the results of this PR 👍 |
Yup will rebase this morning, thanks. |
Rebased and tests + benchmark passed, but I'm going to read the merged code carefully to be triple sure. Two things I can think of after reading this more:
|
|
It doesn't break, it just ignores any mutation after you render it to the prom string one time. This is like an edge case of an edge case and I don't feel bad about it not working right in that case. As for (1), I am almost positive it doesn't violate the spec. Will work to confirm on my end further. The only time it swaps order is if the label moves from passed in to a default label on the same metric. I don't even know if this is possible. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
One idea below, feel free to decline.
These benchmarks are so noisy 😞 hard to detect anything with less than ~15% improvement.
lib/registry.js
Outdated
|
||
const formattedLabels = formatLabels(labels); | ||
const flattened = formattedLabels.join(','); | ||
Object.defineProperty(labels, '__flattened_internal', { value: flattened }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could use a WeakMap here instead of defining a property.
diff --git a/lib/registry.js b/lib/registry.js
index 2169b66..01b0f8e 100644
--- a/lib/registry.js
+++ b/lib/registry.js
@@ -219,14 +219,17 @@ function formatLabels(labels) {
);
}
+const cache = new WeakMap();
+
function flattenSharedLabels(labels) {
- if (labels.__flattened_internal) {
- return labels.__flattened_internal;
+ const cached = cache.get(labels);
+ if (cached) {
+ return cached;
}
const formattedLabels = formatLabels(labels);
const flattened = formattedLabels.join(',');
- Object.defineProperty(labels, '__flattened_internal', { value: flattened });
+ cache.set(labels, flattened);
return flattened;
}
function escapeLabelValue(str) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's a good idea. Let me fix up the changelog and make that change tomorrow morning.
Rendering histograms, particularly in
getMetricAsPrometheusString
, repeats most of the work of label rendering over and over and over and over again on the same strings. This is because we currently fully copy all labels out onto all buckets and then format them all separately as if they weren't the same labelset to begin with.This PR takes a different approach:
histogram
(specifically, thele
label, but this would work for other ones too).get
, we merge them back in so the API is unchanged. No harm, no foul.getMetricAsPrometheusString
can encode and stringify the shared set of original labels exactly once.le
label. Deferring this check until serialization time is faster even if the code is slightly more complex.Here are the benchmark results:
This should also help #543.
[1] Technically, the order of some labels on histograms changes now depending on where they came from. You can see this in the histogram test case, which I needed to tweak. Label order stability doesn't seem super important, but I can try to fix this if anyone feels really strongly. It'll be messy and JS objects aren't really ordered consistently across older node versions anyway if I remember correctly.