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

Update mimir-prometheus to d162bb51b6ef #4759

Merged
merged 4 commits into from
Apr 19, 2023
Merged

Conversation

zenador
Copy link
Contributor

@zenador zenador commented Apr 18, 2023

What this PR does

Updates mimir-prometheus.
See: grafana/mimir-prometheus#480 and grafana/mimir-prometheus#485

Which issue(s) this PR fixes or relates to

N/A

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@zenador zenador force-pushed the zenador/prom-vendor-histo-opt branch from 7682390 to 1010392 Compare April 18, 2023 08:40
@zenador zenador changed the title Update mimir-prometheus to 894f657c Update mimir-prometheus to 21131bf6ad8b Apr 18, 2023
@pracucci
Copy link
Collaborator

@zenador can you upgrade to latest mimir-prometheus commit, so that we also get the changes we want in #4764, please?

@zenador zenador force-pushed the zenador/prom-vendor-histo-opt branch from 1010392 to dff7206 Compare April 18, 2023 16:30
@zenador zenador changed the title Update mimir-prometheus to 21131bf6ad8b Update mimir-prometheus to d162bb51b6ef Apr 18, 2023
@zenador zenador marked this pull request as ready for review April 18, 2023 16:54
@zenador zenador requested review from a team as code owners April 18, 2023 16:54
@krajorama krajorama self-requested a review April 19, 2023 06:12
{fn: "histogram_fraction", tpl: `(<fn>(0,0.5,bar1{}))`},
{fn: "histogram_quantile", tpl: `(<fn>(0.5,bar1{}))`},
})
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to add a note about why this is commented out

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually don't think this should be commented out.

Copy link
Contributor

@krajorama krajorama Apr 19, 2023

Choose a reason for hiding this comment

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

I think @beorn7 made functions more strict in what they return when the input is a histogram. So actually the documentation at https://prometheus.io/docs/prometheus/latest/querying/functions/ might need an update, because now it says: "Functions that do not explicitly mention native histograms in their documentation (see below) effectively treat a native histogram as a float sample of value 0. (This is confusing and will change before native histograms become a stable feature.)" But according to the tests it returns empty set now? Need to verify and probably separate float tests vs histograms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separated tests for floats and histograms.

pkg/mimirpb/mimir.proto Show resolved Hide resolved
// FromPointsToSamples converts []promql.Point to []Sample.
func FromPointsToSamples(points []promql.Point) []Sample {
// FromFPointsToSamples converts []promql.FPoint to []Sample.
func FromFPointsToSamples(points []promql.FPoint) []Sample {
Copy link
Contributor

Choose a reason for hiding this comment

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

note: As a next step (different PR) we could return to casting between promql.FPoint and our Sample type. But I'd also rename mimirpb.Sample to mimirpb.FPoint and also it's members so we can have a unit test that checks the compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would revive #3738

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// FromPointsToHistograms converts []promql.Point to []FloatHistogramPair.
func FromPointsToHistograms(points []promql.Point) []FloatHistogramPair {
// FromHPointsToHistograms converts []promql.HPoint to []FloatHistogramPair.
func FromHPointsToHistograms(points []promql.HPoint) []FloatHistogramPair {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, we have a chance to make this zero copy by aligning the types fully.

Copy link
Contributor

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

FWIW, ruler changes LGTM.

Copy link
Contributor

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

The native histograms related changes LGTM

@krajorama krajorama merged commit a30761c into main Apr 19, 2023
@krajorama krajorama deleted the zenador/prom-vendor-histo-opt branch April 19, 2023 13:19
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.

5 participants