Skip to content

Commit

Permalink
In gauge.inc/dec, default to 1 only if no arg is passed
Browse files Browse the repository at this point in the history
Calling `.inc(value)` with a dynamic value that may be 0 or 1 will always cause to increase metrics by 1. This is very problematic and may distort the metrics in a non obvious way
  • Loading branch information
dapplion authored and zbjornson committed Aug 1, 2021
1 parent a972f0c commit 83cb173
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 2 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -19,6 +19,7 @@ project adheres to [Semantic Versioning](http://semver.org/).
### Added

- feat: added `zero()` to `Histogram` for setting the metrics for a given label combination to zero
- fix: allow `Gauge.inc/dec(0)` without defaulting to 1

## [13.1.0] - 2021-01-24

Expand Down
6 changes: 4 additions & 2 deletions lib/gauge.js
Expand Up @@ -146,14 +146,16 @@ function startTimer(startLabels) {
function dec(labels) {
return value => {
const data = convertLabelsAndValues(labels, value);
this.set(data.labels, this._getValue(data.labels) - (data.value || 1));
if (data.value === undefined) data.value = 1;
this.set(data.labels, this._getValue(data.labels) - data.value);
};
}

function inc(labels) {
return value => {
const data = convertLabelsAndValues(labels, value);
this.set(data.labels, this._getValue(data.labels) + (data.value || 1));
if (data.value === undefined) data.value = 1;
this.set(data.labels, this._getValue(data.labels) + data.value);
};
}

Expand Down
10 changes: 10 additions & 0 deletions test/gaugeTest.js
Expand Up @@ -41,6 +41,16 @@ describe('gauge', () => {
await expectValue(5);
});

it('should set to exactly zero without defaulting to 1', async () => {
instance.set(0);
await expectValue(0);
});

it('should inc by zero without defaulting to 1', async () => {
instance.inc(0);
await expectValue(10);
});

it('should start a timer and set a gauge to elapsed in seconds', async () => {
jest.useFakeTimers('modern');
jest.setSystemTime(0);
Expand Down

0 comments on commit 83cb173

Please sign in to comment.