Cleanup GitHub Stats #2400

mikeyoung85 · 2019-07-09T18:24:36Z

I wanted to get the Github stats fixed up a bit.

In this PR:

moved query and stat classes into Github module
removed some unused or duplicate stats
used repository_host download_issues and download_pull_requests and use the database to gather those stats
redid specs with a smaller repository to cut down on amount of data in VCR cassettes

…tory

app/models/repository_host/github.rb

app/models/maintenance_stats/queries/github/commit_count_query.rb

app/models/maintenance_stats/queries/github/releases_query.rb

kbarrette · 2019-07-10T17:20:19Z

app/models/maintenance_stats/queries/github/repository_contributor_stats_query.rb

+
+          resp = @client.contributor_stats(params[:full_name])
+          if @client.last_response.status == 202
+            sleep 1


4 seconds of total possible sleep time sounds like a lot, although I'm not sure what this is waiting on?

This request to GitHub will often return a 202 instead of the actual data to let GitHub process the information needed for the request. This is there to retry the call after a short wait, not ideal. It would be better if this was handled in the Octokit client.

Maybe use a simple increasing backoff?

app/models/maintenance_stats/stats/github/db_issues_stats.rb

app/models/repository_host/github.rb

kbarrette · 2019-07-10T17:24:51Z

spec/models/maintenance_stats/github/release_stats_spec.rb

    end
  end

  context "repository with no commits" do
    let(:repository) { create(:repository, full_name: 'buddhamagnet/heidigoodchild') }
    let(:query_results) do
-        VCR.use_cassette('github/empty_repository', :match_requests_on => [:body]) do
+        VCR.use_cassette('github/empty_repository', :match_requests_on => [:method, :uri, :body, :query]) do


Is there anything special that needs to be done to re-record these cassettes? If so, now might be good time to document it for posterity.

Where would be a good spot for information like that?

In the past I've just plopped it in comments near the VCR calls

…s instead of class variables

…w to handle VCR changes for maintenance stat queries

kbarrette

Looking good, but I had a few questions about RepoReleasesQuery

kbarrette · 2019-07-11T14:07:16Z

app/models/maintenance_stats/queries/github/releases_query.rb

+
+            result.data.repository&.releases.nodes.each do |release|
+              publish_date = DateTime.parse(release.published_at)
+              if publish_date > end_date


Is this backwards? I feel like it should be publish_date < end_date

Also, should it be <= so end_date is inclusive?

I'll rename these variables. The releases are going from newest to oldest and end_date is the earliest date we would care about. Like get all releases from the last year, end_date would be one year ago. So if the current release's date is after end_date then add it to the list and keep going to the next release.

Ooh, makes sense. The name was confusing, for sure.

app/models/maintenance_stats/queries/github/releases_query.rb

…_date and handle errors within the query class as well as just send back release data instead of the entire graphql result

mikeyoung85 · 2019-07-12T12:56:11Z

app/models/maintenance_stats/queries/github/releases_query.rb

+          :v4
+        end
+
+        def query(params: {})


Made changes through here to make this a little clearer and to actually gather and return release data correctly. Now it actually takes into account the publish date and exits the loop correctly. It also is just sending back the release data now instead of the entire graphQL response and I've updated the stat to take that into account.

mikeyoung85 · 2019-07-12T12:57:30Z

app/models/maintenance_stats/queries/github/repository_contributor_stats_query.rb

+
+          resp = @client.contributor_stats(params[:full_name])
+          if @client.last_response.status == 202
+            sleep (count + 1) * 0.5


Updated this to be a quicker second try and a small backoff after that.

Thoughts on ditching the + 1 to retry after 500ms the first time?

count would be 0 the first time through that's why I added the +1. I can redo count to start at 1 to remove this.

Whoops, that's fine!

mikeyoung85 · 2019-07-12T12:57:52Z

app/models/maintenance_stats/stats/github/contributor_count_stats.rb

-              0
-            end
-          end.sum
+          @results.select{ |contributor| contributed?(contributor, weeks_ago) }.size


Re-arranged this section to try and be a little clearer.

mikeyoung85 · 2019-07-12T12:58:38Z

app/models/maintenance_stats/stats/github/contributor_count_stats.rb

+        end
+
+        def contributed?(contributor, weeks_ago)
+          # todo: verify weeks are actually within the time period?


Right now it is assuming the last week in the array is the latest week. I might go back and rewrite this to actually check the timestamps of the week instead.

mikeyoung85 · 2019-07-12T12:59:41Z

app/models/maintenance_stats/stats/github/release_stats.rb

-          {
+          return {} if @results.nil?
+
+          last_week_releases = @results.select {|release| DateTime.parse(release.published_at) > @now - 1.week}.count


This has been rewritten to use just to read an array of releases instead of the entire graphQL result set.

mikeyoung85 · 2019-07-12T13:00:58Z

app/models/maintenance_stats/stats/github/release_stats.rb

-          last_two_month_releases = @results.data.repository.releases.nodes.select {|release| release.published_at > @now - 60}.count
-          last_year_releases = @results.data.repository.releases.nodes.select {|release| release.published_at > @now - 365}.count
-          {
+          return {} if @results.nil?


This is a slight change. Before we were sending back 0 releases, but our original stance was to return nothing for a stat if we didn't have the data for it. I think this is more correct.

mikeyoung85 · 2019-07-12T13:01:28Z

app/models/maintenance_stats/stats/github/release_stats.rb

@@ -24,7 +26,7 @@ def get_stats
        private

        def last_release_date
-          @results.data.repository.releases.nodes.first&.published_at 
+          @results&.map { |node| DateTime.parse(node.published_at) }.first&.strftime('%FT%TZ')


The strftime is to keep the publish date in the same format as what it was previously.

.iso8601 is there if you'd rather use that than strftime

iso8601 was returning something slightly different than what GitHub is sending back. I'm trying to keep the formatting the same as the stats that are already in the database.

kbarrette

Couple more comments, but nothing blocking - this is looking nice.

app/models/maintenance_stats/queries/github/releases_query.rb

kbarrette · 2019-07-12T19:08:17Z

app/models/maintenance_stats/queries/github/releases_query.rb

+            cursor = result.data.repository.releases.page_info.end_cursor
+
+            # initalize releases if we have not started gathering data
+            releases ||= []


On line 38 releases = [] would make this unnecessary, I think? Unless it's important to return nil?

I'm using nil to indicate no data was returned from the API. At this line we have a response from GitHub so I want to initialize to an empty array unless this is after the first page of information.

kbarrette · 2019-07-12T19:10:25Z

app/models/maintenance_stats/queries/github/repository_contributor_stats_query.rb

+
+          resp = @client.contributor_stats(params[:full_name])
+          if @client.last_response.status == 202
+            sleep (count + 1) * 0.5


Thoughts on ditching the + 1 to retry after 500ms the first time?

app/models/maintenance_stats/stats/github/contributor_count_stats.rb

kbarrette · 2019-07-12T19:13:35Z

app/models/maintenance_stats/stats/github/release_stats.rb

@@ -24,7 +26,7 @@ def get_stats
        private

        def last_release_date
-          @results.data.repository.releases.nodes.first&.published_at 
+          @results&.map { |node| DateTime.parse(node.published_at) }.first&.strftime('%FT%TZ')


.iso8601 is there if you'd rather use that than strftime

mikeyoung85 added 5 commits July 3, 2019 09:46

move query classes

9c9f29c

update query modules and fix indentation

c5e41ca

move spec files

9c1205a

updated to use new module and removed unused stats from github reposi…

d8700cd

…tory

redo github stats and specs

685cfff

mikeyoung85 requested review from katzj, havocp and kbarrette July 9, 2019 18:24

mikeyoung85 commented Jul 9, 2019

View reviewed changes

app/models/repository_host/github.rb Show resolved Hide resolved

mikeyoung85 commented Jul 9, 2019

View reviewed changes

app/models/repository_host/github.rb Show resolved Hide resolved

remove pry

3d0c887

kbarrette suggested changes Jul 10, 2019

View reviewed changes

mikeyoung85 added 2 commits July 10, 2019 14:17

update query names to be all caps, change query params to be constant…

e263972

…s instead of class variables

add better backoff timing for contributor query and added notes on ho…

fea6391

…w to handle VCR changes for maintenance stat queries

mikeyoung85 force-pushed the mikeyoung85/cleanup-github-stats branch from dd0e863 to fea6391 Compare July 10, 2019 20:20

updated spec to actually use chalk instead of rails for api calls

ead7988

mikeyoung85 requested a review from kbarrette July 10, 2019 20:44

kbarrette suggested changes Jul 11, 2019

View reviewed changes

mikeyoung85 added 3 commits July 11, 2019 15:01

redid release query to actually stop querying for releases beyond end…

a4841fa

…_date and handle errors within the query class as well as just send back release data instead of the entire graphql result

remove pry

117bb6d

clean up contributor stat method a bit

f8a4998

mikeyoung85 commented Jul 12, 2019

View reviewed changes

change to use %i instead of calling .to_sym on literals

bb66f16

kbarrette reviewed Jul 12, 2019

View reviewed changes

mikeyoung85 merged commit f2d09ba into master Jul 16, 2019

mikeyoung85 deleted the mikeyoung85/cleanup-github-stats branch July 16, 2019 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup GitHub Stats #2400

Cleanup GitHub Stats #2400

mikeyoung85 commented Jul 9, 2019

kbarrette Jul 10, 2019

mikeyoung85 Jul 10, 2019

kbarrette Jul 10, 2019

kbarrette Jul 10, 2019

mikeyoung85 Jul 10, 2019

kbarrette Jul 10, 2019

kbarrette left a comment

kbarrette Jul 11, 2019

mikeyoung85 Jul 11, 2019

kbarrette Jul 11, 2019

mikeyoung85 Jul 12, 2019

mikeyoung85 Jul 12, 2019

kbarrette Jul 12, 2019

mikeyoung85 Jul 12, 2019

kbarrette Jul 12, 2019

mikeyoung85 Jul 12, 2019

mikeyoung85 Jul 12, 2019

mikeyoung85 Jul 12, 2019

mikeyoung85 Jul 12, 2019

mikeyoung85 Jul 12, 2019

kbarrette Jul 12, 2019

mikeyoung85 Jul 12, 2019

kbarrette left a comment

kbarrette Jul 12, 2019

mikeyoung85 Jul 12, 2019

kbarrette Jul 12, 2019

kbarrette Jul 12, 2019

Cleanup GitHub Stats #2400

Cleanup GitHub Stats #2400

Conversation

mikeyoung85 commented Jul 9, 2019

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

kbarrette 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

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

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

kbarrette 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