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 prometheus #487

Conversation

harry671003
Copy link
Contributor

@harry671003 harry671003 commented Sep 19, 2024

Changes

Note

@GiedriusS
Copy link
Member

I think it won't be so easy due to #486.

@fpetkovski
Copy link
Collaborator

I do not understand the new behavior, raised an issue here: prometheus/prometheus#14961.

@harry671003 harry671003 force-pushed the update-prometheus branch 11 times, most recently from 3f87dc3 to 8df4db9 Compare November 15, 2024 19:14
@harry671003 harry671003 changed the base branch from main to prometheus-update-65f610353919 November 21, 2024 16:49
@harry671003 harry671003 marked this pull request as ready for review November 21, 2024 17:02
@harry671003 harry671003 changed the base branch from prometheus-update-65f610353919 to main November 21, 2024 17:15
@harry671003 harry671003 changed the base branch from main to prometheus-update-65f610353919 November 21, 2024 17:15
Signed-off-by: 🌲 Harry 🌊 John 🏔 <[email protected]>
@yeya24
Copy link
Contributor

yeya24 commented Nov 22, 2024

Quite a lot of tests failed due to round. Should we add my fix to round function to see how much it would help?

--- FAIL: TestInstantQuery/round/disableOptimizers=false/disableFallback=true#04 (0.00s)
                testutil.go:91: engine_test.go:4215: "Query: round(http_requests_total)
                    Explanation:
                    [duplicateLabelCheck]:
                    └──[function] round([http_requests_total]):
                       └──[coalesce]:
                          ├──[concurrent(buff=2)]:
                          │  └──[vectorSelector] {[__name__="http_requests_total"]} 0 mod 2
                          └──[concurrent(buff=2)]:
                             └──[vectorSelector] {[__name__="http_requests_total"]} 1 mod 2
                    
                    "
                    
                    	exp: &promql.Result{Err:error(nil), Value:promql.Vector{promql.Sample{T:0, F:1, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"__name__", Value:"http_requests_total"}, labels.Label{Name:"pod", Value:"nginx-1"}, labels.Label{Name:"series", Value:"1"}}, DropName:true}, promql.Sample{T:0, F:2, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"__name__", Value:"http_requests_total"}, labels.Label{Name:"pod", Value:"nginx-2"}, labels.Label{Name:"series", Value:"2"}}, DropName:true}, promql.Sample{T:0, F:5, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"__name__", Value:"http_requests_total"}, labels.Label{Name:"pod", Value:"nginx-4"}, labels.Label{Name:"series", Value:"3"}}, DropName:true}, promql.Sample{T:0, F:8, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"__name__", Value:"http_requests_total"}, labels.Label{Name:"pod", Value:"nginx-5"}, labels.Label{Name:"series", Value:"1"}}, D...(output trimmed)
                    
                    	got: &promql.Result{Err:error(nil), Value:promql.Vector{promql.Sample{T:0, F:1, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"pod", Value:"nginx-1"}, labels.Label{Name:"series", Value:"1"}}, DropName:false}, promql.Sample{T:0, F:2, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"pod", Value:"nginx-2"}, labels.Label{Name:"series", Value:"2"}}, DropName:false}, promql.Sample{T:0, F:5, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"pod", Value:"nginx-4"}, labels.Label{Name:"series", Value:"3"}}, DropName:false}, promql.Sample{T:0, F:8, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"pod", Value:"nginx-5"}, labels.Label{Name:"series", Value:"1"}}, DropName:false}, promql.Sample{T:0, F:2, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"pod", Value:"nginx-6"}, labels.Label{Name:"series", Value:"2"}}, DropName:false}}, Warnings:annotations.Annotations{}}
                    
                    Diff:
                    --- Expected
                    +++ Actual
                    @@ -1,6 +1,6 @@
                    -(*promql.Result)({__name__="http_requests_total", pod="nginx-1", series="1"} => 1 @[0]
                    -{__name__="http_requests_total", pod="nginx-2", series="2"} => 2 @[0]
                    -{__name__="http_requests_total", pod="nginx-4", series="3"} => 5 @[0]
                    -{__name__="http_requests_total", pod="nginx-5", series="1"} => 8 @[0]
                    -{__name__="http_requests_total", pod="nginx-6", series="2"} => 2 @[0])
                    +(*promql.Result)({pod="nginx-1", series="1"} => 1 @[0]
                    +{pod="nginx-2", series="2"} => 2 @[0]
                    +{pod="nginx-4", series="3"} => 5 @[0]
                    +{pod="nginx-5", series="1"} => 8 @[0]
                    +{pod="nginx-6", series="2"} => 2 @[0])

@MichaHoffmann
Copy link
Contributor

Quite a lot of tests failed due to round. Should we add my fix to round function to see how much it would help?

--- FAIL: TestInstantQuery/round/disableOptimizers=false/disableFallback=true#04 (0.00s)
                testutil.go:91: engine_test.go:4215: "Query: round(http_requests_total)
                    Explanation:
                    [duplicateLabelCheck]:
                    └──[function] round([http_requests_total]):
                       └──[coalesce]:
                          ├──[concurrent(buff=2)]:
                          │  └──[vectorSelector] {[__name__="http_requests_total"]} 0 mod 2
                          └──[concurrent(buff=2)]:
                             └──[vectorSelector] {[__name__="http_requests_total"]} 1 mod 2
                    
                    "
                    
                    	exp: &promql.Result{Err:error(nil), Value:promql.Vector{promql.Sample{T:0, F:1, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"__name__", Value:"http_requests_total"}, labels.Label{Name:"pod", Value:"nginx-1"}, labels.Label{Name:"series", Value:"1"}}, DropName:true}, promql.Sample{T:0, F:2, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"__name__", Value:"http_requests_total"}, labels.Label{Name:"pod", Value:"nginx-2"}, labels.Label{Name:"series", Value:"2"}}, DropName:true}, promql.Sample{T:0, F:5, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"__name__", Value:"http_requests_total"}, labels.Label{Name:"pod", Value:"nginx-4"}, labels.Label{Name:"series", Value:"3"}}, DropName:true}, promql.Sample{T:0, F:8, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"__name__", Value:"http_requests_total"}, labels.Label{Name:"pod", Value:"nginx-5"}, labels.Label{Name:"series", Value:"1"}}, D...(output trimmed)
                    
                    	got: &promql.Result{Err:error(nil), Value:promql.Vector{promql.Sample{T:0, F:1, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"pod", Value:"nginx-1"}, labels.Label{Name:"series", Value:"1"}}, DropName:false}, promql.Sample{T:0, F:2, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"pod", Value:"nginx-2"}, labels.Label{Name:"series", Value:"2"}}, DropName:false}, promql.Sample{T:0, F:5, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"pod", Value:"nginx-4"}, labels.Label{Name:"series", Value:"3"}}, DropName:false}, promql.Sample{T:0, F:8, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"pod", Value:"nginx-5"}, labels.Label{Name:"series", Value:"1"}}, DropName:false}, promql.Sample{T:0, F:2, H:(*histogram.FloatHistogram)(nil), Metric:labels.Labels{labels.Label{Name:"pod", Value:"nginx-6"}, labels.Label{Name:"series", Value:"2"}}, DropName:false}}, Warnings:annotations.Annotations{}}
                    
                    Diff:
                    --- Expected
                    +++ Actual
                    @@ -1,6 +1,6 @@
                    -(*promql.Result)({__name__="http_requests_total", pod="nginx-1", series="1"} => 1 @[0]
                    -{__name__="http_requests_total", pod="nginx-2", series="2"} => 2 @[0]
                    -{__name__="http_requests_total", pod="nginx-4", series="3"} => 5 @[0]
                    -{__name__="http_requests_total", pod="nginx-5", series="1"} => 8 @[0]
                    -{__name__="http_requests_total", pod="nginx-6", series="2"} => 2 @[0])
                    +(*promql.Result)({pod="nginx-1", series="1"} => 1 @[0]
                    +{pod="nginx-2", series="2"} => 2 @[0]
                    +{pod="nginx-4", series="3"} => 5 @[0]
                    +{pod="nginx-5", series="1"} => 8 @[0]
                    +{pod="nginx-6", series="2"} => 2 @[0])

It feels like the issue is on prometheus side, we should bump prometheus to include your fix i think.

@harry671003
Copy link
Contributor Author

Quite a lot of tests failed due to round. Should we add my fix to round function to see how much it would help?

@yeya24, I tested your fix locally. The tests are passing.

It feels like the issue is on prometheus side, we should bump prometheus to include your fix i think.

Yes. This PR is intentionally only updating till prometheus/prometheus@65f610353919. The next commit in Prometheus is the switching to log/slog which is a huge change.

My plan is to work on the Prometheus upgrade incrementally on this branch so that each PR will be small and easier to review.

In retro, the branch could have been named prometheus-v3-update.

@yeya24
Copy link
Contributor

yeya24 commented Nov 24, 2024

@fpetkovski @MichaHoffmann Can you please take a look at the PR?
I reviewed most of the code changes other than code relevant to prometheus/prometheus#14413.

@yeya24
Copy link
Contributor

yeya24 commented Dec 1, 2024

Let's get this PR merged as it targets a non main branch. We can iterate on top of it

@yeya24 yeya24 merged commit 8e20510 into thanos-io:prometheus-update-65f610353919 Dec 1, 2024
4 of 7 checks passed
@fpetkovski
Copy link
Collaborator

Sorry missed this notification. Merging against a non-main branch makes perfect sense 👍

yeya24 pushed a commit that referenced this pull request Dec 3, 2024
SungJin1212 pushed a commit to SungJin1212/promql-engine that referenced this pull request Jan 3, 2025
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