Skip to content

feat(support): fetch data concurrently #12385

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 5, 2024
Merged

feat(support): fetch data concurrently #12385

merged 3 commits into from
Mar 5, 2024

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented Mar 1, 2024

Fetches data concurrently to speed up process of generating a support bundle.

@johnstcn johnstcn requested a review from mafredri March 1, 2024 18:26
@johnstcn johnstcn self-assigned this Mar 1, 2024
mafredri
mafredri reviewed Mar 1, 2024
View reviewed changes
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

I think it works as-is, but could be simplified quite a bit with errgroup.

@johnstcn johnstcn force-pushed the cj/support-concurrent branch from 4e123d4 to b0cd05a Compare March 5, 2024 14:22
mafredri
mafredri approved these changes Mar 5, 2024
View reviewed changes
support/support.go Outdated
var (
wg sync.WaitGroup
m sync.Mutex
)
Copy link
Member

Choose a reason for hiding this comment

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

Switch for errgroup too?

support/support.go Outdated
if err != nil {
log.Error(ctx, "fetch build info", slog.Error(err))
} else {
eg, ctx := errgroup.WithContext(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

Just realized we only log errors, so we might be interested in partial results?

If so, might make sense to just use eg := &errgroup.Group{}. This way the other Go routines won't be interrupted when one runs into an error. (Sorry I didn't take this into account in my previous review.)

mtojek
mtojek approved these changes Mar 5, 2024
View reviewed changes
Copy link
Member

@mtojek mtojek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

👍

out of curiosity: how much time do we save by switching to the concurrent mode?

@johnstcn
Copy link
Member Author

johnstcn commented Mar 5, 2024

👍

out of curiosity: how much time do we save by switching to the concurrent mode?

We're going from about 13 serial requests right now to potentially having 12/13 of those in parallel. So quite a bit, depending on round trip time.

@mtojek
Copy link
Member

mtojek commented Mar 5, 2024

I hope we don't hit any Coder rate limiter or customer's proxy :)

@johnstcn
Copy link
Member Author

johnstcn commented Mar 5, 2024 •
edited
Loading

I hope we don't hit any Coder rate limiter or customer's proxy :)

Fair, I'll set max concurrency to 4 as our default rate limit is 8 requests and change per second.

Edit: actually, if folks run into issues with this we can just set GOMAXPROCS

dannykopping
dannykopping approved these changes Mar 5, 2024
View reviewed changes
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

Neat 👍
Do we have any idea how long these steps take in practice?
Do we need timing info for each step? Presumably we're parallelising the steps because the fetching currently takes too long.

@dannykopping
Copy link
Contributor

I hope we don't hit any Coder rate limiter or customer's proxy :)

Fair, I'll set max concurrency to 4 as our default rate limit is 8 requests and change per second.

Edit: actually, if folks run into issues with this we can just set GOMAXPROCS

Something to keep in mind:
GOMAXPROCS defaults to # of processors unless explicitly set, and since this will presumably be run on beefy servers we can expect this figure to be quite high (> than the 13 steps, surely).

We should probably use https://github.com/uber-go/automaxprocs unless it's been disregarded previously.

@johnstcn
Copy link
Member Author

johnstcn commented Mar 5, 2024

Neat 👍 Do we have any idea how long these steps take in practice? Do we need timing info for each step? Presumably we're parallelising the steps because the fetching currently takes too long.

The rationale for concurrent fetching is mainly from this comment

I don't think we need timing info for the moment; and it can be inferred from the debug-level client logs that get emitted (sdk request vs sdk response).

@johnstcn
Copy link
Member Author

johnstcn commented Mar 5, 2024

We should probably use https://github.com/uber-go/automaxprocs unless it's been disregarded previously.

I've been on the fence about adding this; it's been bugging me for a while that the Go runtime isn't CFS-aware. It would be a big enough change though. Maybe a separate PR?

@johnstcn johnstcn merged commit 5106d9f into main Mar 5, 2024
@johnstcn johnstcn deleted the cj/support-concurrent branch March 5, 2024 17:41
@github-actions github-actions bot locked and limited conversation to collaborators Mar 5, 2024
@mafredri
Copy link
Member

mafredri commented Mar 5, 2024

We had a PR for enabling automaxprocs in the agent, but it seems to have fallen into oblivion. I'm all for it, though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mafredri mafredri mafredri approved these changes

@dannykopping dannykopping dannykopping approved these changes

@mtojek mtojek mtojek approved these changes

Assignees

@johnstcn johnstcn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@johnstcn @mtojek @dannykopping @mafredri