Skip to content

Comments

イベント履歴収集スクリプトの改修#375

Merged
chicaco merged 11 commits intomasterfrom
mod_aggregation
Mar 4, 2019
Merged

イベント履歴収集スクリプトの改修#375
chicaco merged 11 commits intomasterfrom
mod_aggregation

Conversation

@chicaco
Copy link
Contributor

@chicaco chicaco commented Jan 21, 2019 •
edited by yasulab
Loading

Fix #258 (直近の CoderDojo 開催情報を表示したい)

背景

統計スクリプトが本番環境でも失敗しているので、直さないといけない
※ 2018/12/03 は成功していたが、その後失敗

やりたいこと

  • イベント履歴収集スクリプトが本番環境で動いていないので、動かせるようにする
    (2018/12/02 までは収集済み)
  • 併せて、レート制限エラー対応も行う
    現 : 週 or 月の期間 × dojo ごと収集 → 新 : dojo × 一括 or 指定可能なまとまった期間 ごと収集
  • rake タスクの usage を追加する

Fix #371

このPRでやること

  • gem 'faraday_middleware' の復活
  • 週/月の期間ごとの繰り返し収集から、dojo ごとできるだけ一括の期間指定での収集に変更
  • usage を追加
  • 期間判定処理などの rspec 追加
  • connpass, doorkeeper のイベント履歴収集の動作確認
  • アプリレビューで承認されたアカウントのトークンで、facebook のイベント履歴収集の動作確認 facebook イベントのグラフAPI経由での履歴収集はスコープ外にした Facebook Events の情報収集を自動化したい #393

やらなかったこと

特になし

レビューポイント

  • 対象期間の判定
  • lib/statistics/aggregation.rb で一部サブクラスを廃止した
  • lib/statistics/tasks 配下のクラスと、lib/event_service/providers 配下のクラスの守備範囲を一部変更

困ってること

🆕 追記: 本機能リリース後の反響 😻✨

image

image

@chicaco
Gemfile に gem 'faraday_middleware' を追記
5631f29
@chicaco
統計情報収集処理の期間指定を見直し
0cd1300 
日/月ごとの取得でなく、dojo ごとにできるだけ一括取得に
(connpass は期間指定の場合、月or日 の組み合わせにする必要がある)
@chicaco
イベント履歴収集タスクコマンドの説明を追加
417cc7a
@yasulab
Copy link
Member

yasulab commented Jan 21, 2019

PR ありがとうございます! (＞人＜ )✨

WIP の完了条件が書いてないので、何が完了すると WIP が外れるのか Check Box 形式で書いてもらえると助かります 😆 (Check Box 形式にして、Check Box がすべて ✅ されると完了、という形式だとわかりやすいです 😸)

cf. #364

@yasulab
Copy link
Member

yasulab commented Jan 21, 2019

やらなかったこと

やらなかったことは Check Box 形式じゃない方が良さそうですね 👀 「この PR ではやらない」と決めたことなので。

一方で、1つの PR にテストも含めていて欲しいので、

  • rspec 追加

までやってもらえると嬉しいです (＞人＜;) 💦

cf. yasslab/yasslab.jp#61

@yasulab
Copy link
Member

yasulab commented Jan 21, 2019

レビューポイントあるのはいいですね! どこを見ればいいかの参考になって助かります 😆✨

@yasulab yasulab requested a review from nalabjp January 21, 2019 06:01
@chicaco
Copy link
Contributor Author

chicaco commented Jan 21, 2019

ひとまず PR コメントを訂正してみました。 ✨

@yasulab facebook のグループイベントについて
本番環境でも取得できていないようでしたら、facebook 分の収集だけやり直すことができる(プロバイダの種類＋期間指定？)ような機能追加も必要かも知れません。

ご意見、お願いします。
(別 issue にして対応する感じで)

@yasulab
Copy link
Member

yasulab commented Jan 21, 2019

facebook のグループイベントについて
本番環境でも取得できていないようでしたら、facebook 分の収集だけやり直すことができる(プロバイダの種類＋期間指定？)ような機能追加も必要かも知れません。ご意見、お願いします。

なるほど 🤔 @chicaco さんとしてはどのように進めていくと良いと考えていますか？

この PR にその機能も含めてしまうべきですかね？それとも別 Issue / 別 PR で対応した方が良いですかね？ 💭

@chicaco
Copy link
Contributor Author

chicaco commented Jan 21, 2019

なるほど 🤔 @chicaco さんとしてはどのように進めていくと良いと考えていますか？

この PR にその機能も含めてしまうべきですかね？それとも別 Issue / 別 PR で対応した方が良いですかね？ 💭

@yasulab 別 Issue / 別 PR に切り分けてよいかと考えていました。

理由は、以下の通りです。

  • 2018/12/03 以降は全プロバイダ分の収集ができていない
  • facebook イベントがどこから収集できていないか確認できていない(12/02 まで収集できているようでしたら、すぐに必要な機能ではない)
  • 全プロバイダ分は収集し直すことはできる

@yasulab
Copy link
Member

yasulab commented Jan 21, 2019 •
edited
Loading

なるほど！その方向が良さそうですね！😆✨

まとめる必要性もないですし、少しずつリリースできた方がバグが出たときも対応しやすいので、別 PR / 別 Issue で進めていきましょう d(￣ ￣)✨

@nalabjp
Copy link
Member

nalabjp commented Jan 21, 2019

#371 (comment)
#371 (comment)

上記issueコメントについての情報がないのでまずはこちらを整理したほうがいいと思いました。
descriptionの「困っていること」を見たところローカル環境での動作確認も取れていないようなので、このPRをマージしても改善しない可能性もありそうに思います。
facebookのdeveloper toolでの動作確認やrails consoleからの最小限のAPIコールでの確認の上、改修の必要性や方向性を検討したほうが良さそうに思います。
レートリミットは利用者数に依存するみたいなので、ウェイト間隔等をどう調整するかはコール可能な実数に影響するかもしれません。

faraday_middlewareの必要性については
#371 (comment) という理由なので問題ないかと思います。

@yasulab
Copy link
Member

yasulab commented Jan 22, 2019

Description の「完了条件」と「やったこと」を別々に分ける必要がないので、まとめておきました ;) cc/ @chicaco

@chicaco
Copy link
Contributor Author

chicaco commented Jan 22, 2019 •
edited
Loading

コメントありがとうございます。 > @nalabjp @yasulab

#371 (comment)
#371 (comment)

上記issueコメントについての情報がないのでまずはこちらを整理したほうがいいと思いました。

失礼いたしました。

descriptionの「困っていること」を見たところローカル環境での動作確認も取れていないようなので、このPRをマージしても改善しない可能性もありそうに思います。
facebookのdeveloper toolでの動作確認やrails consoleからの最小限のAPIコールでの確認の上、改修の必要性や方向性を検討したほうが良さそうに思います。

connpass, doorkeeper (もちろん static_yaml も) については、ローカル環境で動確取れており、
残すは facebook のみです...。

レートリミットは利用者数に依存するみたいなので、ウェイト間隔等をどう調整するかはコール可能な実数に影響するかもしれません。

dooekeeper もレート制限あるようです。
https://www.doorkeeper.jp/news/2016/3/1/update-to-api
ですので、dojo ごと一括取得は効果あり、と思っています。

faraday_middlewareの必要性については
#371 (comment) という理由なので問題ないかと思います。

お手数お掛けしました。 🙇‍♀️

ひとまず、rspec 追加します。

@chicaco chicaco mentioned this pull request Jan 22, 2019
Open
2 tasks
@chicaco chicaco changed the title [WIP] イベント履歴収集スクリプトの改修 イベント履歴収集スクリプトの改修 Jan 22, 2019
@chicaco
Copy link
Contributor Author

chicaco commented Jan 22, 2019

RSpec 追加しましたので、[WIP] は削除しました。

@chicaco
Copy link
Contributor Author

chicaco commented Jan 23, 2019

説明を見直しました。
また、facebook の動確をやることに追加しました。

@yasulab
Copy link
Member

yasulab commented Jan 23, 2019

説明を見直しました。
また、facebook の動確をやることに追加しました。

d(￣ ￣)✨

@chicaco chicaco self-assigned this Jan 24, 2019
@chicaco chicaco mentioned this pull request Jan 24, 2019
Closed
2 tasks
@chicaco
Copy link
Contributor Author

chicaco commented Feb 18, 2019

Facebook のグループイベント取得について、作戦変更することになりました。
#371 (comment)
ざっと確認したところ、アプリレビューが通るだけでなく、グループ管理者にアプリをインストールしてもらう必要があることが分かりました。
https://developers.facebook.com/docs/graph-api/reference/v3.2/group/events
@yasulab さんと相談し、各Dojoへの依頼＆フォローなどのコストが高くなりそうなので、一旦 Facebook のグラフAPI 経由でのイベント情報取得を断念することにしました。

ということで、Facebook イベントの情報収集は、static_yaml 形式で暫定運用します。

イベント履歴収集スクリプトも、この方針に沿って再修正します。

→ 再度 WIP に戻し、「このPRでやること」を訂正します。

@chicaco chicaco changed the title イベント履歴収集スクリプトの改修 [WIP] イベント履歴収集スクリプトの改修 Feb 18, 2019
@chicaco
Copy link
Contributor Author

chicaco commented Feb 18, 2019

本番環境DBのイベント履歴レコードを確認し、収集できていない Facebook イベント分を static_yaml 化する必要がある。
⇒ 別 issue & PR にする

@chicaco
Copy link
Contributor Author

chicaco commented Feb 25, 2019 •
edited
Loading

MEMO
dojo_event_services テーブルの name カラムは enum で登録済みなので、ここは変更しない。
enum のカウンタが変わらないよう、facebook も残しておく必要がある。
残しつつ、static_yaml 相当の処理を行うようにする。
また、既に登録済みの履歴情報は削除する必要がないので、期間指定は残す。

理由：
他に適切な facebook のイベント取得方法が見付かったときの仕込み。

@chicaco
Copy link
Contributor Author

chicaco commented Feb 26, 2019

Dumper.io から取得した DB バックアップ 20190211_230105.sql.gz に、static_yaml 分のイベント履歴が含まれていない。
connpass, doorkeeper, facebook 分は、指定された期間のイベント履歴を一旦削除して再収集＆登録しているので 2018/12/09 分まで残っているが、static_yaml 分は一旦全イベント履歴を削除して登録するようになっており、facebook で失敗すると static_yaml の処理に進まないため削除したままで終わっている。

過去の実績が変わってしまいますが、「より正しく」なることはヨシとしてよいでしょうか？ > @yasulab

@yasulab
Copy link
Member

yasulab commented Feb 26, 2019 •
edited
Loading

過去の実績が変わってしまいますが、「より正しく」なることはヨシとしてよいでしょうか？ > @yasulab

良いと思います! d(￣ ￣)✨ 数字が大きく下方修正するようであれば一般に周知する必要性も出てくると思うので、その可能性も加味しておくと良さそうですね ;)

@chicaco
Copy link
Contributor Author

chicaco commented Feb 26, 2019

良いと思います! d(￣ ￣)✨ 数字が大きく下方修正するようであれば一般に周知する必要性も出てくると思うので、それも加味して吟味できると良さそうですね ;)

了解です。

facebook でエラーが出始める前までは登録できていた static_yaml 分が今は登録できていない状態なので、今現在表示されている履歴は 2018/11 時点の数字より「少なく」なっているかと。

@chicaco
Copy link
Contributor Author

chicaco commented Feb 26, 2019

dojo_event_services テーブルに、facebook 系のみ、おそらく削除したかったレコードが残っていそう。
dojo は dojo_event_services を has_many で持っているので複数の dojo_event_services を持つこと自体は 🆗 だが、同じ group_id の facebook events の dojo_event_service を持っているのは意味がない。
2017/11 ごろから重複レコードを持っている模様。

rails dojo_event_services:upsert で dojo_event_service のレコードを生成 or 更新するとき、url が異なる(※)ことも想定の上で insert になっている？？
(※) 旧 url の最後には '/?ref=page_internal' があり、2017/10/末 くらいに更新された dojo_event_services.yaml の url にはこれがない。

'/?ref=page_internal' 付きの url を持つレコードが残っていても、イベント履歴収集では今の処悪さはしていなさそうではあるが...。

@chicaco chicaco changed the title [WIP] イベント履歴収集スクリプトの改修 イベント履歴収集スクリプトの改修 Feb 28, 2019
@chicaco
Copy link
Contributor Author

chicaco commented Feb 28, 2019

Facebook イベント履歴の収集について、グラフAPI経由を止め、facebook_event_histories.yaml からの読み込みに変更しました。
facebook_event_histories.yaml には、2018/04/08 までのイベント(すでに DB に登録されていたもの)まで登録済みです。

@chicaco
Copy link
Contributor Author

chicaco commented Mar 4, 2019

@yasulab @nalabjp お手隙のときにレビューお願いします。

長らくかかってしまいましたが、やっとここまで...。

facebook_event_histories.yaml には、すでに DB に登録されていた 2018/04/08 までのイベントに加え、2019/02/末 までのイベント情報を記載済です。

@yasulab
Copy link
Member

yasulab commented Mar 4, 2019 •
edited
Loading

お疲れ様です!! 😆🎉✨ 後で見ますね〜 👀 (と言いつつ、遅いと今週末ぐらいになりそうです... >< 💦 )

@chicaco
Copy link
Contributor Author

chicaco commented Mar 4, 2019 •
edited by yasulab
Loading

以下、備忘のためのメモです。

(1) 2018 年分の実績収集

マージ＆リリース後、2018/04～ の期間を指定してイベント履歴収集スクリプトを実行していただければ、2018 年の実績が揃います。「connpass 側の検索処理が修正されていれば」という条件付きですが。

(2) スクリプト実行日/間隔

スクリプト実行前に yaml にイベント情報を追記する必要があるので、現状通り週次の月曜朝実行ですと、週末開催の facebook 分を収集対象にするのは厳しいかもしれません。

しばらく手作業による追記を行うとして、月曜に yaml 作成 → 火曜朝に収集、が現実的な気がします。
もしくは、プロバイダ指定での履歴収集に対応したら、Facebook 分のみ月次収集にしてしまうとか。

nalabjp
nalabjp approved these changes Mar 4, 2019
View reviewed changes
Copy link
Member

@nalabjp nalabjp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@yasulab yasulab self-requested a review March 4, 2019 09:27
yasulab
yasulab requested changes Mar 4, 2019
View reviewed changes
Copy link
Member

@yasulab yasulab left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

良さそう! lib/tasks/docs/statistics_aggregation.md のドキュメントは README か /docs に置いておいたほうが見つけやすそうかなと思いました ✅ 👀 💭

マージのタイミングはお任せしますね! chicaco さんの都合の良いタイミングにマージしていただければ d(￣ ￣)✨

(間違って request changes を押してしまった...orz)

@chicaco chicaco merged commit fa0d493 into master Mar 4, 2019
This was referenced Mar 5, 2019
Merged
Open
Closed
Merged
Closed
Merged
@chicaco chicaco deleted the mod_aggregation branch March 28, 2019 14:46
@yasulab yasulab mentioned this pull request Jun 6, 2019
Closed
yasulab added a commit that referenced this pull request May 27, 2025
@yasulab
Replace deprecated faraday_middleware gem with faraday default functi…
6df84f7 
…ons for better maintenance #375
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yasulab yasulab yasulab approved these changes

@nalabjp nalabjp nalabjp approved these changes

Assignees

@chicaco chicaco

Labels

None yet

Projects

None yet

Milestone

No milestone

3 participants

@chicaco @yasulab @nalabjp