Skip to content

Commit

Permalink
fix(sdk-metrics): use inclusive upper bounds in histogram (#4935)
Browse files Browse the repository at this point in the history 
Co-authored-by: Daniel Patrick <[email protected]>
  • Loading branch information
@legendecas @danielpatrickdotdev
legendecas and danielpatrickdotdev authored Aug 22, 2024
1 parent 0ee398d commit 0b8d310
Show file tree
Hide file tree
Showing 7 changed files with 43 additions and 36 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se
* fix(sdk-node): avoid spurious diag errors for unknown OTEL_NODE_RESOURCE_DETECTORS values [#4879](https://github.com/open-telemetry/opentelemetry-js/pull/4879) @trentm
* deps(opentelemetry-instrumentation): Bump `shimmer` types to 1.2.0 [#4865](https://github.com/open-telemetry/opentelemetry-js/pull/4865) @lforst
* fix(instrumentation): Fix optional property types [#4833](https://github.com/open-telemetry/opentelemetry-js/pull/4833) @alecmev
* fix(sdk-metrics): fix(sdk-metrics): use inclusive upper bounds in histogram [#4829](https://github.com/open-telemetry/opentelemetry-js/pull/4829)

### :books: (Refine Doc)

Expand Down
6 changes: 3 additions & 3 deletions packages/sdk-metrics/src/aggregator/Histogram.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
} from '../export/MetricData';
import { HrTime } from '@opentelemetry/api';
import { InstrumentType } from '../InstrumentDescriptor';
import { binarySearchLB, Maybe } from '../utils';
import { binarySearchUB, Maybe } from '../utils';
import { AggregationTemporality } from '../export/AggregationTemporality';

/**
Expand Down Expand Up @@ -87,8 +87,8 @@ export class HistogramAccumulation implements Accumulation {
this._current.hasMinMax = true;
}

const idx = binarySearchLB(this._boundaries, value);
this._current.buckets.counts[idx + 1] += 1;
const idx = binarySearchUB(this._boundaries, value);
this._current.buckets.counts[idx] += 1;
}

setStartTime(startTime: HrTime): void {
Expand Down
6 changes: 3 additions & 3 deletions packages/sdk-metrics/src/aggregator/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,18 +38,18 @@ export type LastValue = number;
export interface Histogram {
/**
* Buckets are implemented using two different arrays:
* - boundaries: contains every finite bucket boundary, which are inclusive lower bounds
* - boundaries: contains every finite bucket boundary, which are inclusive upper bounds
* - counts: contains event counts for each bucket
*
* Note that we'll always have n+1 buckets, where n is the number of boundaries.
* This is because we need to count events that are below the lowest boundary.
* This is because we need to count events that are higher than the upper boundary.
*
* Example: if we measure the values: [5, 30, 5, 40, 5, 15, 15, 15, 25]
* with the boundaries [ 10, 20, 30 ], we will have the following state:
*
* buckets: {
* boundaries: [10, 20, 30],
* counts: [3, 3, 1, 2],
* counts: [3, 3, 2, 1],
* }
*/
buckets: {
Expand Down
21 changes: 9 additions & 12 deletions packages/sdk-metrics/src/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,30 +165,27 @@ export function setEquals(lhs: Set<unknown>, rhs: Set<unknown>): boolean {
}

/**
* Binary search the sorted array to the find lower bound for the value.
* Binary search the sorted array to the find upper bound for the value.
* @param arr
* @param value
* @returns
*/
export function binarySearchLB(arr: number[], value: number): number {
export function binarySearchUB(arr: number[], value: number): number {
let lo = 0;
let hi = arr.length - 1;
let ret = arr.length;

while (hi - lo > 1) {
const mid = Math.trunc((hi + lo) / 2);
if (arr[mid] <= value) {
lo = mid;
while (hi >= lo) {
const mid = lo + Math.trunc((hi - lo) / 2);
if (arr[mid] < value) {
lo = mid + 1;
} else {
ret = mid;
hi = mid - 1;
}
}

if (arr[hi] <= value) {
return hi;
} else if (arr[lo] <= value) {
return lo;
}
return -1;
return ret;
}

export function equalsCaseInsensitive(lhs: string, rhs: string): boolean {
Expand Down
12 changes: 6 additions & 6 deletions packages/sdk-metrics/test/Instruments.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ describe('Instruments', () => {
0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000,
7500, 10000,
],
counts: [0, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
counts: [1, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
},
count: 2,
sum: 10,
Expand All @@ -341,7 +341,7 @@ describe('Instruments', () => {
0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000,
7500, 10000,
],
counts: [0, 1, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0],
counts: [1, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0],
},
count: 2,
sum: 100,
Expand Down Expand Up @@ -395,7 +395,7 @@ describe('Instruments', () => {
value: {
buckets: {
boundaries: [1, 9, 100],
counts: [1, 0, 0, 1],
counts: [1, 0, 1, 0],
},
count: 2,
sum: 100,
Expand Down Expand Up @@ -484,7 +484,7 @@ describe('Instruments', () => {
0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000,
7500, 10000,
],
counts: [0, 0, 0, 2, 0, 0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0],
counts: [0, 0, 1, 1, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0],
},
count: 4,
sum: 220,
Expand Down Expand Up @@ -534,7 +534,7 @@ describe('Instruments', () => {
0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000,
7500, 10000,
],
counts: [0, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
counts: [0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
},
count: 2,
sum: 10.1,
Expand All @@ -550,7 +550,7 @@ describe('Instruments', () => {
0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000,
7500, 10000,
],
counts: [0, 1, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0],
counts: [0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0],
},
count: 2,
sum: 100.1,
Expand Down
4 changes: 2 additions & 2 deletions packages/sdk-metrics/test/aggregator/Histogram.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ describe('HistogramAggregator', () => {
value: {
buckets: {
boundaries: [1, 10, 100],
counts: [1, 1, 0, 0],
counts: [2, 0, 0, 0],
},
count: 2,
sum: 1,
Expand Down Expand Up @@ -231,7 +231,7 @@ describe('HistogramAggregator', () => {
value: {
buckets: {
boundaries: [1, 10, 100],
counts: [1, 1, 0, 0],
counts: [2, 0, 0, 0],
},
count: 2,
sum: 1,
Expand Down
29 changes: 19 additions & 10 deletions packages/sdk-metrics/test/utils.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import * as sinon from 'sinon';
import * as assert from 'assert';
import {
binarySearchLB,
binarySearchUB,
callWithTimeout,
hashAttributes,
TimeoutError,
Expand Down Expand Up @@ -66,26 +66,35 @@ describe('utils', () => {
});
});

describe('binarySearchLB', () => {
describe('binarySearchUB', () => {
const tests = [
/** [ arr, value, expected lb idx ] */
[[0, 10, 100, 1000], -1, -1],
[[0, 10, 100, 1000], -1, 0],
[[0, 10, 100, 1000], 0, 0],
[[0, 10, 100, 1000], 1, 0],
[[0, 10, 100, 1000], 1, 1],
[[0, 10, 100, 1000], 10, 1],
[[0, 10, 100, 1000], 100, 2],
[[0, 10, 100, 1000], 101, 3],
[[0, 10, 100, 1000], 1000, 3],
[[0, 10, 100, 1000], 1001, 3],
[[0, 10, 100, 1000], 1001, 4],

[[0, 10, 100, 1000, 10_000], -1, -1],
[[0, 10, 100, 1000, 10_000], -1, 0],
[[0, 10, 100, 1000, 10_000], 0, 0],
[[0, 10, 100, 1000, 10_000], 10, 1],
[[0, 10, 100, 1000, 10_000], 1001, 3],
[[0, 10, 100, 1000, 10_000], 10_001, 4],
[[0, 10, 100, 1000, 10_000], 100, 2],
[[0, 10, 100, 1000, 10_000], 101, 3],
[[0, 10, 100, 1000, 10_000], 1001, 4],
[[0, 10, 100, 1000, 10_000], 10_001, 5],

[[], 1, 0],
[[1], 0, 0],
[[1], 1, 0],
[[1], 2, 1],
] as [number[], number, number][];

for (const [idx, test] of tests.entries()) {
it(`test idx(${idx}): find lb of ${test[1]} in [${test[0]}]`, () => {
assert.strictEqual(binarySearchLB(test[0], test[1]), test[2]);
it(`test idx(${idx}): find ub of ${test[1]} in [${test[0]}]`, () => {
assert.strictEqual(binarySearchUB(test[0], test[1]), test[2]);
});
}
});
Expand Down

0 comments on commit 0b8d310

Please sign in to comment.