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.

Sign up for GitHub

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

Jump to bottom

improve performance of registry.metrics() #373

Closed
wants to merge 5 commits into from
Closed

improve performance of registry.metrics() #373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
266dcd7
improve performance of registry.metrics()
SerayaEryn Jun 16, 2019
7227147
Move Metric `type` to prototype since it's invariant
doochik May 17, 2020
3c4c748
Generate function without external dependencies
doochik May 17, 2020
a1a3675
Fix big with label="undefined"
doochik May 18, 2020
3c0ad3a
CHANGELOG.md
doochik May 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ project adheres to [Semantic Versioning](http://semver.org/).
- chore: refactor metrics to reduce code duplication
- chore: replace `utils.getPropertiesFromObj` with `Object.values`
- chore: remove unused `catch` bindings
- chore: improve performance of `registry.metrics()`

### Added

Expand Down
5 changes: 3 additions & 2 deletions lib/counter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
'use strict';

const util = require('util');
const type = 'counter';
const { hashObject, isObject, getLabels, removeLabels } = require('./util');
const { validateLabel } = require('./validation');
const { Metric } = require('./metric');
Expand Down Expand Up @@ -37,7 +36,7 @@ class Counter extends Metric {
return {
help: this.help,
name: this.name,
type,
type: this.type,
values: Object.values(this.hashMap),
aggregator: this.aggregator,
};
Expand All @@ -58,6 +57,8 @@ class Counter extends Metric {
}
}

Counter.prototype.type = 'counter';

const reset = function () {
this.hashMap = {};

Expand Down
84 changes: 84 additions & 0 deletions lib/formatter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
'use strict';

function getFormatter(metric, defaultLabels) {
const defaultLabelNames = Object.keys(defaultLabels);
const name = metric.name;
const help = escapeString(metric.help);
const type = metric.type;
const labelsCode = getLabelsCode(metric, defaultLabelNames, defaultLabels);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⭐ This is going to make it impossible to implement #298/landing #368. As you can tell from the comments in those, I'm not certain what the fate of that issue is though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify what do you mean? Should I rewrite it or implement inside generated function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zbjornson can you take a look on this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the nudge, will review this PR this weekend.

On this particular comment: I'd (strongly) lean toward making the formatter work with dynamically declared labels. #298 has quite a bit of support so I suspect #368 will land. The main (probably minor) thing holding it up is #298 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This eliminate the main idea of formatter - build labels string once.
I need to think how to rewrite this.


// eslint-disable-next-line no-new-func
return new Function(
'item',
'escapeLabelValue',
'getValueAsString',
`
const info = '# HELP ${name} ${help}\\n# TYPE ${name} ${type}\\n';
let values = '';
for (const val of item.values || []) {
val.labels = val.labels || {};
let metricName = val.metricName || '${name}';
const labels = \`${labelsCode}\`;
const hasLabels = labels.length > 0;
metricName += \`\${hasLabels ? '{' : ''}\${labels}\${hasLabels ? '}' : ''}\`;
let line = \`\${metricName} \${getValueAsString(val.value)}\`;
values += \`\${line}\\n\`;
}
return info + values;
`,
);
}

function getLabelsCode(metric, defaultLabelNames, defaultLabels) {
const labelString = [];
const labelNames = getLabelNames(metric, defaultLabelNames);
for (let index = 0; index < labelNames.length; index++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⭐ It's valid to only set values for a subset of labels when a metric is updated. With this test:

it('should allow subsetse of labels', () => {
	instance = new Gauge({
		name: 'name_2',
		help: 'help',
		labelNames: ['code', 'success'],
	});
	instance.inc({ code: '200' }, 10);
	const str = globalRegistry.getSingleMetricAsString('name_2');
	console.log(str);
});

in this PR, you get

# HELP name_2 help
# TYPE name_2 gauge
name_2{code="200",success="undefined"} 10

whereas in master, you get

# HELP name_2 help
# TYPE name_2 gauge
name_2{code="200"} 10

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should add this as a test, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course. I’ll fix it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above test case passes, but now if the first label is undefined, there's a stray leading comma:

it.only('should allow combinations of labels', () => {
	instance = new Gauge({
		name: 'name_2',
		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 = globalRegistry.getSingleMetricAsString('name_2');
	console.log(str);
});

prints

      # HELP name_2 help
      # TYPE name_2 gauge
      name_2{code="200"} 10
      name_2{code="200",ok="yes"} 10
      name_2{,success="false",ok="yes"} 10
      name_2{,success="false",ok="no"} 10

(notice last two lines)

Something like these should both be committed as test cases. Do you feel like adding them please?

Just checked, benchmarks still look significantly faster despite the new branch added in the last commits. 👍

const comma = index > 0 ? ',' : '';
const labelName = labelNames[index];
if (labelName === 'quantile') {
labelString.push(
`\${val.labels.quantile != null ? \`${comma}quantile="\${escapeLabelValue(val.labels.quantile)}"\` : ''}`,
);
} else if (labelName === 'le') {
labelString.push(
`\${val.labels.le != null ? \`${comma}le="\${escapeLabelValue(val.labels.le)}"\` : ''}`,
);
} else {
const defaultLabelValue = defaultLabels[labelName];
if (typeof defaultLabelValue === 'undefined') {
labelString.push(
`\${val.labels['${labelName}'] != null ? \`${comma}${labelName}="\${escapeLabelValue(val.labels['${labelName}'])}"\` : ''}`,
);
} else {
labelString.push(
`${comma}${labelName}="\${escapeLabelValue(val.labels['${labelName}'] || '${defaultLabelValue}')}"`,
);
}
}
}
return labelString.join('');
}

function getLabelNames(metric, defaultLabelNames) {
const labelNames = [...metric.labelNames];
for (const labelName of defaultLabelNames) {
if (!labelNames.includes(labelName)) {
labelNames.push(labelName);
}
}
if (metric.type === 'summary') {
labelNames.push('quantile');
}
if (metric.type === 'histogram') {
labelNames.push('le');
}
return labelNames;
}

function escapeString(str) {
return str.replace(/\n/g, '\\\\n').replace(/\\(?!n)/g, '\\\\\\\\');
}

module.exports = {
getFormatter,
};
5 changes: 3 additions & 2 deletions lib/gauge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
'use strict';

const util = require('util');
const type = 'gauge';

const {
setValue,
Expand Down Expand Up @@ -85,7 +84,7 @@ class Gauge extends Metric {
return {
help: this.help,
name: this.name,
type,
type: this.type,
values: Object.values(this.hashMap),
aggregator: this.aggregator,
};
Expand Down Expand Up @@ -113,6 +112,8 @@ class Gauge extends Metric {
}
}

Gauge.prototype.type = 'gauge';

function setToCurrentTime(labels) {
return () => {
const now = Date.now() / 1000;
Expand Down
5 changes: 3 additions & 2 deletions lib/histogram.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
'use strict';

const util = require('util');
const type = 'histogram';
const { getLabels, hashObject, isObject, removeLabels } = require('./util');
const { validateLabel } = require('./validation');
const { Metric } = require('./metric');
Expand Down Expand Up @@ -59,7 +58,7 @@ class Histogram extends Metric {
return {
name: this.name,
help: this.help,
type,
type: this.type,
values,
aggregator: this.aggregator,
};
Expand Down Expand Up @@ -100,6 +99,8 @@ class Histogram extends Metric {
}
}

Histogram.prototype.type = 'histogram';

function startTimer(startLabels) {
return () => {
const start = process.hrtime();
Expand Down
11 changes: 10 additions & 1 deletion lib/metric.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
'use strict';

const { globalRegistry } = require('./registry');
const { isObject } = require('./util');
const { isObject, getValueAsString, escapeLabelValue } = require('./util');
const { getFormatter } = require('./formatter');
const { validateMetricName, validateLabelName } = require('./validation');

/**
Expand Down Expand Up @@ -46,6 +47,14 @@ class Metric {
}
}

getPrometheusString(defaultLabels) {
const item = this.get();
if (!this.formatter) {
this.formatter = getFormatter(this, defaultLabels);
}
return this.formatter(item, escapeLabelValue, getValueAsString);
}

reset() {
/* abstract */
}
Expand Down
52 changes: 2 additions & 50 deletions lib/registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,4 @@
'use strict';
const { getValueAsString } = require('./util');

function escapeString(str) {
return str.replace(/\n/g, '\\n').replace(/\\(?!n)/g, '\\\\');
}
function escapeLabelValue(str) {
if (typeof str !== 'string') {
return str;
}
return escapeString(str).replace(/"/g, '\\"');
}

class Registry {
constructor() {
Expand All @@ -23,44 +12,7 @@ class Registry {
}

getMetricAsPrometheusString(metric) {
const item = metric.get();
const name = escapeString(item.name);
const help = `# HELP ${name} ${escapeString(item.help)}`;
const type = `# TYPE ${name} ${item.type}`;
const defaultLabelNames = Object.keys(this._defaultLabels);

let values = '';
for (const val of item.values || []) {
val.labels = val.labels || {};

if (defaultLabelNames.length > 0) {
// Make a copy before mutating
val.labels = Object.assign({}, val.labels);

for (const labelName of defaultLabelNames) {
val.labels[labelName] =
val.labels[labelName] || this._defaultLabels[labelName];
}
}

let metricName = val.metricName || item.name;

const keys = Object.keys(val.labels);
const size = keys.length;
if (size > 0) {
let labels = '';
let i = 0;
for (; i < size - 1; i++) {
labels += `${keys[i]}="${escapeLabelValue(val.labels[keys[i]])}",`;
}
labels += `${keys[i]}="${escapeLabelValue(val.labels[keys[i]])}"`;
metricName += `{${labels}}`;
}

values += `${metricName} ${getValueAsString(val.value)}\n`;
}

return `${help}\n${type}\n${values}`.trim();
return metric.getPrometheusString(this._defaultLabels);
}

metrics() {
Expand All @@ -69,7 +21,7 @@ class Registry {
this.collect();

for (const metric of this.getMetricsAsArray()) {
metrics += `${this.getMetricAsPrometheusString(metric)}\n\n`;
metrics += `${this.getMetricAsPrometheusString(metric)}\n`;
}

return metrics.substring(0, metrics.length - 1);
Expand Down
5 changes: 3 additions & 2 deletions lib/summary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
'use strict';

const util = require('util');
const type = 'summary';
const { getLabels, hashObject, removeLabels } = require('./util');
const { validateLabel } = require('./validation');
const { Metric } = require('./metric');
Expand Down Expand Up @@ -61,7 +60,7 @@ class Summary extends Metric {
return {
name: this.name,
help: this.help,
type,
type: this.type,
values,
aggregator: this.aggregator,
};
Expand Down Expand Up @@ -104,6 +103,8 @@ class Summary extends Metric {
}
}

Summary.prototype.type = 'summary';

function extractSummariesForExport(summaryOfLabels, percentiles) {
summaryOfLabels.td.compress();

Expand Down
9 changes: 9 additions & 0 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
'use strict';

const QUOTE_RE = /"/g;

exports.escapeLabelValue = function escapeLabelValue(str) {
if (typeof str !== 'string') {
return str;
}
return str.replace(QUOTE_RE, '\\"');
};

exports.getValueAsString = function getValueString(value) {
if (Number.isNaN(value)) {
return 'Nan';
Expand Down
Loading