-
Notifications
You must be signed in to change notification settings - Fork 39
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
Handle empty metrics after 10 min #219
Conversation
@@ -534,15 +534,15 @@ end | |||
## Development | |||
|
|||
After checking out the repo, run `bin/setup` to install dependencies. | |||
Then, run `rake spec` to run the tests. | |||
Then, run `bundle exec rake spec` to run the tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least in my machine I needed to add exec and other places in this readme also uses exec so I think this is the safest
Pull Request Test Coverage Report for Build 12263142735Details
💛 - Coveralls |
platformVersion: anything, | ||
bucket: {} | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this working as expected? I thought the intention would be not to post to server when it's empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nvm, we're subtracting by 10 minutes and 1 second, which means that the second part of our condition is not met, so we post the empty metrics. Got it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, turns out this code path was apparently not tested and didn't work as expected. Although it's an edge case I think it makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Nuno Góis <[email protected]>
About the changes
This PR adds a test to validate what happens when metrics are empty for more than 10 minutes. This other PR #218 tested that it was actually failing.
We've also validated that nil bucket returns a 400 from Unleash server side so instead we should always fallback to an empty bucket.