Improve components that are used to unit test instrumentation logic #129
Conversation
This file contains 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
|
|
The `MonotonicClock` interface offers an abstraction for us to manipulate time in test. It aims to improve the implementation of `TestScope` so that develpers can verify metric recording of timers and histograms.
trein
force-pushed
the
testscope-timer-fix
branch
4 times, most recently
from
6821ff7 to
1cb7175
Compare
roman-mazhut
approved these changes
Nov 25, 2024
The following changes have been made as part of this commit: 1. Refactored how snapshots are created to leverage the same reporting mechanism used in regular metrics recording. 2. Fixed snapshots for Timers. The previous `TimerImpl` implementation relied on `NoReporterSink` to properly create `TimerSnapshots`. However given `TestScope` instances are built with `NullStatsReporter`, `TimerSnapshots` were not being created at all when using `TestScope`. 3. Leveraged the `ImmutableBuckets` implementation to compute bucket bounds in `HistogramImpl`, allowing removal of a substantial amount of duplicated code. 4. Added unit tests to cover the fixes for `TimerSnapshots` as well as the changes to the snapshot creation logic.
No functionality change. Just removal of warnings in the code.
trein
force-pushed
the
testscope-timer-fix
branch
2 times, most recently
from
c5b5e77 to
f9fb566
Compare
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.
Several improvements are suggested in this PR:
Introduced a new abstraction so that developers can manipulate time in tests. The
MonotonicClockinterface offers an abstraction for us to manipulate time in test. It aims to improve the implementation ofTestScopeso that developers can verify metric recording of timers and histograms.Refactored how snapshots are created to leverage the same reporting mechanism used in regular metrics recording.
Fixed snapshots for
Timers, which seem to be broken at HEAD. The previousTimerImplimplementation relied onNoReporterSinkto properly createTimerSnapshots. However givenTestScopeinstances are built withNullStatsReporter,TimerSnapshotswere not being created at all when usingTestScope.Leveraged the
ImmutableBucketsimplementation to compute bucket bounds inHistogramImpl, allowing removal of a substantial amount of duplicated code.Added unit tests to cover the fixes for
TimerSnapshotsas well as the changes to the snapshot logic.The changes were structured in dependent but self-contained commits hoping to allow for easier code review. The lack of support for stacked PRs in Github made very difficult to create separate PRs for them.
The changes were tested according to instructions in
DEVELOPMENT.md.