Added script to generate stats on a git checkout

Added script to generate stats on a git checkout

BUG=510666
Committed: https://chromium.googlesource.com/infra/infra/+/ef9c50dcb39bb62651b5a6cef37c0ee4ddb5ce5f

[keelerh, 2015-07-16 02:38:15]

keelerh@google.com changed reviewers:
+ hinoka@chromium.org, pgervais@chromium.org, sergeyberezin@chromium.org

[Sergey Berezin, 2015-07-16 15:23:55]

Mostly nits, otherwise the code looks fine to me.

Also, the description says "script", but you're adding a module, I'm guessing,
to be used by something else later?

https://codereview.chromium.org/1235373004/diff/1/infra/tools/antibody/comput...
File infra/tools/antibody/compute_stats.py (right):

https://codereview.chromium.org/1235373004/diff/1/infra/tools/antibody/comput...
infra/tools/antibody/compute_stats.py:33: e.g. [['1-2015', .251], ['10-2014',
.742]]
nit: in most other systems, we usually use YYYY-MM format - fixed width, can be
meaningfully sorted. Any reason not to use it here?

https://codereview.chromium.org/1235373004/diff/1/infra/tools/antibody/comput...
infra/tools/antibody/compute_stats.py:39: ratios.append([i[0], round(float(i[1])
/ j[1], 3)])
nit: beware of floating point representation being binary: some decimal
fractions cannot be exactly represented by IEEE floating point, resulting in
numbers like 1.243999999.

https://codereview.chromium.org/1235373004/diff/1/infra/tools/antibody/comput...
infra/tools/antibody/compute_stats.py:185: return all_commits_by_date,
total_num_commits, suspicious_commits_by_date,
nit: consider returning a map rather than a tuple, so the individual results
have explicit names. This way, no guessing is involved in what index the needed
data is, and adding new piece of data doesn't mess up indices.

[hinoka, 2015-07-17 22:57:46]

lgtm with some comments

Can you also indent+format your SQL queries. http://sqlformat.org/ is a good
start.

https://codereview.chromium.org/1235373004/diff/1/infra/tools/antibody/comput...
File infra/tools/antibody/compute_stats.py (right):

https://codereview.chromium.org/1235373004/diff/1/infra/tools/antibody/comput...
infra/tools/antibody/compute_stats.py:33: e.g. [['1-2015', .251], ['10-2014',
.742]]
On 2015/07/16 15:23:55, Sergey Berezin wrote:
> nit: in most other systems, we usually use YYYY-MM format - fixed width, can
be
> meaningfully sorted. Any reason not to use it here?

+1

https://codereview.chromium.org/1235373004/diff/1/infra/tools/antibody/comput...
infra/tools/antibody/compute_stats.py:154: cc.execute("""INSERT INTO
addr_tbr_hash (email, tbr, hash)
Lets format this with indents, so it looks like
INSERT INTO ....
SELECT ...
    ...
LEFT JOIN (
  SELECT ...
    WHERE ....
  tbr_count...
ON ....
where ...

Or indent it in a way that makes sense, i'm not entirely sure if the above
example is the right indentation level.

Also a good resource: http://sqlformat.org/ (first result of a "sql indentation"
search).  seems to do an okay job

https://codereview.chromium.org/1235373004/diff/1/infra/tools/antibody/comput...
infra/tools/antibody/compute_stats.py:161: cc.execute("""SELECT email, SUM(CASE
WHEN tbr<>0 THEN 1 ELSE 0 END)
I assume you created a new table to do aggregation.  Why not just run the query
and do the calculation in python?  Creating a remote table, fetching the result,
and then deleting the table seems like a lot of RPC overhead.

[pgervais, 2015-07-21 16:48:34]

A bunch of optional comments. Feel free to fix them in another CL. Otherwise
lgtm.

https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
File infra/tools/antibody/compute_stats.py (right):

https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
infra/tools/antibody/compute_stats.py:54: return results
(not for this CL)
This can be done in SQL only, and would also be more efficient. Something like
this would work I think:

SELECT COUNT(*), YEAR(git_commit.timestamp), MONTH(git_commit.timestamp) 
FROM git_commit
GROUP BY YEAR(git_commit.timestamp), MONTH(git_commit.timestamp)

http://stackoverflow.com/questions/508791/mysql-query-group-by-day-month-year

https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
infra/tools/antibody/compute_stats.py:158: return results
See my comment on total_commits above. You may be able to replace that by a
single SQL query.

https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
infra/tools/antibody/compute_stats.py:224: ratio = round(float(numerator) /
denominator, 3)
Take into account denominator == 0, as you've done in ratio_calculator(). You
may also want to move this function next to ratio_calculator().

And we need tests :-)

https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
infra/tools/antibody/compute_stats.py:233: 
Describe sql_time_specification (same below)

https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
infra/tools/antibody/compute_stats.py:374: cc.execute("""CREATE TEMPORARY TABLE
addr_tbr_hash (email VARCHAR(200), tbr
If the insert statement fails, the table will remain. Adding a 'finally' clause
that calls 'drop table' would prevent this kind of issues.

https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
infra/tools/antibody/compute_stats.py:469: d['timeframe'] = '7_days'
Do that on line 466:
stats_7_days = {'timeframe': '7_days'}

https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
infra/tools/antibody/compute_stats.py:470: sql_insert = 'DATEDIFF(NOW(),
git_commit.timestamp) <= 7'
Just set the value instead of writing a where clause, something like: timeframe
= 7. You can use timeframe = None for 'all times'. Then pass this value directly
to totaled_total_commits and other functions, which will compute the where
clause. If use_timeframe == None, then do not add any clause.

[keelerh, 2015-07-22 16:29:47]

On 2015/07/21 at 16:48:34, pgervais wrote:
> A bunch of optional comments. Feel free to fix them in another CL. Otherwise
lgtm.
> 
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> File infra/tools/antibody/compute_stats.py (right):
> 
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> infra/tools/antibody/compute_stats.py:54: return results
> (not for this CL)
> This can be done in SQL only, and would also be more efficient. Something like
this would work I think:
> 
> SELECT COUNT(*), YEAR(git_commit.timestamp), MONTH(git_commit.timestamp) 
> FROM git_commit
> GROUP BY YEAR(git_commit.timestamp), MONTH(git_commit.timestamp)
> 
> http://stackoverflow.com/questions/508791/mysql-query-group-by-day-month-year
> 
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> infra/tools/antibody/compute_stats.py:158: return results
> See my comment on total_commits above. You may be able to replace that by a
single SQL query.
> 
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> infra/tools/antibody/compute_stats.py:224: ratio = round(float(numerator) /
denominator, 3)
> Take into account denominator == 0, as you've done in ratio_calculator(). You
may also want to move this function next to ratio_calculator().
> 
> And we need tests :-)
> 
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> infra/tools/antibody/compute_stats.py:233: 
> Describe sql_time_specification (same below)
> 
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> infra/tools/antibody/compute_stats.py:374: cc.execute("""CREATE TEMPORARY
TABLE addr_tbr_hash (email VARCHAR(200), tbr
> If the insert statement fails, the table will remain. Adding a 'finally'
clause that calls 'drop table' would prevent this kind of issues.
> 
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> infra/tools/antibody/compute_stats.py:469: d['timeframe'] = '7_days'
> Do that on line 466:
> stats_7_days = {'timeframe': '7_days'}
> 
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> infra/tools/antibody/compute_stats.py:470: sql_insert = 'DATEDIFF(NOW(),
git_commit.timestamp) <= 7'
> Just set the value instead of writing a where clause, something like:
timeframe = 7. You can use timeframe = None for 'all times'. Then pass this
value directly to totaled_total_commits and other functions, which will compute
the where clause. If use_timeframe == None, then do not add any clause.


Which functions should I write tests for? Do we need to make four fake sqlite
databases and try to call all the functions? Or just test things like the ratio
calculator?

[pgervais, 2015-07-22 17:57:47]

On 2015/07/22 16:29:47, keelerh wrote:
> On 2015/07/21 at 16:48:34, pgervais wrote:
> > A bunch of optional comments. Feel free to fix them in another CL. Otherwise
> lgtm.
> > 
> >
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> > File infra/tools/antibody/compute_stats.py (right):
> > 
> >
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> > infra/tools/antibody/compute_stats.py:54: return results
> > (not for this CL)
> > This can be done in SQL only, and would also be more efficient. Something
like
> this would work I think:
> > 
> > SELECT COUNT(*), YEAR(git_commit.timestamp), MONTH(git_commit.timestamp) 
> > FROM git_commit
> > GROUP BY YEAR(git_commit.timestamp), MONTH(git_commit.timestamp)
> > 
> >
http://stackoverflow.com/questions/508791/mysql-query-group-by-day-month-year
> > 
> >
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> > infra/tools/antibody/compute_stats.py:158: return results
> > See my comment on total_commits above. You may be able to replace that by a
> single SQL query.
> > 
> >
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> > infra/tools/antibody/compute_stats.py:224: ratio = round(float(numerator) /
> denominator, 3)
> > Take into account denominator == 0, as you've done in ratio_calculator().
You
> may also want to move this function next to ratio_calculator().
> > 
> > And we need tests :-)
> > 
> >
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> > infra/tools/antibody/compute_stats.py:233: 
> > Describe sql_time_specification (same below)
> > 
> >
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> > infra/tools/antibody/compute_stats.py:374: cc.execute("""CREATE TEMPORARY
> TABLE addr_tbr_hash (email VARCHAR(200), tbr
> > If the insert statement fails, the table will remain. Adding a 'finally'
> clause that calls 'drop table' would prevent this kind of issues.
> > 
> >
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> > infra/tools/antibody/compute_stats.py:469: d['timeframe'] = '7_days'
> > Do that on line 466:
> > stats_7_days = {'timeframe': '7_days'}
> > 
> >
>
https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
> > infra/tools/antibody/compute_stats.py:470: sql_insert = 'DATEDIFF(NOW(),
> git_commit.timestamp) <= 7'
> > Just set the value instead of writing a where clause, something like:
> timeframe = 7. You can use timeframe = None for 'all times'. Then pass this
> value directly to totaled_total_commits and other functions, which will
compute
> the where clause. If use_timeframe == None, then do not add any clause.
> 
> 
> Which functions should I write tests for? Do we need to make four fake sqlite
> databases and try to call all the functions? Or just test things like the
ratio
> calculator

Since we don't have a good story for testing SQL queries, and it would be good
to have a prototype working soon, just write tests for everything that does not
contain SQl (which should be 2-3 function in this file).

[keelerh, 2015-07-22 21:17:48]

The CQ bit was checked by keelerh@google.com

[keelerh, 2015-07-22 21:17:48]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@chromium.org,
pgervais@chromium.org
Link to the patchset: https://codereview.chromium.org/1235373004/#ps40001
(title: "Made changes so pipeline runs through")

[keelerh, 2015-07-22 21:17:55]

https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
File infra/tools/antibody/compute_stats.py (right):

https://codereview.chromium.org/1235373004/diff/20001/infra/tools/antibody/co...
infra/tools/antibody/compute_stats.py:470: sql_insert = 'DATEDIFF(NOW(),
git_commit.timestamp) <= 7'
On 2015/07/21 at 16:48:34, pgervais wrote:
> Just set the value instead of writing a where clause, something like:
timeframe = 7. You can use timeframe = None for 'all times'. Then pass this
value directly to totaled_total_commits and other functions, which will compute
the where clause. If use_timeframe == None, then do not add any clause.

I was initially going to do that, but then didn't know what to do for all_time,
because if I had written the where clause and then timeframe was None and I
didn't want to include the clause, I would have to recopy basically the entire
sql query with the where clause removed, which seemed like a ton of extra space.
Is there a value I can put there where it will include all rows?

[commit-bot: I haz the power, 2015-07-22 21:17:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1235373004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1235373004/40001

[keelerh, 2015-07-22 22:11:34]

The CQ bit was unchecked by keelerh@google.com

[keelerh, 2015-07-22 22:29:16]

The CQ bit was checked by keelerh@google.com

[keelerh, 2015-07-22 22:29:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
pgervais@chromium.org, hinoka@chromium.org
Link to the patchset: https://codereview.chromium.org/1235373004/#ps60001
(title: "Fixed small error in git log call")

[commit-bot: I haz the power, 2015-07-22 22:29:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1235373004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1235373004/60001

[keelerh, 2015-07-22 22:33:24]

The CQ bit was unchecked by keelerh@google.com

[keelerh, 2015-07-22 22:56:23]

The CQ bit was checked by keelerh@google.com

[keelerh, 2015-07-22 22:56:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
pgervais@chromium.org, hinoka@chromium.org
Link to the patchset: https://codereview.chromium.org/1235373004/#ps80001
(title: "fixed tbr by user")

[commit-bot: I haz the power, 2015-07-22 22:56:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1235373004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1235373004/80001

[commit-bot: I haz the power, 2015-07-22 23:24:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-22 23:24:54]

Try jobs failed on following builders:
  infra_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_presubmit...)
  infra_tester on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)

[keelerh, 2015-07-23 00:54:29]

The CQ bit was checked by keelerh@google.com

[keelerh, 2015-07-23 00:54:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
pgervais@chromium.org, hinoka@chromium.org
Link to the patchset: https://codereview.chromium.org/1235373004/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2015-07-23 00:54:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1235373004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1235373004/100001

[commit-bot: I haz the power, 2015-07-23 00:56:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-23 00:56:45]

Try jobs failed on following builders:
  infra_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_presubmit...)
  infra_tester on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)

[keelerh, 2015-07-23 00:58:43]

The CQ bit was checked by keelerh@google.com

[keelerh, 2015-07-23 00:58:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
pgervais@chromium.org, hinoka@chromium.org
Link to the patchset: https://codereview.chromium.org/1235373004/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-07-23 00:58:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1235373004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1235373004/120001

[commit-bot: I haz the power, 2015-07-23 01:00:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-23 01:01:00]

Try jobs failed on following builders:
  infra_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_presubmit...)

[keelerh, 2015-07-23 01:08:05]

The CQ bit was checked by keelerh@google.com

[keelerh, 2015-07-23 01:08:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
pgervais@chromium.org, hinoka@chromium.org
Link to the patchset: https://codereview.chromium.org/1235373004/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-07-23 01:08:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1235373004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1235373004/140001

[commit-bot: I haz the power, 2015-07-23 01:10:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-23 01:10:27]

Try jobs failed on following builders:
  infra_tester on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)

[keelerh, 2015-07-23 01:21:33]

The CQ bit was checked by keelerh@google.com

[keelerh, 2015-07-23 01:21:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
pgervais@chromium.org, hinoka@chromium.org
Link to the patchset: https://codereview.chromium.org/1235373004/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2015-07-23 01:21:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1235373004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1235373004/160001

[keelerh, 2015-07-23 01:23:34]

The CQ bit was unchecked by keelerh@google.com

[keelerh, 2015-07-23 01:24:07]

The CQ bit was checked by keelerh@google.com

[commit-bot: I haz the power, 2015-07-23 01:24:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1235373004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1235373004/160001

[commit-bot: I haz the power, 2015-07-23 01:26:45]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/infra/infra/+/ef9c50dcb39bb62651b5a6cef37c0...