Tool to calculate the average time that takes for a GitHub pull request to get merged

Question

I'm learning Go, and this is my first attempt at a command-line tool that uses the GitHub API to calculate the average time that takes for a pull request to get merged for a particular organisation or repository.

The algorithm follows a very simple approach:

1. Get all the repositories for an organisation.
2. For each repository, get all the closed pull requests.
3. Calculate the average time using merged_at and created_at of each pull request.

For large organisations/projects, step 2 can take a long time, especially if it can't get all the pull requests from a single result page. So I want to execute that part concurrently.

Here's the part of the code that does that: GitHub

c := make(chan Result, len(repos))
var wg sync.WaitGroup
for _, repo := range repos {
    wg.Add(1)
    go func(val Repository) {
        if cmd.Debug {
            fmt.Printf("Executing goroutine for value: %s", val.Name)
        }
        pullRequests, err := getPullRequests(ctx, client, val)
        c <- Result{PullRequests: pullRequests, Err: err}
        wg.Done()
    }(repo)
}
wg.Wait()
close(c)

timeAccumulator := float64(0)
prAccumulator := int64(0)

var cmdErr error
for result := range c {
    pullRequests, err := result.PullRequests, result.Err
    if err != nil {
        cmdErr = err
        break
    }
    for _, pullRequest := range pullRequests {
        if pullRequest.GetMergedAt().IsZero() {
            continue
        }
        delta := pullRequest.GetMergedAt().Sub(pullRequest.GetCreatedAt()).Hours()
        timeAccumulator += delta
        prAccumulator++
        if cmd.Debug {
            fmt.Printf("PR: %s\nCreated at: %v\nMerged at:%v\nDelta in hours: %f\n", pullRequest.GetTitle(), pullRequest.GetCreatedAt(), pullRequest.GetMergedAt(), delta)
        }
    }
}

It works, but I'd like to get some feedback about the WaitGroup usage. It does feel that I don't need it here, because looping over range c would take results until the channel is closed. It's just not clear to me when I should really close the channel without a WaitGroup.

And of course any other feedback is really welcomed!

Answer 1

I think you're using the WaitGroup correctly, and you're right that you need to wait to know when to close the channel. A few things, however.

You're creating a buffered channel that will store all of the repos in memory and wait until all of them are fetched before it starts processing them. The processing in this case is likely very fast (just some math), but it'd be more memory-efficient and better practice to start processing them concurrently as they come in. I think you can do this by putting the wg.Wait(); close(c) in a goroutine, so that execution will continue to the processing code immediately:

c := make(chan Result) // note: no buffering now
var wg sync.WaitGroup
for _, repo := range repos {
    // ...
}
go func() {
   wg.Wait()
   close(c) 
}()

totalHours := 0.0 // 0.0 is perhaps more idiomatic than float64(0)
numPRs := 0       // it would be more common to use a plain int here
for result := range c {
    // ...
}

I've sneakily renamed your "accumulator" variables to what I think are shorter, clearer names. :-)

However, with an unbuffered channel, you'll want to be careful to "drain" the channel in case of error, otherwise the outstanding goroutines will stay alive. Perhaps the following:

for result := range c {
    if cmdErr != nil {
        continue
    }
    pullRequests, err := result.PullRequests, result.Err
    if err != nil {
        cmdErr = err
        continue
    }
    // ...
}

Another thing to note is that you're not limiting the concurrency. If you have 1000 repos it'll blindly fire up 1000 goroutines and start 1000 HTTP requests to the GitHub API -- probably not a good idea, and GitHub will probably rate-limit you anyway. You probably want to limit the concurrency to say 10 or 20. Here is a good way to do that with a buffered semaphore channel (though you can use a chan of struct{} rather than bool to save a tiny bit of memory). Note that his second for loop also means you don't need a WaitGroup (though you'll want to do that bit in a goroutine, like we did above).

I'm not sure how you're fetching the pull requests, as you don't show the code for getPullRequests, but using the GitHub API you should be able to filter to only fetch state=closed pull requests. If the repo has a lot of open pull requests, this would avoid fetching quite so many (only to filter them out in code).

One other thing: now that we're processing concurrently, since you want to exit early on the first error, it'd be good to cancel not-yet-started or in-flight requests. I'll leave this as an exercise to the reader :-), but you should be able to create a cancellable Context and cancel it when the first error occurs. You'll probably also want to explicitly check ctx.Err() before starting each repo goroutine, to avoid starting remaining goroutines only to cancel them right away.