Skip to content

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 <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
@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 <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
@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