Implemented Graphql requests for Github PR's, Issues and comments scanning #4431

kashifkhan0771 · 2025-08-29T10:28:10Z

Description:

This Pull request replaces the HTTP API requests for Github source PR's, Issues and their comments with GraphQL requests.

Scan Results with no verification:

Note: While results may vary slightly between runs, GraphQL consistently outperforms REST in scan duration.

Checklist:

Tests passing (make test-community)?
Lint passing (make lint this requires golangci-lint)?

rosecodym

I was initially skeptical of putting the new code in graphql.go, but I think I see why you did it and I can't think of a better option.

However, I did have some trouble following the rate limiting code - in particular, how it interacts with the rest of the GitHub rate limiting code. Specifically, it's doing a lot of its own stuff before it begins to use the rest of the rate limiting code, and I don't understand why it needs to do that beforehand. (Also, mentioned inline, 5 minutes is a huge interval, and Prometheus gets skipped in many cases.)

Can you try another pass over the rate limiting code to see if things can be clarified a bit?

rosecodym · 2025-09-04T13:58:43Z

pkg/sources/github/graphql.go

+		ctx.Logger().Info("GraphQL RATE_LIMITED error (fallback)",
+			"retry_after", retryAfter.String())
+		time.Sleep(retryAfter)
+		return true


This early return (and all of the later ones) happen before we report rate limiting to Prometheus, which means that we won't have much visibility when it happens. Relatedly, I find it a bit hard to follow this new rate limiting code - I suspect the difficulty was with integrating it with the existing rate limiting code. (For example: Why does this initial request have a 3x 5minute retry step before getting into the global rate limiting code?) You might be able to resolve both issues at once with another refactoring pass at this function.

I overlooked the reporting to Prometheus. Thanks for pointing out!

The reason I initially set the request interval to 5 minutes was because the GraphQL API sometimes returns a rate limit error before the remaining value reaches 0. When this happens, the response includes no rate limit info, so we can't rely on the usual rate limit object and the resetAt value defaults to zero, which can cause incorrect handling.

To deal with this, I first handled the error case directly, then move on to process the rate limit if available.

Your comment gave me a new perspective, so I adjusted the approach a bit and now:

We first check if the global rate limit is already set.

If it's set, we use that value and delay accordingly.

If it's not set, and we receive an error:

We check if it's a rate limit issue.

In that case, we set the global rate limit (so other requests know to wait) and continue reporting to Prometheus.

If there's no error, we check the actual rate limit object:

If remaining < 3, we wait until the resetAt time.

Why 3? Because some GraphQL queries can cost more than one point, depending on complexity.

rosecodym · 2025-09-04T13:58:55Z

pkg/sources/github/graphql.go

+func (s *Source) handleGraphQLRateLimit(ctx context.Context, rl *rateLimit, errIn error, reporters ...errorReporter) bool {
+	// if rate limit exceeded error happened, wait for 5 minute before trying again
+	if errIn != nil && strings.Contains(errIn.Error(), "rate limit exceeded") {
+		retryAfter := 5 * time.Minute


This is a huge retry interval. Does GitHub prescribe it?

Answered in the above comment.

camgunz

Broadly looks good, just had a couple notes. We should add tests for this--probably the integration tests should cover this (maybe a unit test or two for chunkIDs), so if we just run the integration tests w/ UseGithubGraphqlAPI both false and true, that'd give us even more confidence we're not breaking anything.

camgunz · 2025-09-09T10:30:12Z

main.go

 	feature.GitlabProjectsPerPage.Store(100)

+	// OSS Default using github graphql api for issues, pr's and comments
+	feature.UseGithubGraphqlAPI.Store(true)


Let's start this off as false so it's not automatically turned on for EE customers (we can/should make it true for OSS after we get the flag in EE)

Also, regretfully I think the initialism is GraphQL--if we're gonna do it for REST we should probably do it here too

Sure, I'll temporarily turn it off. I didn't quite understand your second comment - just to clarify, we don’t use any initialism for REST.

Oh I was just saying the "QL" is capitalized; Graphql -> GraphQL

camgunz · 2025-09-09T10:32:52Z

pkg/sources/github/github.go

 )

-func (s *Source) processRepoComments(ctx context.Context, repoInfo repoInfo, reporter sources.ChunkReporter, cutoffTime *time.Time) error {
+func (s *Source) processIssueandPRsWithCommentsREST(ctx context.Context, repoInfo repoInfo, reporter sources.ChunkReporter, cutoffTime *time.Time) error {


I'm an 80 columns person because I have a bunch of buffers open side-by-side, but I accept I'm a relic from an earlier time. This one (and at least one more below) are > 150 though--can we wrap somewhere between 90 and 120?

camgunz · 2025-09-09T10:40:13Z

pkg/sources/github/github.go

-		return s.processRepoComments(ctx, repoInfo, reporter, cutoffTime)
+		// if we need to use graphql api for repo issues, prs and comments
+		if feature.UseGithubGraphqlAPI.Load() {
+			return s.processRepoIssueandPRsWithCommentsGraphql(ctx, repoInfo, reporter, cutoffTime)


(No need to do anything here, just musing)

Hrm, w/ the old code passing cutoffTime just the once was kind of 🤷🏻 , but now that we're carrying it around everywhere, it makes me think it'd be nice if we processed s.commentsTimeframeDays up top in Init, that way we don't need to drill it down everywhere.

Sound's good we can do it in a separate optimization PR.

camgunz · 2025-09-09T11:49:59Z

pkg/sources/github/graphql.go

+				return err
+			}
+
+			// if a pull request has more than 100 comments - some interns pull request might endup here :)


kashifkhan0771 · 2025-09-11T06:47:49Z

We should add tests for this--probably the integration tests should cover this (maybe a unit test or two for chunkIDs), so if we just run the integration tests w/ UseGithubGraphqlAPI both false and true, that'd give us even more confidence we're not breaking anything.

I added the test but even the existing integration tests are not working for me 😿

martinlocklear

Left a couple of comments about rate limiting, and a strong opinion about tabular tests. (I've been drawing folks attention to testify's testsuite functionality to serve similar purposes but be more maintainable).

In general I think PRs this size are too large to effectively review - nothing stands out to me as a problem though.

martinlocklear · 2025-10-17T15:46:04Z

pkg/sources/github/graphql.go

+				// request this issue more comments
+				var commentsQuery singleIssueComments
+				err := s.connector.GraphQLClient().Query(ctx, &commentsQuery, commentVars)
+				if s.handleGraphqlRateLimitWithChunkReporter(ctx, reporter, &query.RateLimit, err) {


Suggested change

if s.handleGraphqlRateLimitWithChunkReporter(ctx, reporter, &query.RateLimit, err) {

if s.handleGraphqlRateLimitWithChunkReporter(ctx, reporter, &commentsQuery.RateLimit, err) {

Not sure if this is what you want or not.

Good catch 👀

martinlocklear · 2025-10-17T15:52:11Z

pkg/sources/github/graphql.go

+				}
+
+				err := s.connector.GraphQLClient().Query(ctx, &commentQuery, singlePRVars)
+				if s.handleGraphqlRateLimitWithChunkReporter(ctx, reporter, &query.RateLimit, err) {


Suggested change

if s.handleGraphqlRateLimitWithChunkReporter(ctx, reporter, &query.RateLimit, err) {

if s.handleGraphqlRateLimitWithChunkReporter(ctx, reporter, &commentQuery.RateLimit, err) {

Similar to another comment I made. It's not obvious to me from looking here which rate limit we should be respecting. Some inline comments might help.

If we aren't interested in respecting separate rate limits for each query object separately, I don't understand why the rate limit object is attached to the query object. (maybe out of scope for this?).

We want to make sure we're respecting the rate limits for single-query objects, like the comments on a single PR. I mistakenly used the wrong rate limit in this case thanks for catching that. While it's rare to hit the rate limit on these individual queries, it's still important to get it right.

martinlocklear · 2025-10-17T15:52:52Z

pkg/sources/github/graphql.go

+				commentsPagination: (*githubv4.String)(nil),
+			}
+
+			if err := s.connector.GraphQLClient().Query(ctx, &query, reviewThreadVars); err != nil {


Does this need to be rate limited as well?

martinlocklear · 2025-10-17T15:55:18Z

pkg/sources/github/graphql.go

+	}
+
+	var retryAfter time.Duration
+	// if rate limit exceeded error happened, wait for 5 minute before trying again


I think this comment is out of date now.

martinlocklear · 2025-10-17T16:01:27Z

pkg/sources/github/github_integration_test.go

+		numExpectedChunks int
+	}{
+		{
+			name: "unauthenticated, single repo, issue comments and pr comments",


Please avoid using tabular tests like this. They end up being hard to understand and even harder to maintain. Breaking these out into individual test functions (or even a new test suite) will make our lives easier in the future. imo, it's just as easy to copypasta a test function as it is to copypasta an item in a list of tests, but it's harder to manage as code paths diverge when they're not broken out.

The github_integration_test.go file uses tabular test cases throughout. If we want to update the test case structure, we should apply the change consistently across all test cases. It might be better to create a separate ticket or PR for that cleanup.

kashifkhan0771 and others added 11 commits August 28, 2025 20:18

Added graphql API for issues and PRs

ac2c609

Merge branch 'main' into feature/oss-209

48a35a5

enhancements

f827313

more cleanup

24571ec

more enhancements

5d96170

some final touches

cda3e75

some more cleanup

f2ff9ca

Merge branch 'main' into feature/oss-209

a9dc2e0

tweaked threads vars

871a490

minor changes

3f78ee1

scan the markdown text not plain text

a96a2ea

kashifkhan0771 marked this pull request as ready for review September 1, 2025 13:36

kashifkhan0771 requested review from a team as code owners September 1, 2025 13:36

kashifkhan0771 marked this pull request as draft September 1, 2025 13:36

kashifkhan0771 self-assigned this Sep 1, 2025

kashifkhan0771 added 2 commits September 2, 2025 16:06

ratelimit handling

1b0af83

added ratelimit handling

e6fff98

kashifkhan0771 force-pushed the feature/oss-209 branch from 5890953 to e6fff98 Compare September 2, 2025 13:34

lint error fix

0b738a4

kashifkhan0771 force-pushed the feature/oss-209 branch from 451a1aa to 0b738a4 Compare September 2, 2025 13:37

kashifkhan0771 added 2 commits September 2, 2025 18:44

Merge branch 'main' into feature/oss-209

2ab5514

refactored the thread review comments chunking

9b9761e

kashifkhan0771 force-pushed the feature/oss-209 branch from 274a842 to 9b9761e Compare September 3, 2025 08:19

kashifkhan0771 added 2 commits September 3, 2025 14:59

Merge branch 'main' into feature/oss-209

2fdf5cd

final commit - probably

c3a4de9

kashifkhan0771 force-pushed the feature/oss-209 branch from 6c487cb to c3a4de9 Compare September 3, 2025 10:55

kashifkhan0771 marked this pull request as ready for review September 3, 2025 11:03

kashifkhan0771 requested a review from a team as a code owner September 3, 2025 11:03

rosecodym reviewed Sep 4, 2025

View reviewed changes

Merge branch 'main' into feature/oss-209

f6445af

kashifkhan0771 force-pushed the feature/oss-209 branch from 24cd3cc to f6445af Compare September 5, 2025 06:08

kashifkhan0771 and others added 3 commits September 5, 2025 11:28

refactored the rate limit handling

53bc5f1

adjusted comments

4ddb045

Merge branch 'main' into feature/oss-209

ed85e32

camgunz requested changes Sep 9, 2025

View reviewed changes

kashifkhan0771 and others added 5 commits September 9, 2025 21:20

Merge branch 'main' into feature/oss-209

aa88342

Merge branch 'main' into feature/oss-209

531985f

Merge branch 'main' into feature/oss-209

5d00b5a

resolved comments

d420cc2

remove old camel case func

b993a03

kashifkhan0771 requested review from camgunz and rosecodym September 11, 2025 06:48

renamed featureflag

3d2fd13

GoBlaq1604 approved these changes Sep 16, 2025

View reviewed changes

martinlocklear reviewed Oct 17, 2025

View reviewed changes

resolved martin's comments

615cc9f

kashifkhan0771 force-pushed the feature/oss-209 branch from b9ce711 to 615cc9f Compare October 20, 2025 06:29

kashifkhan0771 added 2 commits October 22, 2025 11:31

Merge branch 'main' into feature/oss-209

434e92a

updated test cases

8a5b356

kashifkhan0771 force-pushed the feature/oss-209 branch from 56f0ebb to 8a5b356 Compare October 22, 2025 06:41

Merge branch 'main' into feature/oss-209

0c4cda5

	if s.handleGraphqlRateLimitWithChunkReporter(ctx, reporter, &query.RateLimit, err) {
	if s.handleGraphqlRateLimitWithChunkReporter(ctx, reporter, &commentsQuery.RateLimit, err) {

Implemented Graphql requests for Github PR's, Issues and comments scanning #4431

Are you sure you want to change the base?

Implemented Graphql requests for Github PR's, Issues and comments scanning #4431

Uh oh!

Conversation

kashifkhan0771 commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Scan Results with no verification:

Checklist:

Uh oh!

rosecodym left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camgunz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kashifkhan0771 commented Sep 11, 2025

Uh oh!

martinlocklear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kashifkhan0771 commented Aug 29, 2025 •

edited

Loading