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

Concurrently fetch metrics #12

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

Conversation

AnshulMalik
Copy link

Fixes #11

@fogfish
Copy link
Member

fogfish commented Apr 8, 2024

👍

@fogfish
Copy link
Member

fogfish commented Apr 8, 2024

@pioneerit can you please also review the proposed changes.

@pioneerit
Copy link
Member

As for the PR LGTM.
Perhaps logic with error handling and exit on goroutine via <-ctx.Done() is something I'd double-check. AFAIK the cancel itself won't cause a stop.
At the same time by reading the code, if I understand the idea - the goal is to stop and exit on the first error.

I also looking that our test case didn't cover multiple metrics, so we're not testing concurrency at all.

@fogfish last one, not related to this PR, but just spot a magic number chunkSize. We don't have any comments or docs to it, and the values inside the func, is not the best practice. I'd create small issue and follow up on it.

P.S. I can try to help with test on Wednesday, as today and tomorrow pretty booked already

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.

Make concurrent fetch of samples
3 participants