From 0b8d310cf5fc62fa341337f4212cba035b7b851b Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Thu, 22 Aug 2024 10:12:59 +0100 Subject: [PATCH] fix(sdk-metrics): use inclusive upper bounds in histogram (#4935) Co-authored-by: Daniel Patrick --- CHANGELOG.md | 1 + .../sdk-metrics/src/aggregator/Histogram.ts | 6 ++-- packages/sdk-metrics/src/aggregator/types.ts | 6 ++-- packages/sdk-metrics/src/utils.ts | 21 ++++++-------- packages/sdk-metrics/test/Instruments.test.ts | 12 ++++---- .../test/aggregator/Histogram.test.ts | 4 +-- packages/sdk-metrics/test/utils.test.ts | 29 ++++++++++++------- 7 files changed, 43 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cde506c375..e4042573d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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) diff --git a/packages/sdk-metrics/src/aggregator/Histogram.ts b/packages/sdk-metrics/src/aggregator/Histogram.ts index 5c9390b81f..85f46098e9 100644 --- a/packages/sdk-metrics/src/aggregator/Histogram.ts +++ b/packages/sdk-metrics/src/aggregator/Histogram.ts @@ -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'; /** @@ -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 { diff --git a/packages/sdk-metrics/src/aggregator/types.ts b/packages/sdk-metrics/src/aggregator/types.ts index 9be5247702..afbc35464b 100644 --- a/packages/sdk-metrics/src/aggregator/types.ts +++ b/packages/sdk-metrics/src/aggregator/types.ts @@ -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: { diff --git a/packages/sdk-metrics/src/utils.ts b/packages/sdk-metrics/src/utils.ts index 9a8f80abde..8648a12361 100644 --- a/packages/sdk-metrics/src/utils.ts +++ b/packages/sdk-metrics/src/utils.ts @@ -165,30 +165,27 @@ export function setEquals(lhs: Set, rhs: Set): 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 { diff --git a/packages/sdk-metrics/test/Instruments.test.ts b/packages/sdk-metrics/test/Instruments.test.ts index e02a393397..a3348a2f88 100644 --- a/packages/sdk-metrics/test/Instruments.test.ts +++ b/packages/sdk-metrics/test/Instruments.test.ts @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, diff --git a/packages/sdk-metrics/test/aggregator/Histogram.test.ts b/packages/sdk-metrics/test/aggregator/Histogram.test.ts index 894e30e4af..036d79cc0e 100644 --- a/packages/sdk-metrics/test/aggregator/Histogram.test.ts +++ b/packages/sdk-metrics/test/aggregator/Histogram.test.ts @@ -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, @@ -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, diff --git a/packages/sdk-metrics/test/utils.test.ts b/packages/sdk-metrics/test/utils.test.ts index 16ec0a2b3a..03df6e863d 100644 --- a/packages/sdk-metrics/test/utils.test.ts +++ b/packages/sdk-metrics/test/utils.test.ts @@ -17,7 +17,7 @@ import * as sinon from 'sinon'; import * as assert from 'assert'; import { - binarySearchLB, + binarySearchUB, callWithTimeout, hashAttributes, TimeoutError, @@ -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]); }); } });