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 concurrent recording by using a waitgroup #35

Merged

Conversation

stefanotorresi
Copy link
Collaborator

Fixes #34

I should have done this since the beginning: to simplify, RecordConcurrently only sent the first error that occurred among all the concurrent metric recording functions, returning as early as that error occurred.
This could cause some of the goroutines to leak outside the life-cycle of Collect, causing a panic when trying to send on a closed metrics channel.

To be fair I didn't know that, once Collect is done, the metric channel would be closed.

The RecordConcurrently implementation now uses a Waitgroup to wait for all the recording functions, avoiding any of them to leak, and returns all the errors in an array, instead of just the first.

@stefanotorresi
Copy link
Collaborator Author

stefanotorresi commented Apr 7, 2020

I had to add the fmt and vet checks to the test target in the Makefile because I realise that I always forget to run them, and I always use make test. Actually let me do this elsewhere.

@stefanotorresi stefanotorresi force-pushed the fix/34-concurrent-metric-recording branch from 2d9e04d to f48f0a0 Compare April 7, 2020 16:40
@stefanotorresi stefanotorresi force-pushed the fix/34-concurrent-metric-recording branch from f48f0a0 to 06e18fa Compare April 7, 2020 16:56
Copy link
Contributor

@MalloZup MalloZup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@stefanotorresi stefanotorresi merged commit adf4c1f into SUSE:master Apr 8, 2020
@stefanotorresi stefanotorresi deleted the fix/34-concurrent-metric-recording branch April 8, 2020 12:32
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.

Exporter fails with panic error
2 participants