fix(sdk-metrics): use inclusive upper bounds in histogram (#4935)

Co-authored-by: Daniel Patrick <[email protected]>

Author: legendecas

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)
 

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 {

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: {

packages/sdk-metrics/src/utils.ts

@@ -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 {

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,

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,

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]);
       });
     }
   });