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

Revert "Upgrade to google.golang.org/grpc v1.66.2 / modify certain protobuf messages to retain their unmarshaling buffer" #9718

Closed
wants to merge 1 commit into from

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Oct 23, 2024

Reverts #9401

Running go test -v -run="TestBothEnginesReturnSameResultsForBenchmarkQueries/a_100,.*" from pkg/streamingpromql/benchmarks fails roughly 50% of the time for me, and it looks like Prometheus' engine is getting corrupted label names and values, for example:

=== RUN   TestBothEnginesReturnSameResultsForBenchmarkQueries/2_*_a_100,_range_query_with_100_steps
    utils.go:61:
        	Error Trace:	/Users/charleskorn/Repositories/Grafana/mimir/pkg/streamingpromql/testutils/utils.go:61
        	            				/Users/charleskorn/Repositories/Grafana/mimir/pkg/streamingpromql/benchmarks/comparison_test.go:111
        	Error:      	Not equal:
        	            	expected: labels.Labels{labels.Label{Name:"\x03", Value:"\n"}}
        	            	actual  : labels.Labels{labels.Label{Name:"l", Value:"0"}}

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -2,4 +2,4 @@
        	            	  (labels.Label) {
        	            	-  Name: (string) (len=1) "\x03",
        	            	-  Value: (string) (len=1) "\n"
        	            	+  Name: (string) (len=1) "l",
        	            	+  Value: (string) (len=1) "0"
        	            	  }
        	Test:       	TestBothEnginesReturnSameResultsForBenchmarkQueries/2_*_a_100,_range_query_with_100_steps

(the result from Prometheus' engine is the expected value in this test, MQE is the actual)

Another sample failure:

=== RUN   TestBothEnginesReturnSameResultsForBenchmarkQueries/a_100,_range_query_with_1000_steps
    utils.go:61:
        	Error Trace:	/Users/charleskorn/Repositories/Grafana/mimir/pkg/streamingpromql/testutils/utils.go:61
        	            				/Users/charleskorn/Repositories/Grafana/mimir/pkg/streamingpromql/benchmarks/comparison_test.go:111
        	Error:      	Not equal:
        	            	expected: labels.Labels{labels.Label{Name:"\x00(\x03@\n\x01\xd0\x02", Value:"\x00\xa0\xfd\x00("}, labels.Label{Name:"\xd0", Value:"4\x00"}}
        	            	actual  : labels.Labels{labels.Label{Name:"__name__", Value:"a_100"}, labels.Label{Name:"l", Value:"0"}}

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -2,8 +2,8 @@
        	            	  (labels.Label) {
        	            	-  Name: (string) (len=8) "\x00(\x03@\n\x01\xd0\x02",
        	            	-  Value: (string) (len=5) "\x00\xa0\xfd\x00("
        	            	+  Name: (string) (len=8) "__name__",
        	            	+  Value: (string) (len=5) "a_100"
        	            	  },
        	            	  (labels.Label) {
        	            	-  Name: (string) (len=1) "\xd0",
        	            	-  Value: (string) (len=2) "4\x00"
        	            	+  Name: (string) (len=1) "l",
        	            	+  Value: (string) (len=1) "0"
        	            	  }
        	Test:       	TestBothEnginesReturnSameResultsForBenchmarkQueries/a_100,_range_query_with_1000_steps

These tests consistently pass without the change in #9401.

@charleskorn charleskorn marked this pull request as ready for review October 23, 2024 05:40
@colega
Copy link
Contributor

colega commented Oct 23, 2024

Reverted just the conflicting part: #9721

@aknuds1 aknuds1 deleted the revert-9401-arve/upgrade-grpc branch November 14, 2024 07:45
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