Share a single vcgencmd runner between temperature and throttled collectors - #96
Merged
Conversation
…collectors TemperatureCollector and ThrottledCollector each implemented their own lazy vcgencmd detection/re-detection logic (exec.LookPath, throttled retry) verbatim. Extract that into a shared vcgencmdRunner type used by both, so the duplicated code collapses to one place and vcgencmd is looked up once (shared across both collectors) instead of once per collector. True single-exec batching of measure_temp and get_throttled isn't possible: vcgencmd only accepts one subcommand per invocation, so both collectors still shell out separately on the hot path. This change targets the actionable part of issue #66 - the duplicated detection/re-detection logic - rather than a batching scheme vcgencmd doesn't support.
The previous commit's tests never actually ran a subprocess through vcgencmdRunner.run(): every test kept vcg nil or pre-set "unavailable", so the real exec.CommandContext success/failure paths, and exec.LookPath's success path, were untested new code and tripped SonarCloud's coverage gate. Add a writeFakeVcgencmd test helper (mirroring the existing aptPath: "true" stub-command pattern in updates_test.go) that writes a small executable shell script standing in for vcgencmd, and use it to exercise: vcgencmdRunner.run() executing successfully and failing, redetectLocked() finding a binary via PATH, and the resulting success/failure/malformed-output behavior through TemperatureCollector.Collect and ThrottledCollector.Collect end to end.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📖 Description
TemperatureCollector(measure_temp) andThrottledCollector(get_throttled) each implemented their own copy of the lazy vcgencmd detection/re-detection logic (exec.LookPath("vcgencmd"), throttled retry viadetectRetryInterval) — duplicated verbatim between the two files.This extracts a shared
vcgencmdRunnertype (internal/collector/vcgencmd.go) that owns detection/re-detection and arun(ctx, subcommand)helper, and has both collectors delegate to a single shared instance (constructed once incollector.New). The duplicated logic now lives in one place, and vcgencmd is looked up once total instead of once per collector.Scope note on the issue's "halves the exec rate" suggestion: true single-exec batching of
measure_tempandget_throttledisn't possible —vcgencmdonly accepts one subcommand per invocation, so both collectors still shell out separately on the fast-tick hot path (this is unchanged from before). This PR targets the actionable, achievable part of the finding: eliminating the duplicated detection logic and the redundantexec.LookPathcalls.🎫 Issues
Closes #66
👩💻 Reviewer Notes
internal/collector/vcgencmd.gois the new shared type;temperature.goandthrottled.gowere trimmed down to delegate to it via avcg *vcgencmdRunnerfield.ThrottledCollectorandTemperatureCollectorboth treat anilvcg(or an unresolved vcgencmd) the same way as before: no GPU/throttled reading, no error — off-Pi/dev-machine behavior is unchanged.vcgencmd//proc//sysaccess in tests, perdocs/TESTS.md).📑 Test Plan
internal/collector/vcgencmd_test.gocovering the new runner: unavailable-without-exec, throttled re-detection, and re-detection after the throttle window elapses.temperature_test.go/throttled_test.gofor the new field shape (vcg *vcgencmdRunnerinstead of per-collector vcgencmd fields); addedTestThrottledCollector_Collect_NilRunner.go build ./...,go vet ./...,go test ./... -race -cover, andgolangci-lint runall pass locally.✅ Checklist
General
go test ./... -race -coverpasses locally).go vet ./...andgolangci-lint runare clean.ARCHITECTURE.mdif this changes a documented design decision. — Not needed: the architecture doc's description of the collector/vcgencmd relationship (oneCollect()method per metric family, optionally shelling out tovcgencmd) is unchanged; this is an internal implementation detail.REST API / configuration / packaging
Not applicable — no REST API, configuration, or packaging changes.
⏭ Next Steps
None.