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

refactor: replace slice-based iteration with go 1.23 iterators #277

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lvlcn-t
Copy link
Collaborator

@lvlcn-t lvlcn-t commented Mar 23, 2025

Motivation

This pull request refactors the iteration mechanism throughout the codebase by replacing slice-based iterations with the new iterator feature utilizing the iter package. Although benchmark tests (see below) did not show a significant change in performance, the new approach streamlines the code with newly introduced patterns regarding iterators.

Changes

Iteration Improvements:

Code Refactoring:

  • Factory Update:

  • Controller Adjustments:

    • Updated methods like Shutdown, Reconcile, and GenerateCheckSpecs in pkg/sparrow/controller.go to work with the new iter.Seq.

Additional Changes:

Commits

This commit refactors the 'Iter' methods to return iterators instead of complete slices. We utilize the newly introduced 'iter' standard library package to achieve this. This change improves the performance of the 'Iter' methods by reducing the number of allocations and copying operations.

Signed-off-by: lvlcn-t [email protected]

For additional information look at the commits.

Tests done

I've added a benchmark test to be able to better see if there is any performance gained. You can find the test results below.

Benchmark Results

The new benchmark test for the Reconcile function shows nearly identical results before and after the changes:

  • Before:
    ~1,002,994,868 ns/op, 102,224 B/op, 1,315 allocs/op

  • After:
    ~1,003,090,300 ns/op, 102,776 B/op, 1,320 allocs/op

These results confirm that while the refactoring does not significantly change the performance characteristics, it improves the code structure by using new language features.

Before
go test -benchmem -run=^$ -bench ^BenchmarkReconcile$ github.com/telekom/sparrow/pkg/sparrow -race -cover -count=1

{"time":"2025-03-23T19:16:31.855068339+01:00","level":"INFO","source":{"function":"github.com/telekom/sparrow/pkg/checks/health.(*Health).Run","file":"/home/tom/dev/sparrow/pkg/checks/health/health.go","line":62},"msg":"Starting healthcheck","interval":"1s"}
{"time":"2025-03-23T19:16:31.855257319+01:00","level":"INFO","source":{"function":"github.com/telekom/sparrow/pkg/checks/latency.(*Latency).Run","file":"/home/tom/dev/sparrow/pkg/checks/latency/latency.go","line":64},"msg":"Starting latency check","interval":"1s"}
{"time":"2025-03-23T19:16:31.855346099+01:00","level":"INFO","source":{"function":"github.com/telekom/sparrow/pkg/checks/dns.(*DNS).Run","file":"/home/tom/dev/sparrow/pkg/checks/dns/dns.go","line":74},"msg":"Starting dns check","interval":"1s"}
goos: linux
goarch: amd64
pkg: github.com/telekom/sparrow/pkg/sparrow
cpu: AMD Ryzen 7 5700X3D 8-Core Processor           
BenchmarkReconcile-16    	{"time":"2025-03-23T19:16:32.8566561+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/checks/health.getHealth","file":"/home/tom/dev/sparrow/pkg/checks/health/health.go","line":213},"msg":"Error while requesting health","url":"https://telekom.com","error":"Get \"https://telekom.com\": context canceled"}
       1	1002994868 ns/op	  102224 B/op	    1315 allocs/op
{"time":"2025-03-23T19:16:32.85666629+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/checks/latency.getLatency","file":"/home/tom/dev/sparrow/pkg/checks/latency/latency.go","line":224},"msg":"Error while checking latency","url":"https://telekom.com","error":"Get \"https://telekom.com\": context canceled"}
--- BENCH: BenchmarkReconcile-16
    controller_test.go:555: Run called for check mockCheck0
    controller_test.go:555: Run called for check mockCheck1
    controller_test.go:555: Run called for check mockCheck2
    controller_test.go:555: Run called for check mockCheck3
    controller_test.go:555: Run called for check mockCheck4
    controller_test.go:555: Run called for check mockCheck5
    controller_test.go:555: Run called for check mockCheck6
    controller_test.go:555: Run called for check mockCheck7
    controller_test.go:555: Run called for check mockCheck8
    controller_test.go:555: Run called for check mockCheck11
	... [output truncated]
PASS
{"time":"2025-03-23T19:16:32.856711708+01:00","level":"WARN","source":{"function":"github.com/telekom/sparrow/pkg/checks/health.(*Health).check.func2","file":"/home/tom/dev/sparrow/pkg/checks/health/health.go","line":182},"msg":"Health check failed after 0 retries","target":"https://telekom.com","error":"Get \"https://telekom.com\": context canceled"}
{"time":"2025-03-23T19:16:32.85673268+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/checks/latency.(*Latency).check.func2","file":"/home/tom/dev/sparrow/pkg/checks/latency/latency.go","line":188},"msg":"Error while checking latency","target":"https://telekom.com","error":"Get \"https://telekom.com\": context canceled"}
{"time":"2025-03-23T19:16:32.856946414+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/checks/health.(*Health).Run","file":"/home/tom/dev/sparrow/pkg/checks/health/health.go","line":66},"msg":"Context canceled","err":"context canceled"}
{"time":"2025-03-23T19:16:32.856996865+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/checks/latency.(*Latency).Run","file":"/home/tom/dev/sparrow/pkg/checks/latency/latency.go","line":68},"msg":"Context canceled","err":"context canceled"}
{"time":"2025-03-23T19:16:32.857020004+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/sparrow.(*ChecksController).RegisterCheck.func1","file":"/home/tom/dev/sparrow/pkg/sparrow/controller.go","line":132},"msg":"Failed to run check","check":"health","error":"context canceled"}
{"time":"2025-03-23T19:16:32.857037246+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/sparrow.(*ChecksController).RegisterCheck.func1","file":"/home/tom/dev/sparrow/pkg/sparrow/controller.go","line":132},"msg":"Failed to run check","check":"latency","error":"context canceled"}
{"time":"2025-03-23T19:16:32.8577985+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/checks/dns.getDNS","file":"/home/tom/dev/sparrow/pkg/checks/dns/dns.go","line":219},"msg":"Error while looking up address","address":"telekom.com","error":"lookup telekom.com on 10.255.255.254:53: dial udp 10.255.255.254:53: operation was canceled"}
{"time":"2025-03-23T19:16:32.857861896+01:00","level":"WARN","source":{"function":"github.com/telekom/sparrow/pkg/checks/dns.(*DNS).check.func2","file":"/home/tom/dev/sparrow/pkg/checks/dns/dns.go","line":185},"msg":"Error while looking up address","target":"telekom.com","error":"lookup telekom.com on 10.255.255.254:53: dial udp 10.255.255.254:53: operation was canceled"}
coverage: 14.5% of statements
{"time":"2025-03-23T19:16:32.857963395+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/checks/dns.(*DNS).Run","file":"/home/tom/dev/sparrow/pkg/checks/dns/dns.go","line":78},"msg":"Context canceled","err":"context canceled"}
{"time":"2025-03-23T19:16:32.858049466+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/sparrow.(*ChecksController).RegisterCheck.func1","file":"/home/tom/dev/sparrow/pkg/sparrow/controller.go","line":132},"msg":"Failed to run check","check":"dns","error":"context canceled"}
ok  	github.com/telekom/sparrow/pkg/sparrow	2.024s
After
go test -benchmem -run=^$ -bench ^BenchmarkReconcile$ github.com/telekom/sparrow/pkg/sparrow -race -cover -count=1

{"time":"2025-03-23T19:17:07.842020443+01:00","level":"INFO","source":{"function":"github.com/telekom/sparrow/pkg/checks/health.(*Health).Run","file":"/home/tom/dev/sparrow/pkg/checks/health/health.go","line":62},"msg":"Starting healthcheck","interval":"1s"}
{"time":"2025-03-23T19:17:07.84206065+01:00","level":"INFO","source":{"function":"github.com/telekom/sparrow/pkg/checks/dns.(*DNS).Run","file":"/home/tom/dev/sparrow/pkg/checks/dns/dns.go","line":74},"msg":"Starting dns check","interval":"1s"}
{"time":"2025-03-23T19:17:07.842053546+01:00","level":"INFO","source":{"function":"github.com/telekom/sparrow/pkg/checks/latency.(*Latency).Run","file":"/home/tom/dev/sparrow/pkg/checks/latency/latency.go","line":64},"msg":"Starting latency check","interval":"1s"}
goos: linux
goarch: amd64
pkg: github.com/telekom/sparrow/pkg/sparrow
cpu: AMD Ryzen 7 5700X3D 8-Core Processor           
BenchmarkReconcile-16    	       1	1003090300 ns/op	  102776 B/op	    1320 allocs/op
--- BENCH: BenchmarkReconcile-16
    controller_test.go:557: Run called for check mockCheck0
    controller_test.go:557: Run called for check mockCheck1
    controller_test.go:557: Run called for check mockCheck2
    controller_test.go:557: Run called for check mockCheck3
    controller_test.go:557: Run called for check mockCheck4
    controller_test.go:557: Run called for check mockCheck5
    controller_test.go:557: Run called for check mockCheck6
    controller_test.go:557: Run called for check mockCheck7
    controller_test.go:557: Run called for check mockCheck8
    controller_test.go:557: Run called for check mockCheck9
	... [output truncated]
PASS
{"time":"2025-03-23T19:17:08.843570608+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/checks/health.getHealth","file":"/home/tom/dev/sparrow/pkg/checks/health/health.go","line":213},"msg":"Error while requesting health","url":"https://telekom.com","error":"Get \"https://telekom.com\": context canceled"}
{"time":"2025-03-23T19:17:08.843795225+01:00","level":"WARN","source":{"function":"github.com/telekom/sparrow/pkg/checks/health.(*Health).check.func2","file":"/home/tom/dev/sparrow/pkg/checks/health/health.go","line":182},"msg":"Health check failed after 0 retries","target":"https://telekom.com","error":"Get \"https://telekom.com\": context canceled"}
{"time":"2025-03-23T19:17:08.84376923+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/checks/latency.getLatency","file":"/home/tom/dev/sparrow/pkg/checks/latency/latency.go","line":224},"msg":"Error while checking latency","url":"https://telekom.com","error":"Get \"https://telekom.com\": context canceled"}
{"time":"2025-03-23T19:17:08.843923275+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/checks/health.(*Health).Run","file":"/home/tom/dev/sparrow/pkg/checks/health/health.go","line":66},"msg":"Context canceled","err":"context canceled"}
{"time":"2025-03-23T19:17:08.843934585+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/checks/latency.(*Latency).check.func2","file":"/home/tom/dev/sparrow/pkg/checks/latency/latency.go","line":188},"msg":"Error while checking latency","target":"https://telekom.com","error":"Get \"https://telekom.com\": context canceled"}
{"time":"2025-03-23T19:17:08.843962444+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/sparrow.(*ChecksController).RegisterCheck.func1","file":"/home/tom/dev/sparrow/pkg/sparrow/controller.go","line":132},"msg":"Failed to run check","check":"health","error":"context canceled"}
{"time":"2025-03-23T19:17:08.844109729+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/checks/latency.(*Latency).Run","file":"/home/tom/dev/sparrow/pkg/checks/latency/latency.go","line":68},"msg":"Context canceled","err":"context canceled"}
{"time":"2025-03-23T19:17:08.844177193+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/sparrow.(*ChecksController).RegisterCheck.func1","file":"/home/tom/dev/sparrow/pkg/sparrow/controller.go","line":132},"msg":"Failed to run check","check":"latency","error":"context canceled"}
{"time":"2025-03-23T19:17:08.844500632+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/checks/dns.getDNS","file":"/home/tom/dev/sparrow/pkg/checks/dns/dns.go","line":219},"msg":"Error while looking up address","address":"telekom.com","error":"lookup telekom.com on 10.255.255.254:53: dial udp 10.255.255.254:53: operation was canceled"}
{"time":"2025-03-23T19:17:08.844567857+01:00","level":"WARN","source":{"function":"github.com/telekom/sparrow/pkg/checks/dns.(*DNS).check.func2","file":"/home/tom/dev/sparrow/pkg/checks/dns/dns.go","line":185},"msg":"Error while looking up address","target":"telekom.com","error":"lookup telekom.com on 10.255.255.254:53: dial udp 10.255.255.254:53: operation was canceled"}
{"time":"2025-03-23T19:17:08.844671836+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/checks/dns.(*DNS).Run","file":"/home/tom/dev/sparrow/pkg/checks/dns/dns.go","line":78},"msg":"Context canceled","err":"context canceled"}
{"time":"2025-03-23T19:17:08.844729455+01:00","level":"ERROR","source":{"function":"github.com/telekom/sparrow/pkg/sparrow.(*ChecksController).RegisterCheck.func1","file":"/home/tom/dev/sparrow/pkg/sparrow/controller.go","line":132},"msg":"Failed to run check","check":"dns","error":"context canceled"}
coverage: 14.5% of statements
ok  	github.com/telekom/sparrow/pkg/sparrow	2.027s

TODO

  • I've assigned this PR to myself
  • I've labeled this PR correctly

lvlcn-t added 2 commits March 23, 2025 17:22
…mance

This commit refactors the 'Iter' methods to return iterators instead of complete slices. We utilize the newly introduced 'iter' standard library package to achieve this. This change improves the performance of the 'Iter' methods by reducing the number of allocations and copying operations.

Signed-off-by: lvlcn-t <[email protected]>
@lvlcn-t lvlcn-t added refactoring Refactoring of existing code area/checks-controller Issues/PRs related to the ChecksController labels Mar 23, 2025
@lvlcn-t lvlcn-t self-assigned this Mar 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/checks-controller Issues/PRs related to the ChecksController refactoring Refactoring of existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant