perf(query): Align start/end times with step #1473
Conversation
Range queries will now have their start and end times aligned with the step. This will help metric stores take better advantage of caches for queries. See (https://promlabs.com/blog/2020/06/18/the-anatomy-of-a-promql-query#range-queries) for details on range queries. - closes opencost#1472 Signed-off-by: pokom <mark.poko@grafana.com>
Pokom
force-pushed
the
perf/align-step-range-queries
branch
from
b94f65d
to
ef27ef8
Compare
|
Awesome thank you! We will review this soon and are excited by the potential perf improvements! |
|
@AjayTripathy I'm going to add some unit tests for aligning the start/end times, my main concern right now is around the correctness of the time conversions. Let me know if there's anything obviously off with it while I write up some tests. |
|
I found an issue with the new method: |
Caught two bugs due to mismatch in seconds and milliseconds when calculating the updated values for aligning the window. `isRequestStepAligned` will now convert the step to seconds and then check if the modulo of the start and end time(represented as unix time stamps) is zero. If either of the values is not aligned, return false. Otherwise will return true. `alignWindow` will be called if the result of `isRequestStepAligned` is false. This method accepts the start, end and step times. It will convert each number to seconds and then truncate the amount time proportional to the step function. The current implementation will round always round down. IE, if the step is an hour then the minutes and seconds will be removed. See the unit tests for some examples of how the times will be aligned. Adds basic unit tests for both methods. Signed-off-by: pokom <mark.poko@grafana.com>
|
@AjayTripathy I added some unit tests and fixed two bugs I found due to converting between seconds/milliseconds. Let me know when you get a chance to review the PR! I'm in the #opencost CNCF slack channel, feel free to reach out to me there as well 😄 |
|
This LGTM. @nikovacevic would be great if you could take a quick look but in general I think this is low risk since it only touches QueryRange and thus only /model/allocation/compute codepaths. |
|
@AjayTripathy I don't think it even touches that. (All I see using Seems fine to me. |
Range queries will now have their start and end times aligned with the step. This will help metric stores take better advantage of caches for queries. See
(https://promlabs.com/blog/2020/06/18/the-anatomy-of-a-promql-query#range-queries) for details on range queries.
What does this PR change?
Does this PR relate to any other PRs?
How will this PR impact users?
Does this PR address any GitHub or Zendesk issues?
How was this PR tested?
Does this PR require changes to documentation?
Have you labeled this PR and its corresponding Issue as "next release" if it should be part of the next Opencost release? If not, why not?