Skip to content
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

[query] Add support for last_over_time #3884

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

ryansammonaiven
Copy link
Contributor

This <aggregation>_over_time function was added in
Prometheus v2.26.0 (changelog, docs).

What this PR does / why we need it:

Fixes #3883.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

Add support for `last_over_time`.

Does this PR require updating code package or user-facing documentation?:

NONE

@CLAassistant
Copy link

CLAassistant commented Oct 28, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Oct 28, 2021

Codecov Report

Merging #3884 (d1fb173) into master (9c0ab8e) will increase coverage by 8.4%.
The diff coverage is n/a.

❗ Current head d1fb173 differs from pull request most recent head 3f94f40. Consider uploading reports for the commit 3f94f40 to get more accurate results

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3884      +/-   ##
=========================================
+ Coverage    48.4%   56.8%    +8.4%     
=========================================
  Files         213     552     +339     
  Lines       19364   63291   +43927     
=========================================
+ Hits         9386   36005   +26619     
- Misses       9254   24065   +14811     
- Partials      724    3221    +2497     
Flag Coverage Δ
aggregator 64.3% <ø> (+1.3%) ⬆️
cluster ∅ <ø> (∅)
collector 58.4% <ø> (ø)
dbnode 60.3% <ø> (?)
m3em 46.4% <ø> (ø)
metrics 19.7% <ø> (ø)
msg 74.2% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c0ab8e...3f94f40. Read the comment docs.

@ryansammonaiven ryansammonaiven marked this pull request as ready for review October 28, 2021 20:23
@fingon
Copy link
Contributor

fingon commented Oct 29, 2021

src/query/test/compatibility/testdata/functions.test might need to have last_over_time uncommented too?

@ryansammonaiven ryansammonaiven marked this pull request as draft October 29, 2021 11:57
@ryansammonaiven ryansammonaiven force-pushed the ryansammonaiven/last_over_time branch 5 times, most recently from 3c98630 to 4066702 Compare October 29, 2021 15:01
@ryansammonaiven
Copy link
Contributor Author

src/query/test/compatibility/testdata/functions.test might need to have last_over_time uncommented too?

It appears that when an aggregation returns NaN, it isn't returned, which is why the min_over_time and max_over_time tests were also commented out (referencing "keep NaN"). I uncommented what currently works for all three of these so at least there is some testing like this.

When doing this, I discovered that Prometheus expects the series to keep the metric name (e.g. data) for the last_over_time function, but not the other ones (e.g. min_over_time, max_over_time). This is intentional, and I traced it to this code: https://github.com/prometheus/prometheus/blob/a278ea4b58226c3db271fb78703cd110089cba9b/promql/engine.go#L1292-L1298

Unfortunately, this makes the change a bit more complex than I initially anticipated, as now part of the code that was unconditional before, now needs to be conditional on what function is being executed.

@ryansammonaiven ryansammonaiven marked this pull request as ready for review October 29, 2021 16:31
@wesleyk
Copy link
Collaborator

wesleyk commented Nov 17, 2021

@ryansammonaiven thanks for the contribution! Can you update the branch and we'll get it reviewed/merged?

@ryansammonaiven
Copy link
Contributor Author

@wesleyk thanks for taking a look! I have updated the branch and all checks have passed.

@wesleyk
Copy link
Collaborator

wesleyk commented Nov 18, 2021

@ryansammonaiven looks like you need another update

This `<aggregation>_over_time` function was added in
Prometheus v2.26.0.
@wesleyk wesleyk merged commit 9851b4f into m3db:master Nov 18, 2021
@ryansammonaiven
Copy link
Contributor Author

@wesleyk thanks for the approval and merge!

@wesleyk
Copy link
Collaborator

wesleyk commented Nov 18, 2021

@ryansammonaiven we'll be cutting v1.4.0 soon with your change!

@wesleyk
Copy link
Collaborator

wesleyk commented Nov 19, 2021

@ryansammonaiven v1.4.0 is cut now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for last_over_time
4 participants