fix: measure latencySecond with monotonic high-resolution clock#322
Open
fix: measure latencySecond with monotonic high-resolution clock#322
Conversation
Signed-off-by: Alessandro Yuichi Okimoto <yuichijpn@gmail.com>
There was a problem hiding this comment.
Pull request overview
Updates the SDK’s evaluation-fetch latency measurement to use a monotonic, high-resolution clock so very fast responses no longer emit latencySecond: 0 and get rejected by the backend.
Changes:
- Added
latencyStartMillis()/latencySecondsSince()helpers backed byperformance.now(). - Switched
ApiClientImpl.getEvaluationslatency timing fromDate.now()to the new helpers. - Added unit tests covering the new latency helpers and the original
latencySecond is 0regression.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/internal/utils/time.ts |
Introduces performance.now()-based helpers for latency measurement. |
src/internal/remote/ApiClient.ts |
Uses the new helpers to compute seconds for evaluation fetch latency. |
test/internal/utils/time.spec.ts |
Adds tests intended to validate non-zero/sub-ms latency measurement behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
59
to
+75
| @@ -71,7 +72,7 @@ export class ApiClientImpl implements ApiClient { | |||
| return { | |||
| type: 'success', | |||
| sizeByte: contentLength, | |||
| seconds: (finish - start) / 1000, | |||
| seconds, | |||
Comment on lines
+70
to
+88
| test('latencySecondsSince matches a parallel performance.now() reading', () => { | ||
| // Sanity: the helper actually divides the performance.now() diff by | ||
| // 1000. Compute the same interval independently and confirm the | ||
| // helper's value is consistent with it. | ||
| const start = latencyStartMillis() | ||
| const perfStart = performance.now() | ||
| // tiny burn of work so the interval is non-zero | ||
| let acc = 0 | ||
| for (let i = 0; i < 1000; i++) { | ||
| acc += Math.sqrt(i) | ||
| } | ||
| expect(acc).toBeGreaterThan(0) | ||
| const second = latencySecondsSince(start) | ||
| const perfDiffSec = (performance.now() - perfStart) / 1000 | ||
| expect(second).toBeGreaterThan(0) | ||
| // helper measured BEFORE the second performance.now() read, so it | ||
| // must be <= the independently-computed value (with a small ε for | ||
| // jitter). | ||
| expect(second).toBeLessThanOrEqual(perfDiffSec + 1e-6) |
Comment on lines
+32
to
+67
| test('latencySecondsSince > 0 for an awaited microtask (regression for "latencySecond is 0")', async () => { | ||
| // The most realistic SDK scenario: a fetch call that resolves quickly. | ||
| // `await` schedules a microtask, which always takes >> 1ns. With the | ||
| // old `Date.now()` clock this measurement routinely came back 0; with | ||
| // `performance.now()` it must be strictly > 0 every time. | ||
| for (let i = 0; i < 100; i++) { | ||
| const start = latencyStartMillis() | ||
| await Promise.resolve() | ||
| const second = latencySecondsSince(start) | ||
| expect( | ||
| second, | ||
| `iteration ${i}: expected latencySecondsSince > 0 for an awaited microtask, got ${second}`, | ||
| ).toBeGreaterThan(0) | ||
| } | ||
| }) | ||
|
|
||
| test('latencySecondsSince has sub-millisecond resolution (proves the fix)', async () => { | ||
| // The pre-fix `Date.now()` timer has 1ms granularity, so this assertion | ||
| // would have been impossible to satisfy. Show that the new helper can | ||
| // measure intervals smaller than 1 millisecond. We sample several | ||
| // microtasks; at least one should come back below 1ms on any | ||
| // reasonable hardware. | ||
| let sawSubMs = false | ||
| for (let i = 0; i < 50; i++) { | ||
| const start = latencyStartMillis() | ||
| await Promise.resolve() | ||
| const second = latencySecondsSince(start) | ||
| if (second > 0 && second < 0.001) { | ||
| sawSubMs = true | ||
| break | ||
| } | ||
| } | ||
| expect( | ||
| sawSubMs, | ||
| 'expected at least one awaited microtask to measure < 1ms with the new helper', | ||
| ).toBe(true) |
duyhungtnn
approved these changes
May 9, 2026
Collaborator
duyhungtnn
left a comment
duyhungtnn
left a comment
There was a problem hiding this comment.
@cre8ivejp it looks great.
after merge this PR, I'd like to merge this following PR
https://github.com/bucketeer-io/javascript-client-sdk/pull/325/changes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace
Date.now()-based latency measurement inApiClient.getEvaluationswithperformance.now()so that sub-millisecond network responses no longer reportlatencySecond: 0and get rejected by the backend withduration is nil and latencySecond is 0: gateway: metrics event has invalid duration.Why
The JS SDK measured latency as:
Date.now()is wall-clock and integer-millisecond resolution, so any operation that completes in less than 1 ms producesseconds: 0, the SDK shipslatencySecond: 0, and the backend correctly flags it as invalid (proto3doublehas no field-presence, so0is indistinguishable from "unset"):The same root cause has already been fixed in the Node and Android SDKs in their respective repos. A 100 000-iteration sweep confirmed
Date.now() - Date.now()returning0ms in 99 997 / 100 000 consecutive calls. In production we observedlatencySecond: 0events forsourceId=JAVASCRIPTflowing through the same code path.This PR brings the JS SDK in line with the iOS / Go SDKs, both of which already use higher-resolution monotonic timers.
What changed
src/internal/utils/time.ts(new): addlatencyStartMillis()andlatencySecondsSince(start)backed byperformance.now()(W3C High Resolution Time spec). The helper works in every target this SDK builds for — browser, Node 16+, and React Native — without runtime branching, becauseperformance.now()is part of the global API in all three.src/internal/remote/ApiClient.ts::getEvaluations: switch the network-call timer to the new helpers and pass the result directly toseconds.test/internal/utils/time.spec.ts(new): 5 unit tests, including a regression test that assertslatencySecondsSince(...) > 0for an awaited microtask across 100 iterations (the exact pattern the SDK runs aroundawait postInternal(...)), plus a sub-millisecond-resolution test that the previousDate.now()clock would have been physically incapable of satisfying.Clock(used for event timestamps, not latency) is intentionally untouched — its consumers want wall-clock seconds, not monotonic time.No backend, proto, or public-API changes are required.
Test plan
pnpm test:node --run— 327/327 passing, including the 5 newinternal/utils/timetests.pnpm test:browser --run— 327/327 passing.pnpm typecheck:libandpnpm typecheck:test— both clean.BKTClientintegration tests, which exercise the realApiClient->EventInteractor->register_eventspipeline, now ship payloads like"latencySecond": 0.0005245419999999968(~525 µs) where they previously would have shipped"latencySecond": 0. Verified by inspecting MSW request bodies during the test run.