-
Notifications
You must be signed in to change notification settings - Fork 8k
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
fix(slo): history details data #183097
fix(slo): history details data #183097
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
/ci |
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
.filter( | ||
(bucket) => | ||
moment(bucket.key_as_string).isSameOrAfter(dateRange.range.from) && | ||
moment(bucket.key_as_string).isSameOrBefore(dateRange.range.to) | ||
) |
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.
can you please explain why this post-filtering is required?
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 , works as expected now , even for smaller ranges !!
💚 Build Succeeded
Metrics [docs]Async chunks
Canvas Sharable Runtime
Page load bundle
History
To update your PR or re-run it, just comment with: |
Resolves #182251
🌮 Summary
This PR fixes the calculation of the historical summary data when an ad-hoc date range is provided. Indeed, when querying the SLI data for a rolling SLO with a provided date range, we need to query since the start of the range minus the slo rolling window duration. Otherwise the cumulative sum is not correct.
One way to see it is that when we zoom into the chart, the values should not change (when keeping the same bucket sizes, i.e when the range duration is in the same order of magnitude than the time window duration)
I'm also cleaning up the range type, by keeping only one common type, e.g. using Date type. I think that string or number for representing dates are dangerous, and difficult to follow in a codebase.
I also need to go back to the historical summary client and clean up the DateRange i've introduced to fix the issue aforementioned issue. it's not very clean, and pretty confusing at the moment. So far, I only came up with a comment. Any suggestion for improving this part of the code,
getDateRange()
?I've used this to validate the tests: https://docs.google.com/spreadsheets/d/1uDoqq4sJPt2Dnksc1U2x6zAfQB73SK2udvweLl6ODuo/edit#gid=908013864
How to test
node x-pack/scripts/data_forge.js --events-per-cycle 50 --lookback now-14d --dataset fake_stack --install-kibana-assets --kibana-url http://localhost:5601/kibana
historical.slo.page.mov