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

[tests] fix brittle deviation test for tracer #945

Merged
merged 4 commits into from
May 20, 2019

Conversation

majorgreys
Copy link
Contributor

@majorgreys majorgreys commented May 17, 2019

The RateSamplerTest.test_sample_rate_deviation was failing for sample_rate = 0.1. The actual deviation was 0.0216 though the manual threshold is set to 0.02. While we could increase the number of iterations or do more convergence testing, we are increasing the deviation threshold to 0.05 to keep a reasonable upper bound in place while making the test less brittle.

__________________ RateSamplerTest.test_sample_rate_deviation __________________

self = <tests.test_sampler.RateSamplerTest testMethod=test_sample_rate_deviation>

    def test_sample_rate_deviation(self):
        for sample_rate in [0.1, 0.25, 0.5, 1]:
            tracer = get_dummy_tracer()
            writer = tracer.writer
    
            tracer.sampler = RateSampler(sample_rate)
    
            random.seed(1234)
    
            iterations = int(1e4 / sample_rate)
    
            for i in range(iterations):
                span = tracer.trace(i)
                span.finish()
    
            samples = writer.pop()
    
            # We must have at least 1 sample, check that it has its sample rate properly assigned
            assert samples[0].get_metric(SAMPLE_RATE_METRIC_KEY) == sample_rate
    
            # Less than 2% deviation when 'enough' iterations (arbitrary, just check if it converges)
            deviation = abs(len(samples) - (iterations * sample_rate)) / (iterations * sample_rate)
>           assert deviation < 0.02, 'Deviation too high %f with sample_rate %f' % (deviation, sample_rate)
E           AssertionError: Deviation too high 0.021600 with sample_rate 0.100000
E           assert 0.0216 < 0.02

tests/test_sampler.py:37: AssertionError

Since we introduced random.SystemRandom for generating unique identifiers for spans, here we've removed random.seed calls as they do not affect the state of underlying random.SystemRandom.

@majorgreys majorgreys requested a review from a team as a code owner May 17, 2019 21:29
@majorgreys majorgreys force-pushed the majorgreys/fix-deviation-test branch from 86a4371 to 8ba576b Compare May 17, 2019 21:30
jd
jd previously requested changes May 18, 2019
Copy link
Contributor

@jd jd left a comment

Choose a reason for hiding this comment

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

Can you explain in the commit message why thix fix the issue and which issue it fixes?

@majorgreys
Copy link
Contributor Author

Ok, removing random.seed does not fix this issue:

            # Less than 2% deviation when 'enough' iterations (arbitrary, just check if it converges)
            deviation = abs(len(samples) - (iterations * sample_rate)) / (iterations * sample_rate)
>           assert deviation < 0.02, 'Deviation too high %f with sample_rate %f' % (deviation, sample_rate)
E           AssertionError: Deviation too high 0.020500 with sample_rate 0.100000
E           assert 0.0205 < 0.02

I see three approaches to fixing this moving forward:

  1. Increase the threshold, say to 2.5%
  2. Increase the number of iterations for lower sample rates since we might not have collected enough samples
  3. Test convergence rather than a single rate test

@majorgreys majorgreys requested a review from jd May 20, 2019 19:26
@majorgreys majorgreys dismissed jd’s stale review May 20, 2019 19:26

Updated comment

@majorgreys majorgreys changed the title [tests] fix deviation test for tracer [tests] fix brittle deviation test for tracer May 20, 2019
@majorgreys majorgreys merged commit 1bb3d42 into master May 20, 2019
@majorgreys majorgreys deleted the majorgreys/fix-deviation-test branch May 20, 2019 19:27
@majorgreys majorgreys added this to the 0.26.0 milestone Jun 3, 2019
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