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

fix(dogstatsd_sink): switch from pure map to list of key/value pairs #627

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

JDeuce
Copy link
Contributor

@JDeuce JDeuce commented Jun 20, 2024

Flaky build was observed in this run https://github.com/envoyproxy/ratelimit/actions/runs/9600573727/job/26477125368

--- FAIL: TestLoadMogrifiersFromEnv (0.00s)
    --- FAIL: TestLoadMogrifiersFromEnv/Two_mogrifiers:_First_match (0.00s)
        mogrifier_map_test.go:146: 
            	Error Trace:	/workdir/src/godogstats/mogrifier_map_test.go:146
            	Error:      	Not equal: 
            	            	expected: "custom.foo"
            	            	actual  : "ratelimit.service.rate_limit.foo"
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -1 +1 @@
            	            	-custom.foo
            	            	+ratelimit.service.rate_limit.foo
            	Test:       	TestLoadMogrifiersFromEnv/Two_mogrifiers:_First_match

The test for Two mogrifiers: First match has flaky results because the underlying iteration order from the map is not guaranteed. Sometimes matching first and sometimes matching second mogrifier.

Having a consistent mogrifier order is required for some types of priority based manipulation, and that is how the keys are defined in the environment, so changing the overall implementation to a slice to maintain ordered iteration is the best option. And can be done by only changing the internal private types.

Repro'd originally bug on main and verified results are no longer flaky by running several iterations of the tests:

go test -timeout 30s -count=5000 github.com/envoyproxy/ratelimit/src/godogstats

See
https://go.dev/blog/maps

Iteration order
When iterating over a map with a range loop, the iteration order is not specified and is not guaranteed to be the same from one iteration to the next. If you require a stable iteration order you must maintain a separate data structure that specifies that order.

@JDeuce JDeuce force-pushed the flakefix branch 3 times, most recently from 8fa135d to 23c511f Compare June 20, 2024 18:17
The test for Two mogrifiers: First match has flaky results because the
underlying iteration order from the map is not guaranteed. Sometimes
matching first and sometimes matching second mogrifier.

Having a consistent mogrifier order is required for some types of
priority based manipulation, and that is how the keys are defined in the
environment, so changing the overall implementation to a slice to
maintain ordered iteration is the best option. And can be done by only
changing the internal private types.

Signed-off-by: Josh Jaques <[email protected]>
@JDeuce JDeuce marked this pull request as ready for review June 20, 2024 18:29
@ysawa0 ysawa0 merged commit 91484c5 into envoyproxy:main Jun 20, 2024
6 checks passed
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