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.

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

feat(sdk-metrics): apply binary search in histogram recording #3539

Merged
merged 2 commits into from
Jan 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/

### :rocket: (Enhancement)

* feat(sdk-metrics): apply binary search in histogram recording [#3539](https://github.com/open-telemetry/opentelemetry-js/pull/3539) @legendecas

### :bug: (Bug Fix)

### :books: (Refine Doc)
Expand Down
14 changes: 4 additions & 10 deletions packages/sdk-metrics/src/aggregator/Histogram.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {
import { DataPointType, HistogramMetricData } from '../export/MetricData';
import { HrTime } from '@opentelemetry/api';
import { InstrumentDescriptor, InstrumentType } from '../InstrumentDescriptor';
import { Maybe } from '../utils';
import { binarySearchLB, Maybe } from '../utils';
import { AggregationTemporality } from '../export/AggregationTemporality';

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

for (let i = 0; i < this._boundaries.length; i++) {
if (value < this._boundaries[i]) {
this._current.buckets.counts[i] += 1;
return;
}
}
// value is above all observed boundaries
this._current.buckets.counts[this._boundaries.length] += 1;
const idx = binarySearchLB(this._boundaries, value);
this._current.buckets.counts[idx + 1] += 1;
}

setStartTime(startTime: HrTime): void {
Expand All @@ -104,7 +98,7 @@ export class HistogramAggregator implements Aggregator<HistogramAccumulation> {
public kind: AggregatorKind.HISTOGRAM = AggregatorKind.HISTOGRAM;

/**
* @param _boundaries upper bounds of recorded values.
* @param _boundaries sorted upper bounds of recorded values.
* @param _recordMinMax If set to true, min and max will be recorded. Otherwise, min and max will not be recorded.
*/
constructor(
Expand Down
27 changes: 27 additions & 0 deletions packages/sdk-metrics/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,3 +163,30 @@ export function setEquals(lhs: Set<unknown>, rhs: Set<unknown>): boolean {
}
return true;
}

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

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

if (arr[hi] <= value) {
return hi;
} else if (arr[lo] <= value) {
return lo;
}
return -1;
}
31 changes: 30 additions & 1 deletion packages/sdk-metrics/test/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,12 @@

import * as sinon from 'sinon';
import * as assert from 'assert';
import { callWithTimeout, hashAttributes, TimeoutError } from '../src/utils';
import {
binarySearchLB,
callWithTimeout,
hashAttributes,
TimeoutError,
} from '../src/utils';
import { assertRejects } from './test-utils';
import { MetricAttributes } from '@opentelemetry/api';

Expand Down Expand Up @@ -60,4 +65,28 @@ describe('utils', () => {
}
});
});

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

[[0, 10, 100, 1000, 10_000], -1, -1],
[[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],
] 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]);
});
}
});
});