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: scope metric variables to collector function #3

Merged
merged 7 commits into from
Dec 14, 2023

Conversation

wbollock
Copy link
Collaborator

@wbollock wbollock commented Dec 11, 2023

This helps fix some concurrency problems by scoping and instantiating metric variables to each probe.

It also tightens up the ping_success logic to closely match man ping and provide Info logs on ping timeout or other failures

Lastly there is a refactor to reduce the long parameter list in the collector function

This helps fix some concurrency problems by registering the metrics once
outside the collector and introducing a mutex lock when values get
updated
this cuts down on the long parameter list in the collector function
@wbollock wbollock marked this pull request as ready for review December 13, 2023 16:39
@wbollock wbollock changed the title ref: register metrics and lock outside collector fix: register metrics and lock outside collector Dec 13, 2023
Turns out the entire contention/value leakage issue was just due to
these globals in the icmp collector function
@wbollock wbollock changed the title fix: register metrics and lock outside collector fix: scope metric variables to collector function Dec 13, 2023
internal/collector/icmp_collector.go Outdated Show resolved Hide resolved
internal/collector/icmp_collector.go Outdated Show resolved Hide resolved
Co-authored-by: Will Hegedus <[email protected]>
@wbollock wbollock merged commit b0c0689 into main Dec 14, 2023
2 checks passed
@wbollock wbollock deleted the fix/concurrency_lock branch December 14, 2023 22:54
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