Implementation to aggregate statistical information #12 #128

nalabjp · 2017-07-30T16:46:07Z

#12

統計情報の集計のために以下の機能を実装しました。

Features

event_historiesテーブルの作成
dojo_event_servicesテーブルの作成
APIクライアントの実装
- Connpass
- Doorkeeper
API経由のイベント開催実績をevent_historiesテーブルに流し込む実装
- Runner
- Rake Task
group_idを調べるためのRake Task

運用イメージ

前提

dojo毎のdojo_event_serviceレコードを作成する
- connpassのseries.id / doorkeeperのgroupを調べる必要がある
- rake statistics:search[keyword]でキーワード検索 or なんとかして調べる

月次バッチ

cronもしくはスケジューラ等で毎月月初に前月のバッチ処理を行う
- rake statistics:aggregation[yyyymm]を叩く
- yyyymmは省略可能（省略すると前月分を実行）

過去分

年月を指定して過去分の月次バッチ処理を行う
- rake statistics:aggregation[yyyymm]を叩く
- yyyymmに集計したい年月を指定する

…able

yasulab · 2017-07-31T01:27:07Z

うぉぉぉぉ、PR ありがとうございます...!! 🙏 ✨ まだ WIP とのことですが、何か気づいたことがあれば随時共有して行きますね! ;)

nalabjp · 2017-08-13T19:07:42Z

一通り実装し終えたのでレビューをお願いします。

なお、CIが回せていないのは SCRIVITO_TENANT や SCRIVITO_API_KEYあたりが必要なためです。
https://docs.travis-ci.com/user/environment-variables/#Defining-encrypted-variables-in-.travis.yml
ここに載っている通りセキュアな文字列にしてしまえば良さそうなんですが、
やってもよいか念のため確認してからにしようと思いコミットしていません。
問題なければセキュアな文字列にしてコミットするのでご確認をお願いします😌

yasulab · 2017-08-14T00:43:53Z

ありがとうございます! 後ほど拝見します! 👀 💨

hanachin · 2017-08-14T02:23:01Z

.travis.yml

@@ -12,4 +12,10 @@ deploy:
  run:
      - "bundle exec rails db:migrate"
      - "bundle exec rails dojos:update_db_by_yaml"
-
+rvm:
+  - 2.4.0


travisのログみた感じだと.ruby-versionがあればここ指定する必要はないみたいです。

$ rvm use $(< .ruby-version) --install --binary --fuzzy

https://travis-ci.org/coderdojo-japan/coderdojo.jp/jobs/260468747

なるほど。
fixed 563bd68

複数バージョンのRubyに対してテスト実行したいときは便利そうですが今は使ってるRuby1つしかないので消しちゃって大丈夫そうですね 👍

hanachin · 2017-08-14T02:24:09Z

.travis.yml

+cache:
+  - bundler
+script:
+  - RAILS_ENV=test bundle exec rake db:migrate --trace


travisだとデフォルトでRAILS_ENV=testなのでここで環境変数指定する必要はなさそうです
https://docs.travis-ci.com/user/environment-variables/#Default-Environment-Variables

😅
fixed f44f78e

hanachin · 2017-08-14T02:28:38Z

lib/statistics/client.rb

+
+    private
+
+    def connection_for(endpoint, &block)


&blockつかわれてなさそう

こちらで使っています〜
db0afac#diff-eaf50acb3e83c4f729603033a19f20c4R54

blockの変数自体は使ってなさそうに見えました。

あ、確かに。
失礼しました。

fixed 754cf52

hanachin · 2017-08-14T02:52:56Z

lib/statistics/client.rb

+        @client = Client.new(ENDPOINT)
+      end
+
+      def fetch_series_id(**params)


ハッシュ受け取るだけなら単にparamsでよさそうな気も

インタフェースが変わって消えてしまいました〜

hanachin · 2017-08-14T02:59:32Z

lib/statistics/client.rb

+      def fetch_series_id(**params)
+        @client.get('event/', params.merge(count: 1))
+          .fetch('events')
+          .first


配列の1番目取るなら.dig(0, 'series', 'id')でよさそう

配列の要素がないときエラーにしたい感じなのかな?

[4] pry(main)> [{foo: 1}].dig(0, :foo) => 1 [5] pry(main)> [].dig(0, :foo) => nil [6] pry(main)> [].first.dig(:foo) NoMethodError: undefined method `dig' for nil:NilClass from (pry):6:in `__pry__'

おぉ、indexも引数に渡せるからそういう使い方できるんですね。💡
勉強になりました。

hanachin · 2017-08-14T03:27:30Z

config/application.rb

+
+    # Timezone
+    config.time_zone = 'Asia/Tokyo'
+    ENV['TZ'] = 'Asia/Tokyo'


Heroku、travis-ciのほうで設定した方がいいかも?

Herokuのenvはどなたかにお願いするとして、そうしましょうか。

Heroku の env 確認しましたが、既に入ってました ;) ✅

$ heroku config ... TZ: Asia/Tokyo

ご確認ありがとうございます！😁

fixed 0366800

hanachin · 2017-08-14T03:28:25Z

config/application.rb

@@ -11,5 +11,9 @@ class Application < Rails::Application
    # Settings in config/environments/* take precedence over those specified here.
    # Application configuration should go into files in config/initializers
    # -- all .rb files in that directory are automatically loaded.
+
+    # Timezone
+    config.time_zone = 'Asia/Tokyo'


hanachin · 2017-08-14T03:32:47Z

lib/statistics/client.rb

+
+          break if part['results_returned'].zero?
+
+          events.push(*part.fetch('events'))


こことL99、count: 100なので大丈夫そうですがevents個数がものすごく多いとスタック溢れそうですね

そうですねー。
一応connpassはcount: 100がAPIの上限値なのでまぁ良いかなぁと判断しました。
doorkeeperは25固定です。

hanachin · 2017-08-14T03:43:29Z

lib/tasks/statistics.rake

@@ -15,6 +15,9 @@ namespace :statistics do

    raise ArgumentError, "Invalid format: #{args[:yyyymm]}" if date.nil?

+
+    EventHistory.where(evented_at: date.beginning_of_month..date.end_of_month).delete_all


その月の分まるごと作り直す感じなんですねー。

あまり無いとは思いますがたまにやり直したいケースがあったりするのでそのようにしました。

いいと思います!

hanachin · 2017-08-14T03:44:20Z

lib/statistics/aggregation.rb

+  class Aggregation
+    class << self
+      def run(date:)
+        cnps_dojos = Dojo.joins(:dojo_event_service).where(dojo_event_services: { name: 'connpass' }).to_a


ここでnameに渡している文字列、マジックナンバーになっていますね。このカラムをEnumか何かにしたいですね

ご指摘のとおりですね。

fixed d392c68

yasulab · 2017-08-15T01:40:39Z

なお、CIが回せていないのは SCRIVITO_TENANT や SCRIVITO_API_KEYあたりが必要なためです。
https://docs.travis-ci.com/user/environment-variables/#Defining-encrypted-variables-in-.travis.yml
ここに載っている通りセキュアな文字列にしてしまえば良さそうなんですが、
やってもよいか念のため確認してからにしようと思いコミットしていません。
問題なければセキュアな文字列にしてコミットするのでご確認をお願いします😌

@nalabjp Travis の encrypt ですね! はい、そちらもコミットしてもらえると助かります 😆

別件ですが、以前 coderdojo-japan/dojopaas を実装している時に、Travis CI のログにトークンが流れるというミスをしてしまったことがあります 😅

cf. クライアントのdebugがtrueだとアクセストークンが出力されてしまう
sakura-internet/node-sacloud#9

今回のケースでは特に問題にならないと思いますが、念のためチェックしておくと良さそうですね ;)

yasulab · 2017-08-15T03:13:20Z

encrypt したトークンを追加したり、指摘した点の対応が終わったら、CIが通ってるのを確認してマージみたいな流れかなという認識です! 🙆 何か他に懸念点などあれば、随時共有する形で進められたらと考えています😆

In heroku, it is already defined.

…m `string`

nalabjp · 2017-08-20T10:02:39Z

travisのencrypted env vars、PRからだと読み込まれなくなっているようですね。
以下のログが出ていました。

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions

https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
一部を引用すると

Pull requests sent from forked repositories do not have access to encrypted variables or data.

ということらしいです。
これはもしかして回避する方法がないってことなんでしょうかね？🤔

（ところで、travisコマンドでymlにsecretを追加しなくても、既にsettingsに環境変数として設定されていたんですね、見ていませんでした😅）

nalabjp · 2017-08-20T10:05:29Z

travisの件以外はレビューのフィードバックに対応したつもりです 💪 😄

hanachin

修正確認しました〜! よさそうです。
travisの件、回避する方法はなさそうですね。

yasulab · 2017-08-21T02:27:23Z

Pull requests sent from forked repositories do not have access to encrypted variables or data.

なるほど 🤔 そういう理由だったんですね 😅 ナラさんにコミット権は既に付与してたと思うので、branch 間の PR でも大丈夫そうです! 😸

yasulab · 2017-08-21T02:28:04Z

僕の方でもあとで確認して、問題なければマージしようと思います! 📑 👀

yasulab · 2017-08-21T04:03:19Z

確認しました! こちらのPRは一旦マージした後に集計を行い、その結果 (数字) をトップページに出すところまで持っていきたいですね 📈 ✨ (デザインや見せ方はさておき、数字をトップページに置くだけでもインパクトはあると思うので...!!)

nalabjp · 2017-08-21T04:58:23Z

レビューありがとうございました〜😌

nalabjp added 9 commits July 30, 2017 05:12

Gem faraday

e3fd895

Draft implementaion of API client

db0afac

Create a table named event_histories

04f645c

Create a model named EventHistory

6ff7f21

Create a table named dojo_event_services

453d28f

Create a model named DojoEventService

3669f2a

Add associations

b3d39bc

Add timezone as Asia/Tokyo

f3580de

Draft implementation of importing data via API to event_histories t…

3bc50c1

…able

nalabjp added 12 commits August 7, 2017 00:06

Fix

dfdc16c

Support multiple requests

c93dfb8

Support a period for search conditions

9eb61aa

Support idempotency in re-running with same conditions

a4cb3ec

Rename a class name

d75d2fe

Use instance variables instead of constants

8ba4506

Fix typo

b1a5ad1

Create a task for the monthly aggregation

7197558

Want to see some search results

983d828

Rake task for searching

cf033f8

Fix typo

70ef722

Migrate database on Travis CI

5168879

nalabjp force-pushed the statistics branch from 3f5fe62 to fd408e6 Compare August 13, 2017 12:50

nalabjp added 3 commits August 13, 2017 21:59

Run RSpec on Travis CI

ba6fa8f

Ruby 2.4.0 on Travis CI

59fe612

Cache bundler on Travis CI

ec719e1

nalabjp force-pushed the statistics branch 2 times, most recently from 7394a32 to ec719e1 Compare August 13, 2017 13:17

nalabjp added 2 commits August 14, 2017 02:57

Shared context for API stubs

0024192

Remove nested Hash key

b7e8213

Remove records of the target period before executing the aggregation

740ca73

nalabjp changed the title ~~[WIP] Implementation to aggregate statistical information #12~~ Implementation to aggregate statistical information #12 Aug 13, 2017

Add description for statistics:search

86b1f6c

hanachin reviewed Aug 14, 2017

View reviewed changes

nalabjp added 3 commits August 15, 2017 01:41

Remove needless configuration

563bd68

Remove needless configuration

f44f78e

Fix association name

0d72302

Remove needless argument

754cf52

nalabjp force-pushed the statistics branch from ab8e6cb to 404273b Compare August 20, 2017 08:35

Move to .travis.yml

0366800

In heroku, it is already defined.

nalabjp force-pushed the statistics branch from 404273b to 0366800 Compare August 20, 2017 08:47

nalabjp added 2 commits August 20, 2017 17:54

Add NOT NULL restriction

9adb714

Change the column type of dojo_event_services.name to integer fro…

d392c68

…m `string`

hanachin approved these changes Aug 21, 2017

View reviewed changes

yasulab merged commit f3a83d3 into coderdojo-japan:master Aug 21, 2017

nalabjp deleted the statistics branch August 21, 2017 04:57

This was referenced Sep 26, 2017

[WIP] Dojo event services data #139

Closed

Upserting dojo_event_services table #140

Merged

nalabjp mentioned this pull request Oct 26, 2017

一部の PR では Scrivito に関するテストが落ちる問題を解決したい #160

Closed

yasulab added the 統計情報 Tracking event record function via APIs: https://coderdojo.jp/stats label Jan 18, 2018


		break if part['results_returned'].zero?

		events.push(*part.fetch('events'))

		@@ -15,6 +15,9 @@ namespace :statistics do

		raise ArgumentError, "Invalid format: #{args[:yyyymm]}" if date.nil?


		EventHistory.where(evented_at: date.beginning_of_month..date.end_of_month).delete_all

Implementation to aggregate statistical information #12 #128

Implementation to aggregate statistical information #12 #128

Conversation

nalabjp commented Jul 30, 2017 • edited

Features

運用イメージ

前提

月次バッチ

過去分

yasulab commented Jul 31, 2017

nalabjp commented Aug 13, 2017

yasulab commented Aug 14, 2017

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

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

yasulab commented Aug 15, 2017 • edited

yasulab commented Aug 15, 2017

nalabjp commented Aug 20, 2017

nalabjp commented Aug 20, 2017

hanachin left a comment

Choose a reason for hiding this comment

yasulab commented Aug 21, 2017

yasulab commented Aug 21, 2017

yasulab commented Aug 21, 2017

nalabjp commented Aug 21, 2017

nalabjp commented Jul 30, 2017 •

edited

yasulab commented Aug 15, 2017 •

edited