-
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): Force using exactly one rolling window as date range by default #183754
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
8594b14
to
ecf29c7
Compare
@@ -14,7 +14,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { | |||
// failsOnMKI, see https://github.com/elastic/kibana/issues/180481 | |||
describe('Trained models list', function () { | |||
this.tags(['failsOnMKI']); | |||
|
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.
@elastic/ml-ui CI auto commited this for you to review 😅
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
@@ -46,7 +46,7 @@ const generateEsResponseForRollingSLO = (slo: SLODefinition, overridedRange?: Da | |||
? rollingDurationInDays + overridedRangeInDays | |||
: rollingDurationInDays * 2; | |||
const numberOfBuckets = fullDuration * bucketsPerDay; | |||
const startDay = moment().subtract(fullDuration, 'day').startOf('day'); | |||
const startDay = moment().subtract(fullDuration, 'day').startOf('minute'); |
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.
@kdelemme Now that you changed to startOf('minute')
, maybe you would like to change the name of the current startDay
variable? Would startMinute
make sense or is it still startDay?
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.
I guess startRange would be even better
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Canvas Sharable Runtime
History
To update your PR or re-run it, just comment with: |
Fix: #183748
Summary
This changes the start date for both ranges used in the historical summary. We now use the
startOf("minute")
instead ofstartOf("day")
for the start range.It makes the number of buckets deterministic, e.g. always 7d * 24buckets/d = 168 buckets, or 30d * 6buckets/d = 180 buckets.
Previously, by defining the range as
now.minus(7d).startOf("day")
tonow.startOf("minutes")
, we were using different range duration depending on the time of the day.