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

MQE: Add support for resets function #9859

Merged
merged 10 commits into from
Nov 13, 2024
Merged

MQE: Add support for resets function #9859

merged 10 commits into from
Nov 13, 2024

Conversation

lamida
Copy link
Contributor

@lamida lamida commented Nov 10, 2024

What this PR does

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

  • Tests updated.
  • n.a Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • n.a about-versioning.md updated with experimental features.

@lamida lamida marked this pull request as ready for review November 10, 2024 19:23
@lamida lamida requested a review from a team as a code owner November 10, 2024 19:23
@lamida lamida force-pushed the lamida/mqe-resets-function branch 2 times, most recently from 2749328 to 30921cf Compare November 11, 2024 06:59
lamida and others added 8 commits November 13, 2024 01:49
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
@lamida lamida force-pushed the lamida/mqe-resets-function branch from 9c4f643 to 08264fe Compare November 12, 2024 17:50
@lamida lamida requested a review from charleskorn November 12, 2024 19:10
Copy link
Contributor

@charleskorn charleskorn left a comment

Choose a reason for hiding this comment

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

LGTM modulo nits


# Testing resets
load 1m
simple_metric_all_same_no_reset{num="0"} 0 0 0 0 0 0 0 0
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] It might be clearer to change these series to use labels like these:

metric{case="all the same, no resets, value 0"}
metric{case="all the same, no resets, value 3"}
metric{case="all floats, no resets"}
metric{case="all floats, some resets"}
...

This would make the cases clearer given we can use full English phrases. Then the evals below can be collapsed into one eval range from 0 to X step Y resets(metric) as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored the test in 647feb3

Comment on lines 388 to 389
haveFloats := len(fHead) > 0 || len(fTail) > 0
haveHistograms := len(hHead) > 0 || len(hTail) > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] The head slice will always be populated if there are any points, so we could drop the len(xTail) checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from xOverTime function 😆 . Such as. So this means, we can drop tail check there too, right? Maybe we can do it in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, we can drop it there too, and I'm happy for you to do that in another PR.

Signed-off-by: Jon Kartago Lamida <[email protected]>
@lamida lamida merged commit cc5f5cc into main Nov 13, 2024
29 checks passed
@lamida lamida deleted the lamida/mqe-resets-function branch November 13, 2024 03:42
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.

2 participants