Skip to content

MQE: add support for timestamp #10532

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Feb 3, 2025
Merged

MQE: add support for timestamp #10532

merged 6 commits into from
Feb 3, 2025

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Jan 29, 2025 •
edited
Loading

What this PR does

This PR adds support for the timestamp function to MQE.

Compared to Prometheus' engine, timestamp runs up to 60% faster in MQE in our benchmarks:

goos: darwin
goarch: arm64
pkg: github.com/grafana/mimir/pkg/streamingpromql/benchmarks
cpu: Apple M1 Pro
                                                        │  Prometheus  │               Mimir                │
                                                        │    sec/op    │   sec/op     vs base               │
Query/timestamp(a_1),_instant_query-10                     142.5µ ± 2%   139.4µ ± 1%   -2.16% (p=0.002 n=6)
Query/timestamp(a_1),_range_query_with_100_steps-10        163.6µ ± 1%   146.2µ ± 1%  -10.67% (p=0.002 n=6)
Query/timestamp(a_1),_range_query_with_1000_steps-10       361.5µ ± 4%   193.7µ ± 1%  -46.41% (p=0.002 n=6)
Query/timestamp(a_100),_instant_query-10                   931.7µ ± 1%   775.5µ ± 6%  -16.76% (p=0.002 n=6)
Query/timestamp(a_100),_range_query_with_100_steps-10      2.111m ± 1%   1.269m ± 0%  -39.91% (p=0.002 n=6)
Query/timestamp(a_100),_range_query_with_1000_steps-10    12.139m ± 0%   5.401m ± 0%  -55.51% (p=0.002 n=6)
Query/timestamp(a_2000),_instant_query-10                  12.37m ± 1%   10.48m ± 2%  -15.31% (p=0.002 n=6)
Query/timestamp(a_2000),_range_query_with_100_steps-10     36.39m ± 1%   19.30m ± 1%  -46.98% (p=0.002 n=6)
Query/timestamp(a_2000),_range_query_with_1000_steps-10   238.41m ± 0%   95.11m ± 1%  -60.11% (p=0.002 n=6)
geomean                                                    3.031m        1.948m       -35.74%

Compared to main, the changes to InstantVectorSelector do introduce some additional latency, but the absolute differences are small and would likely be noise in the context of a more complex query:

goos: darwin
goarch: arm64
pkg: github.com/grafana/mimir/pkg/streamingpromql/benchmarks
cpu: Apple M1 Pro
                                                                                      │   main.txt   │             after.txt              │
                                                                                      │    sec/op    │    sec/op     vs base              │
Query/a_1,_instant_query/engine=Mimir-10                                                138.6µ ±  5%   147.5µ ±  8%  +6.38% (p=0.004 n=6)
Query/a_1,_range_query_with_100_steps/engine=Mimir-10                                   147.6µ ± 12%   152.2µ ±  2%       ~ (p=0.394 n=6)
Query/a_1,_range_query_with_1000_steps/engine=Mimir-10                                  191.5µ ± 11%   203.7µ ±  2%       ~ (p=0.065 n=6)
Query/a_100,_instant_query/engine=Mimir-10                                              777.4µ ±  4%   794.4µ ±  1%       ~ (p=0.394 n=6)
Query/a_100,_range_query_with_100_steps/engine=Mimir-10                                 1.288m ±  4%   1.310m ±  3%       ~ (p=0.180 n=6)
Query/a_100,_range_query_with_1000_steps/engine=Mimir-10                                5.378m ±  0%   5.618m ±  3%  +4.45% (p=0.002 n=6)
Query/a_2000,_instant_query/engine=Mimir-10                                             10.39m ±  1%   10.53m ±  1%  +1.27% (p=0.002 n=6)
Query/a_2000,_range_query_with_100_steps/engine=Mimir-10                                19.23m ±  1%   19.48m ±  2%  +1.30% (p=0.002 n=6)
Query/a_2000,_range_query_with_1000_steps/engine=Mimir-10                               94.38m ±  1%   95.71m ±  1%  +1.41% (p=0.002 n=6)
Query/nh_1,_instant_query/engine=Mimir-10                                               180.3µ ± 10%   190.8µ ±  4%  +5.82% (p=0.041 n=6)
Query/nh_1,_range_query_with_100_steps/engine=Mimir-10                                  225.6µ ±  5%   242.4µ ±  7%  +7.45% (p=0.004 n=6)
Query/nh_1,_range_query_with_1000_steps/engine=Mimir-10                                 722.4µ ±  8%   728.2µ ±  7%       ~ (p=0.589 n=6)
Query/nh_100,_instant_query/engine=Mimir-10                                             4.681m ±  7%   4.824m ±  5%       ~ (p=0.310 n=6)
Query/nh_100,_range_query_with_100_steps/engine=Mimir-10                                8.680m ±  1%   8.836m ±  4%  +1.80% (p=0.004 n=6)
Query/nh_100,_range_query_with_1000_steps/engine=Mimir-10                               46.19m ±  1%   46.43m ±  3%       ~ (p=0.180 n=6)
Query/nh_2000,_instant_query/engine=Mimir-10                                            84.42m ±  1%   84.68m ±  4%  +0.31% (p=0.041 n=6)
Query/nh_2000,_range_query_with_100_steps/engine=Mimir-10                               153.6m ±  1%   154.1m ±  1%       ~ (p=0.818 n=6)
Query/nh_2000,_range_query_with_1000_steps/engine=Mimir-10                              802.4m ±  2%   812.3m ±  2%       ~ (p=0.310 n=6)

There are no noticeable changes to peak memory utilisation in our benchmarks.

Which issue(s) this PR fixes or relates to

Part of #10067

Checklist

  • Tests updated.
  • [n/a] Documentation added.
  • [covered by Mimir Query Engine #10067] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • [n/a] about-versioning.md updated with experimental features.

@charleskorn charleskorn mentioned this pull request Jan 29, 2025
Open
@charleskorn charleskorn marked this pull request as ready for review January 29, 2025 05:32
@charleskorn charleskorn requested a review from a team as a code owner January 29, 2025 05:32
jhesketh
jhesketh approved these changes Feb 3, 2025
View reviewed changes
Copy link
Contributor

@jhesketh jhesketh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but I think this could also be worth passing through the gauntlet.

pkg/streamingpromql/testdata/ours/functions.test
{case="histograms only"} 0 30 120
{case="floats and histograms"} 0 30 120

# If timestamp() is used over something that is not a selector, it returns the timestamps of the points returned.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be nice to see a test for something like timestamp(rate(metric[5m])) or similar

@charleskorn charleskorn enabled auto-merge (squash) February 3, 2025 05:23
@charleskorn charleskorn force-pushed the charleskorn/mqe-timestamp branch from 1a7e3ea to 1a8a51a Compare February 3, 2025 05:55
@charleskorn charleskorn force-pushed the charleskorn/mqe-timestamp branch from 1a8a51a to 31804e9 Compare February 3, 2025 23:04
@charleskorn charleskorn merged commit d3dd093 into main Feb 3, 2025
28 checks passed
@charleskorn charleskorn deleted the charleskorn/mqe-timestamp branch February 3, 2025 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhesketh jhesketh jhesketh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@charleskorn @jhesketh