zapcore: Unflake TestSamplerConcurrent #1012
Merged
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
Codecov Report
@@ Coverage Diff @@
## master #1012 +/- ##
==========================================
+ Coverage 98.10% 98.20% +0.10%
==========================================
Files 46 47 +1
Lines 2058 2062 +4
==========================================
+ Hits 2019 2025 +6
+ Misses 30 29 -1
+ Partials 9 8 -1
Continue to review full report at Codecov.
|
We need to be able to use the controlled clock for some other tests so move it from clock_test to the ztest package and rename it to MockClock. To keep the interface for the MockClock clear, don't embed the benbjohnson/clock and instead, use it as an attribute.
The TestSamplerConcurrent test frequently fails with the following error
in CI:
--- FAIL: TestSamplerConcurrent (0.25s)
sampler_test.go:198:
Error Trace: sampler_test.go:198
Error: Max difference between 1250 and 1004 allowed is 125, but difference was 246
Test: TestSamplerConcurrent
Messages: Unexpected number of logs
FAIL
The test is intended to verify that
despite an onsalught of messages from multiple goroutines,
we only allow at most `logsPerTick` messages per `tick`.
This was accompilshed by spin-looping 10 goroutines for `numTicks`,
each logging one of `numMessages` different messages,
and then verifying the final log count.
The source of flakiness here was the non-determinism in
how far a goroutine would get in logging enough messages such that
the sampler would be engaged.
In #948, we added a `zapcore.Clock` interface with a ticker and
a mock implementation.
Move that to `ztest` for use here.
To unflake the test, use the mock clock to control time and
for each goroutine, log `logsPerTick*2` messages `numTicks` times.
This gives us,
for numGoroutines (10)
for numTicks (25)
log logsPerTick * 2 (50) messages
We end up attempting to log a total of,
(numGoroutines * numTicks * logsPerTick * 2) messages
= (10 * 25 * 50) messages
= 12500 messages
Of these, the following should be sampled:
numMessages * numTicks * logsPerTick
= 5 * 10 * 25
= 1250
Everything else should be dropped.
For extra confidence, use a SamplerHook (added in #813) to verify that
the number of sampled and dropped messages meet expectations.
abhinav
force-pushed
the
abg/sampler-flaky
branch
from
77c90b4
to
f0f2f30
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.
The TestSamplerConcurrent test frequently fails with the following error
in CI:
The test is intended to verify that
despite an onsalught of messages from multiple goroutines,
we only allow at most
logsPerTickmessages pertick.This was accompilshed by spin-looping 10 goroutines for
numTicks,each logging one of
numMessagesdifferent messages,and then verifying the final log count.
The source of flakiness here was the non-determinism in
how far a goroutine would get in logging enough messages such that
the sampler would be engaged.
In #948, we added a
zapcore.Clockinterface with a ticker anda mock implementation.
Move that to
ztestfor use here.To unflake the test, use the mock clock to control time and
for each goroutine, log
logsPerTick*2messagesnumTickstimes.This gives us,
We end up attempting to log a total of,
Of these, the following should be sampled:
Everything else should be dropped.
For extra confidence, use a SamplerHook (added in #813) to verify that
the number of sampled and dropped messages meet expectations.
Refs GO-873