Feat-Implement httpcache middleware for GitHub API #203

naveensrinivasan · 2021-02-21T14:39:19Z

Please check if the PR fulfills these requirements

Tests for the changes have been added (for bug fixes/features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature - Caching
What is the current behavior? (You can also link to an open issue here)
Scorecard scalability limitation: Reduce GitHub API calls #80 Reducing GitHub API calls to scale scanning repositories #202
What is the new behavior (if this is a feature change)?
The GitHub API supports conditional requests
https://docs.github.com/en/rest/overview/resources-in-the-rest-api#conditional-requests

https://github.com/google/go-github supports Conditional requests
https://github.com/google/go-github#conditional-requests

As we are scaling more and more projects this would add a lot of value.

Initial run fetches information using httpcache as a middleware,
which caches the HTTP response initially in a large disk (PVC),
probably move to Redis later as a cache instead of disk.

Subsequent cron runs will utilize the httpcache for checking content modification and
load it from the cache if it isn't modified, which reduces the hitting the
Rate Limit of the GitHub API.

Also fixed the golang-ci warnings.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
None
Other information:

Subsequent cache runs on 50 repositories takes about 18 minutes with 3 GitHub tokens

❯ time ./cron.sh
github.com/18F/identity-idp
github.com/1N3/Sn1per
github.com/24pullrequests/24pullrequests
github.com/AElfProject/AElf
github.com/Activiti/Activiti
github.com/AdguardTeam/AdguardFilters
github.com/AlchemyCMS/alchemy_cms
github.com/AliasIO/wappalyzer
github.com/Amanieu/parking_lot
github.com/abonas/kubeclient
github.com/abpframework/abp
github.com/acassen/keepalived
github.com/acmesh-official/acme.sh
github.com/actions/runner
github.com/activeadmin/activeadmin
github.com/activemerchant/active_merchant
github.com/activerecord-hackery/ransack
github.com/activescaffold/active_scaffold
github.com/actix/actix
github.com/actix/actix-extras
github.com/actix/actix-net
github.com/actix/actix-web
github.com/adafruit/Adafruit_CircuitPython_Bundle
github.com/adempiere/adempiere
github.com/afragen/github-updater
github.com/aframevr/aframe
github.com/ai/autoprefixer-rails
github.com/aio-libs/aiohttp
github.com/airbnb/javascript
github.com/airbrake/airbrake
github.com/akaunting/akaunting
github.com/akeneo/pim-community-dev
github.com/akka/akka
github.com/akka/akka-http
github.com/akkadotnet/akka.net
github.com/alacritty/alacritty
github.com/alanmcgovern/monotorrent
github.com/alexcrichton/cc-rs
github.com/alexcrichton/curl-rust
github.com/alexcrichton/proc-macro2
github.com/alexcrichton/toml-rs
github.com/alexreisner/geocoder
github.com/alibaba/dragonwell8
github.com/alibaba/druid
github.com/alibaba/fastjson
github.com/alibaba/nacos
github.com/allenai/allennlp
github.com/allinurl/goaccess
github.com/alphagov/whitehall
github.com/alpinelinux/aports
./cron.sh  46.68s user 19.95s system 6% cpu 18:26.70 total

Folder size

du -sh ./cache
801M

Files in the folder

find ./cache -type f | wc -l
10347

naveensrinivasan · 2021-02-21T14:40:28Z

go.mod

@@ -7,10 +7,11 @@ require (
 	github.com/golang/protobuf v1.4.3 // indirect
 	github.com/google/go-github/v32 v32.1.0
 	github.com/kr/text v0.2.0 // indirect
+	github.com/naveensrinivasan/httpcache v1.2.1


This is a fork of gregjones/httpcache#104 this PR.

github-actions · 2021-02-21T14:41:15Z

Integration tests failure for 35f3b88dbb4e5dad84ca2916dc6db3d8e7e32d05

github-actions · 2021-02-21T14:43:19Z

Integration tests success for 35f3b88dbb4e5dad84ca2916dc6db3d8e7e32d05

github-actions · 2021-02-21T16:54:41Z

Integration tests success for 73ebc1a54963240d4ff9241dce169df0f5131478

github-actions · 2021-02-21T23:49:14Z

Integration tests failure for e5b609b096fc67b393e928b3245a122f3919ef31

github-actions · 2021-02-21T23:52:28Z

Integration tests success for e5b609b096fc67b393e928b3245a122f3919ef31

dlorenc

A couple tiny nits, LGTM!

dlorenc · 2021-02-22T16:24:35Z

README.md

@@ -153,6 +152,20 @@ Signed-Releases: Fail 0
 Signed-Tags: Fail 10
 ```

+### Caching
+
+Scorecard uses `httpcache` with <https://docs.github.com/en/rest/overview/resources-in-the-rest-api#conditional-requests> for caching httpresponse. The default cache is in-memory.


Maybe link to the etags stuff in GitHub - the real benefit is avoiding the API quota

dlorenc · 2021-02-22T16:25:48Z

roundtripper/roundtripper.go

+// shouldUseDiskCache checks the env variables USE_DISK_CACHE and DISK_CACHE_PATH to determine if
+// disk should be used for caching.
+func shouldUseDiskCache() (string, bool) {
+	if isDiskCache := os.Getenv(UseDiskCache); isDiskCache != "" {


nit: I think you can avoid this if statement and go straight into ParseBool since "" parses as false.

inferno-chromium

Thanks a lot! This is very exciting.

inferno-chromium · 2021-02-22T16:21:16Z

.gitignore

@@ -23,3 +23,6 @@ results.json

 # tools 
 bin
+
+#temp


remove this.

inferno-chromium · 2021-02-22T16:29:58Z

README.md

+
+To use disk cache two env variables have to be set `USE_DISK_CACHE=true` and `DISK_CACHE_PATH=./cache`.
+
+There are not TTL on cache.


"There is no"

inferno-chromium · 2021-02-22T16:31:01Z

roundtripper/roundtripper.go

+			}
+		}
+	}
+	return "", false


nit: maybe nil instead of ""

can't do nil for a string in go

inferno-chromium · 2021-02-22T16:32:36Z

roundtripper/roundtripper_test.go

+	t.Parallel()
+	tests := []struct {
+		name  string
+		want  string


s/want/diskCachePath
s/want1/useDiskCache

The GitHub API supports conditional requests https://docs.github.com/en/rest/overview/resources-in-the-rest-api#conditional-requests https://github.com/google/go-github supports Conditional requests https://github.com/google/go-github#conditional-requests As we are scaling more and more projects this would add a lot of value. Initial run fetches information using `httpcache` as a middleware, which caches the HTTP response initially in a large disk (PVC), probably move to Redis later as a cache instead of disk. Subsequent `cron runs` will utilize the `httpcache` for checking content modification and load it from the cache if it isn't modified, which reduces the hitting the Rate Limit of the GitHub API.

github-actions · 2021-02-22T17:18:19Z

Integration tests success for 9645825ac6716a3a988b682ddc00145bb62695df

* set GITHUB_TOKEN as default token * updates * Update doc * Update doc * updates * updates * update * update * update * update * updates

naveensrinivasan requested review from dlorenc and inferno-chromium February 21, 2021 14:39

naveensrinivasan self-assigned this Feb 21, 2021

naveensrinivasan commented Feb 21, 2021

View reviewed changes

naveensrinivasan mentioned this pull request Feb 21, 2021

Move to more reliable and scalable caching solution #205

Closed

dlorenc reviewed Feb 22, 2021

View reviewed changes

inferno-chromium approved these changes Feb 22, 2021

View reviewed changes

naveensrinivasan force-pushed the feat/caching branch from e5b609b to 9645825 Compare February 22, 2021 17:15

naveensrinivasan enabled auto-merge (squash) February 22, 2021 17:17

naveensrinivasan merged commit db81680 into main Feb 22, 2021

naveensrinivasan mentioned this pull request May 10, 2021

🌱 Setup PubSub framework code #428

Merged

2 tasks

naveensrinivasan deleted the feat/caching branch January 14, 2022 22:52

justaugustus pushed a commit to justaugustus/scorecard that referenced this pull request May 25, 2022

✨ Update documentation (ossf#203)

1c09180

* set GITHUB_TOKEN as default token * updates * Update doc * Update doc * updates * updates * update * update * update * update * updates

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat-Implement httpcache middleware for GitHub API #203

Feat-Implement httpcache middleware for GitHub API #203

naveensrinivasan commented Feb 21, 2021

naveensrinivasan Feb 21, 2021

github-actions bot commented Feb 21, 2021

github-actions bot commented Feb 21, 2021

github-actions bot commented Feb 21, 2021

github-actions bot commented Feb 21, 2021

github-actions bot commented Feb 21, 2021

dlorenc left a comment

dlorenc Feb 22, 2021

dlorenc Feb 22, 2021

inferno-chromium left a comment

inferno-chromium Feb 22, 2021

inferno-chromium Feb 22, 2021

inferno-chromium Feb 22, 2021

naveensrinivasan Feb 22, 2021

inferno-chromium Feb 22, 2021

github-actions bot commented Feb 22, 2021

@@ @@ -23,3 +23,6 @@ results.json @@
               # tools
               bin
+              #temp


		To use disk cache two env variables have to be set `USE_DISK_CACHE=true` and `DISK_CACHE_PATH=./cache`.

		There are not TTL on cache.

Feat-Implement httpcache middleware for GitHub API #203

Feat-Implement httpcache middleware for GitHub API #203

Conversation

naveensrinivasan commented Feb 21, 2021

Folder size

Files in the folder

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2021

github-actions bot commented Feb 21, 2021

github-actions bot commented Feb 21, 2021

github-actions bot commented Feb 21, 2021

github-actions bot commented Feb 21, 2021

dlorenc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inferno-chromium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 22, 2021