Closed ngavalas closed 1 year ago
I see that we actually ban people from passing in le
in histograms, so technically there should be no conflicts in that particular case, but it does seem safer to just keep the extra collision check in registry.js
so we don't double apply labels ever.
Thanks for sending this PR @ngavalas! Could you rebase now next
has landed on master
?
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:
1) I am pretty sure label order doesn't strictly matter. In any case, this change only affects label order in a "breaking" way across the old version and the new version; within a single scrape it should be consistent.
2) What happens if someone passes in a labels
object and then mutates it outside of the library? This won't play nicely with my memoization (__flattened_internal
) but I want to avoid making an unnecessary copy if things already get weird if people do that, which I suspect is the case
Object.freeze
. If any mutation is just ignored, I think that's fine.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. :)
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: 1) The labels we're passed in are stored separately from the ones automatically added by
histogram
(specifically, thele
label, but this would work for other ones too). 2) If we get asked forget
, we merge them back in so the API is unchanged. No harm, no foul. 3) The fast path returns the shared labels separately so thatgetMetricAsPrometheusString
can encode and stringify the shared set of original labels exactly once. 4) When rendering the rest of the labels to strings, we check and make sure they don't exist in the shared set first. This is because in the original code, the provided set of labels takes precedence over the "internal"le
label. Deferring this check until serialization time is faster even if the code is slightly more complex. 5) We append the shared label set onto the end of the internal label set and bam, it's like nothing ever happened.[1]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.