RUM-4894 Fix the CoreTracer flaky tests#2081
Conversation
| it.getInt(SAMPLING_PRIORITY_KEY) == PrioritySampling.USER_KEEP.toInt() | ||
| } | ||
| val offset = 10 | ||
| val offset = 20 |
There was a problem hiding this comment.
I think the test will be still flaky, even with this change. And 20% is quite big offset, maybe it makes sense only a small absolute values. And if you have actual say 1, and expected as 2, this won't help (because diff will be 50% for such numbers).
There was a problem hiding this comment.
I tested with 1000 repeated tests and never failed. I think it is fine but also my question is regarding the offset as it is too high. Should we still keep this test ?
There was a problem hiding this comment.
I don't have a definitive answer here. I think we have similar tests checking sampling rate in other modules, and they probably don't have a high offset? Maybe we can modify the test somehow?
There was a problem hiding this comment.
yes...will try to have a look what else I can do
There was a problem hiding this comment.
Note
One way to limit flakyness is to increase the numberOfSpans
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2081 +/- ##
===========================================
- Coverage 68.73% 68.58% -0.15%
===========================================
Files 719 719
Lines 26571 26569 -2
Branches 4472 4472
===========================================
- Hits 18262 18220 -42
- Misses 7123 7139 +16
- Partials 1186 1210 +24 |
8f5f6b1 to
7ee5529
Compare
| fun `M use user-keep or user-drop priority W buildSpan { tracer was provided a sample rate }`( | ||
| @StringForgery fakeInstrumentationName: String, | ||
| @DoubleForgery(min = 0.0, max = 100.0) sampleRate: Double, | ||
| @IntForgery(min = 0, max = 100) sampleRate: Int, |
There was a problem hiding this comment.
Note
Why do we need to generate an Int and cast it to a Double instead of generating a Double directly?
There was a problem hiding this comment.
yes good catch was a left over during my tests to see why it is flaky
| val offset = 10 | ||
| // Because of the way sampling works the deviation can be quite high so we will have to use an offset of 20 | ||
| // here to make sure this test will never be flaky | ||
| val offset = 20 |
There was a problem hiding this comment.
Note
The offset should be based off of the numberOfSpans and not hardcoded, e.g.:
val offset = (numberOfSpans * 15) / 100| it.getInt(SAMPLING_PRIORITY_KEY) == PrioritySampling.USER_KEEP.toInt() | ||
| } | ||
| val offset = 10 | ||
| val offset = 20 |
There was a problem hiding this comment.
Note
One way to limit flakyness is to increase the numberOfSpans
7ee5529 to
4c94c57
Compare
| val span = tracer.spanBuilder(forge.anAlphabeticalString()).startSpan() | ||
| // there is a throttle on the sampler which drops all the spans over the 100 limit in 1 second | ||
| // so we need to sleep a bit to make sure the spans are not dropped because of throttling | ||
| Thread.sleep(100) |
There was a problem hiding this comment.
this makes test very slow: 100ms*300 spans is 30 seconds at least for waiting in this test, which is a lot.
drops all the spans over the 100 limit in 1 second
it means we can sleep for 10ms instead of 100 to be within the limit? But ideally we should disable throttling for the test or change the threshold.
There was a problem hiding this comment.
yes I wanted to make sure but indeed we can sleep for 10ms
There was a problem hiding this comment.
still ideally we won't have to use sleep, but 3s is much better than 30s.
4c94c57 to
dc254f2
Compare
| val offset = 10 | ||
| // Because of the way sampling works the deviation can be quite high so we will have to use an offset of 15% | ||
| // here to make sure this test will never be flaky | ||
| val offset = numberOfSpans * 15 / 100 |
There was a problem hiding this comment.
nit: I think here we should use float types, otherwise we are losing a lot by doing an integer division.
| val offset = numberOfSpans * 15 / 100 | |
| val offset = numberOfSpans * 15f / 100 |
There was a problem hiding this comment.
the offset needs to be an int in the end so I will keep it like that.
4 participants
What does this PR do?
A brief description of the change being made with this pull request.
Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)