Closed yosiat closed 9 months ago
zbjornson
commented
10 months ago Thanks for the PR. This has been proposed before in #373 and would make it impossible to implement #298/#368. See also this comment about validly accepting subsets of labels: https://github.com/siimon/prom-client/pull/373#discussion_r426204193.
yosiat
commented
10 months ago @zbjornson thanks for the feedback.
First regarding the tests example in the comments:
const prom = require('.');
async function test1() {
instance = new prom.Gauge({
name: 'name_2',
help: 'help',
labelNames: ['code', 'success'],
});
instance.inc({ code: '200' }, 10);
const str = await prom.Registry.globalRegistry.getSingleMetricAsString('name_2');
console.log(str);
}
async function test2() {
instance = new prom.Gauge({
name: 'test',
help: 'help',
labelNames: ['code', 'success', 'ok'],
});
instance.inc({ code: '200' }, 10);
instance.inc({ code: '200', ok: 'yes' }, 10);
instance.inc({ ok: 'yes', success: 'false' }, 10);
instance.inc({ success: 'false', ok: 'no' }, 10);
const str = await prom.Registry.globalRegistry.getSingleMetricAsString('test');
console.log(str);
}
async function main() {
await test1();
console.log("\n\n");
await test2();
}
main();I am getting this output:
# HELP name_2 help
# TYPE name_2 gauge
name_2{code="200"} 10
# HELP test help
# TYPE test gauge
test{code="200"} 10
test{code="200",ok="yes"} 10
test{ok="yes",success="false"} 10
test{success="false",ok="no"} 10Which is the exact same output as master branch.
Regarding support of dynamic labels, I have strong opinion against this, since it can lead to a mess in metrics on large-scale codebases and having labels predefined gives more strictness.
But, I don't think my PR is blocking having dynamic labels - see hashObject - if I don't get labelNames I fallback to extracting them like the old code.
Please re-consider this change, and I am happy to get more feedback here.
zbjornson
commented
10 months ago if I don't get
labelNamesI fallback to extracting them like the old code.
My bad, I missed the fallback. That's a great solution then.
Can you add a changelog entry please?
yosiat
commented
10 months ago @zbjornson added changelog entry, let me know if it's ok.
yosiat
commented
10 months ago @zbjornson @SimenB after addressing your questions & concerns, can we move forward with this PR? or there is something left here?
SimenB
commented
9 months ago
Since label names are predefined ahead-of-time and constant during runtime, we can sort them before-hand and use them when running
hashObject.Benchmarks
Running on node v18.18.2. Note: the registry benchmarks I couldn't run because of OOM (and on master branch as well).