Skip to content

Add latency recorder window statistics test - #3582

Merged
wwbmmm merged 1 commit into
apache:masterfrom
wasphin:test-recorder-window-statistics
Oct 6, 2026
Merged

wwbmmm merged 1 commit into
apache:masterfrom
wasphin:test-recorder-window-statistics

Conversation

@wasphin

@wasphin wasphin commented Oct 5, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue Number: N/A

Problem Summary:

A nonzero sampled latency does not prove that a window contains every
recorded observation. Add focused coverage for complete latency recorder
window statistics when writes span multiple sampler ticks.

What is changed and the side effects?

Changed:

  • Add RecorderTest.latency_recorder_window_statistics and a test helper
    that exposes the existing protected window snapshot interface.
  • Record 1, 2, and 3, wait for that batch to be sampled, then record 4
    through 7 to exercise aggregation across sampler ticks.
  • Use a ten-second window with a five-second total wait budget. Confirm
    the full snapshot contains seven observations before checking sum 28
    and average 4, and independently observe and save maximum latency 7.
  • Leave production code and existing tests unchanged.

Side effects:

  • Performance effects: The new test normally waits for two sampling
    cycles and has a five-second polling budget.
  • Breaking backward compatibility: None; test-only addition.

Check List:

  • Built with CMake on macOS and ran the new test 20 times; all passed.
  • git diff --check passed.
  • The change adds focused unit-test coverage for existing behavior.
  • Please follow Contributor Covenant Code of Conduct.

Exercise latency observations split across sampler ticks using a window
longer than the bounded wait budget. Confirm the sampled count is complete
before checking the sum and average from the saved snapshot, and verify
the maximum through its independent sampler.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The test is bounded, correctly validates complete window aggregation, and introduces no production changes.

Review effort: Balanced
Findings: None

What changed in this PR

Adds focused coverage for LatencyRecorder window aggregation across multiple sampler ticks.

Changes:

  • Adds a helper exposing latency-window snapshots.
  • Verifies complete count, sum, average, and maximum latency across two sampled batches.
File Description
test/​bvar_recorder_unittest.cpp Adds multi-tick latency window statistics coverage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@wwbmmm
wwbmmm merged commit d69b9ea into apache:master Oct 6, 2026
45 of 46 checks passed
@wasphin
wasphin deleted the test-recorder-window-statistics branch October 6, 2026 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants