[MM-26385] Include cache and rate limiter to GH client by ethervoid · Pull Request #155 · mattermost/mattermost-mattermod

ethervoid · 2020-07-06T12:05:31Z

Summary

The Github client now is created with two layers on top of it:

The first one is a private LRU cache, using httpcache, that will
check for stored requests/responses and will serve those if needed
The second layer is a rate limiter, using the rate package, that will
allow performing, with the default configuration, 10 requests per
second

Ticket Link

https://mattermost.atlassian.net/browse/MM-26385

The Github client now is created with two layers on top of it: - The first one is a private LRU cache, using httpcache, that will check for stored requests/responses and will serve those if needed - The second layer is a rate limiter, using the rate package, that will allow to perform, with the default configuration, 10 requests per second

ethervoid · 2020-07-07T15:23:32Z

go.mod

 	github.com/gorilla/mux v1.7.4
 	github.com/heroku/docker-registry-client v0.0.0-20190909225348-afc9e1acc3d5
+	github.com/jarcoal/httpmock v1.0.5
+	github.com/m4ns0ur/httpcache v0.0.0-20200426190423-1040e2e8823f


We need this fork/fix until is merged in master

We need a new method that allows as to pass the limiter in order to test it and also can be used to create a different client with different rate limiting policy

agnivade

Amazing piece of work! Just some small comments to take care of.

server/github_client.go

server/github_test.go

server/limit_rate_gh.go

server/github_test.go

metanerd

Smart testing and awesome work! Thank you so much @ethervoid ! This will resolve a ton of issues we are having currently! I added some minor suggestions/nits which you can also just ignore/dismiss.

metanerd · 2020-07-08T09:39:46Z

server/github_test.go

+				body := &github.Reference{Object: &github.GitObject{}}
+				err := json.Unmarshal([]byte(testGetRefReturn), &body)
+				require.NoError(t, err)
+				resp, err := httpmock.NewJsonResponse(200, body)


My IDE says resp could be nil.

Since there are 2 needed responses in this test series, IMO it would improve readability by naming them resp1 and resp2 to make the scheme more obvious. Or put in the name which kind of response it should be. I know golang wants short variable names, but personally, I find it more readable that way, instead of having to read the comment. This is just a suggestion/nit. I leave it up to you.

Can these tests be run in parallel?

In this case, resp is not possible to be nil because we're providing the elements, so no need to check for nil here.

Changing literal 200 for http.StatusOK is a great suggestion and I've included it. Thank you!

Can these tests be run in parallel?

Could be done but given the time spent by these tests, we don't need now to execute them in parallel

metanerd · 2020-07-08T10:53:18Z

server/github_test.go

+				body := &github.Reference{Object: &github.GitObject{}}
+				err := json.Unmarshal([]byte(testGetRefReturn), &body)
+				require.NoError(t, err)
+				resp, err := httpmock.NewJsonResponse(200, body)


Is there a convention? http.StatusOK or 200? So that I know for my future code.

metanerd

Perfect! ❤️

agnivade

Nice. So demo next week? ;)

server/github_client.go

Co-authored-by: Agniva De Sarker <[email protected]>

ethervoid · 2020-07-08T14:50:20Z

@agnivade hahaha yeah sure :D

isacikgoz

LGTM 👍

agnivade · 2020-07-08T15:59:49Z

@ethervoid - Something caught my eye. Any reason the default is set to 10? From what I seeing here https://github.com/google/go-github#rate-limiting, it should be around 1.3. Perhaps then we need to split out the rate with the burst and have 2 separate flags? The word TokenReserve isn't really immediately apparent that it means both.

@metanerd - Just heads up that the GitHubTokenReserve field now denotes both the rate limit and token burst. So the config needs to be updated properly. Perhaps hold off deploying until we have the previous discussion sorted out.

ethervoid · 2020-07-08T16:04:28Z

@agnivade Yes but is being used to provided limit and burst token at the same time. With 10 you get 10 requests/second and 10 burst tokens. We shouldn't limit to the 5000 requests because our rate consumption is not linear so we will be dropping or making requests wait for nothing. And also, taking into account we have the cache as the first layer we shouldn't be reaching the limit.

In summary, I rather prefer to have a broader limit and see what happens that put a very tight one

agnivade · 2020-07-08T16:05:40Z

Ah that makes sense. So the current prod setting is 50. Do you suggest it to be 10 then?

ethervoid · 2020-07-08T16:06:19Z

@agnivade yes. 10 should be more than enough to work perfectly fine without making a bottleneck

agnivade · 2020-07-08T16:07:04Z

Great. @metanerd - over to you. Please change https://github.com/mattermost/platform-private/blob/375ffed89d8a416604378b2d4512ddc91210028a/mattermod/config-mattermod.json#L8 to 10 and let's deploy.

metanerd · 2020-07-08T16:15:08Z

@agnivade done

ethervoid added the Work In Progress Not yet ready for review label Jul 6, 2020

ethervoid added 2 commits July 6, 2020 14:21

Merge branch 'master' into MM-26385

3f03d25

Test cache response in GH client

7c95f81

ethervoid commented Jul 7, 2020

View reviewed changes

ethervoid added 7 commits July 7, 2020 17:59

Test that non-cached response is received when max-age expires

40a357c

More cache transport test cases

999467c

Rate limit tokens must be greater than 0

ca561cd

Fix lint

69639cc

New github client constructor with limiter

7e5dc0f

We need a new method that allows as to pass the limiter in order to test it and also can be used to create a different client with different rate limiting policy

Rate limiting tests

06403c3

Merge branch 'master' into MM-26385

de10007

ethervoid requested review from agnivade, isacikgoz and metanerd July 7, 2020 18:25

ethervoid added 2: Dev Review Requires review by a core committer and removed Work In Progress Not yet ready for review labels Jul 7, 2020

ethervoid changed the title ~~Include cache and rate limiter to GH client~~ [MM-26385] Include cache and rate limiter to GH client Jul 7, 2020

agnivade suggested changes Jul 8, 2020

View reviewed changes

metanerd approved these changes Jul 8, 2020

View reviewed changes

CR suggestions

13a8109

ethervoid requested review from agnivade and metanerd July 8, 2020 12:28

metanerd approved these changes Jul 8, 2020

View reviewed changes

Merge branch 'master' into MM-26385

38ae296

agnivade approved these changes Jul 8, 2020

View reviewed changes

server/github_client.go Outdated Show resolved Hide resolved

ethervoid and others added 2 commits July 8, 2020 16:47

Update server/github_client.go

a94cfc5

Co-authored-by: Agniva De Sarker <[email protected]>

Merge branch 'master' into MM-26385

387bba9

isacikgoz approved these changes Jul 8, 2020

View reviewed changes

isacikgoz added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jul 8, 2020

ethervoid merged commit 376a7de into master Jul 8, 2020

ethervoid deleted the MM-26385 branch July 8, 2020 14:58

Conversation

ethervoid commented Jul 6, 2020

Summary

Ticket Link

Uh oh!

ethervoid Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

agnivade left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metanerd left a comment

Choose a reason for hiding this comment

Uh oh!

metanerd Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

metanerd Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

metanerd Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

ethervoid Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metanerd Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

metanerd left a comment

Choose a reason for hiding this comment

Uh oh!

agnivade left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ethervoid commented Jul 8, 2020

Uh oh!

isacikgoz left a comment

Choose a reason for hiding this comment

Uh oh!

agnivade commented Jul 8, 2020

Uh oh!

ethervoid commented Jul 8, 2020

Uh oh!

agnivade commented Jul 8, 2020

Uh oh!

ethervoid commented Jul 8, 2020

Uh oh!

agnivade commented Jul 8, 2020

Uh oh!

metanerd commented Jul 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ethervoid Jul 8, 2020 •

edited

Loading