Add a threshold for expected zero values in the SPM script #5753
Conversation
Signed-off-by: FlamingSaint <raghuramkannan400@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5753 +/- ##
===========================================
+ Coverage 53.70% 96.66% +42.95%
===========================================
Files 169 341 +172
Lines 8506 16451 +7945
===========================================
+ Hits 4568 15902 +11334
+ Misses 3495 361 -3134
+ Partials 443 188 -255
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: FlamingSaint <raghuramkannan400@gmail.com>
scripts/spm-integration-test.sh
Outdated
| local non_zero_count=0 | ||
| local threshold=3 |
There was a problem hiding this comment.
| local non_zero_count=0 | |
| local threshold=3 | |
| local non_zero_count=0 | |
| local expected_non_zero_count=3 | |
| local zero_count=0 | |
| local expected_max_zero_count=3 |
There was a problem hiding this comment.
but tbh, I would consider deleting zero_count logic altogether. If zero values can be produced due to race conditions in the start-up, maybe we should only be looking for 3+ non-zero values and not worry about zeros.
If we still want to check for errors with a threshold, then it would make sense to only allow zeros at the beginning on the time series, once we start getting data I don't see a strong reason why zeros should show up again
There was a problem hiding this comment.
Yea makes sense, I think we can just wait for 3 non-zero values. Maybe timeout if we still haven't gotten 3 values ?
There was a problem hiding this comment.
we already have the timeout logic handled. This function just checks if "in the current state" the data satisfies the requirements. And that requirement could be 0{0-3} >0{3+}, i.e. no more than 3 leading zeroes followed by at least 3 non-zeros.
There was a problem hiding this comment.
Understood. I will make the changes.
There was a problem hiding this comment.
@FlamingSaint I merged this for now to prevent spurious failures affecting other PRs, but let's make the change we discussed as a next improvement.
Signed-off-by: FlamingSaint <raghuramkannan400@gmail.com>
yurishkuro
added
the
changelog:ci
…cing#5753) ## Which problem is this PR solving? - Makes sure the SPM CI doesn't fail if the first value received is zero. ## Description of the changes - The script now waits until the threshold (Currently set to 3) number of zero values are received before throwing an error ## How was this change tested? - Manually ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [ ] I have added unit tests for the new functionality - [ ] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: FlamingSaint <raghuramkannan400@gmail.com>
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test