Add table and job for daily tracking and recording of wiki metrics#880
Conversation
rosalieper
force-pushed
the
T383421
branch
3 times, most recently
from
bacc211 to
37591b6
Compare
tarrow
left a comment
tarrow
left a comment
There was a problem hiding this comment.
Just a few thoughts for you to while over; looks like a great start :)
| use Illuminate\Contracts\Queue\ShouldQueue; | ||
| use Illuminate\Foundation\Bus\Dispatchable; | ||
|
|
||
| class UpdateWikiMetricDailyJob implements ShouldQueue |
There was a problem hiding this comment.
I get how we got here but I wonder if we can think of a way to name this or add a comment to make it clearer how this jobs differs from all of the other metric jobs
| $isDeleted = (bool)$this->wiki->deleted_at; | ||
| if( !$oldRecord || $oldRecord->pages != $todayPageCount || !$oldRecord->is_deleted ) { | ||
| WikiDailyMetric::create([ | ||
| 'id' => $this->wiki->id . '_' . date('Y-m-d'), |
There was a problem hiding this comment.
Looks like this is doing exactly what the ticket asked; I wondered for a while if it would be better to have a a primary key that is less of a natural key. See for example https://stackoverflow.com/a/718200
Perhaps you should ask Anton if he's really sure he wants this: for example is he certain he won't want to get two of these in a single day? Would there ever be an issue in merging or splitting wikis in the future?
OTOH I think this isn't a dealbreaker and we can probably live with it either way
| { | ||
| $today = now()->format('Y-m-d'); | ||
| $oldRecord = WikiDailyMetric::where('wiki_id', $this->wiki->id)->latest('date')->first(); | ||
| $todayPageCount = $this->wiki->wikiSiteStats()->first()->pages ?? null; |
There was a problem hiding this comment.
probably fine to just null this; to note this can happen if the wikiSiteStats update job hasn't run at all for this wiki yet
There was a problem hiding this comment.
I am not sure I understand. Do you mean I should set the value of $todayPageCount to null?
There was a problem hiding this comment.
After looking it's not fine to null this since pages claims to be an integer https://github.com/wbstack/api/pull/880/files#diff-516b6ba1298695ebbde7ccd7327da6954e9d29a05b82c3c51898d6fa7ad64a91R17
| $wikis = Wiki::all(); | ||
|
|
||
| foreach ($wikis as $wiki) { | ||
| dispatch(new UpdateWikiMetricDailyJob($wiki)); |
There was a problem hiding this comment.
Probably fine... I wonder if we're going to find that in production creating 1k+ jobs all at the same time is is problematic; how much worse would you find it to add a job then then spawns the other jobs over a period of time? Maybe we could even add a sleep to this closure here?
There was a problem hiding this comment.
One way to go about this would be to loop through the wikis in one job, instead of calling a job per wiki but my issue was how to test that. I wanted a way to inject the wiki in the job for testing.
| WikiDailyMetric::create([ | ||
| 'id' => $this->wiki->id . '_' . date('Y-m-d'), | ||
| 'pages' => $todayPageCount, | ||
| 'id_deleted' => $isDeleted, |
There was a problem hiding this comment.
I discovered trying to run it that this is a typo
| 'id_deleted' => $isDeleted, | |
| 'is_deleted' => $isDeleted, |
Add Job for the above table updates daily Bug:T383421
rosalieper
changed the title
| ]); | ||
| } | ||
|
|
||
| /** @test*/ |
There was a problem hiding this comment.
| /** @test*/ |
| { | ||
| use RefreshDatabase; | ||
|
|
||
| /** @test */ |
There was a problem hiding this comment.
| /** @test */ |
tarrow
left a comment
tarrow
left a comment
There was a problem hiding this comment.
looks good to me; i'd remove these test annotations since we don't have them anywhere else.
Let's ship this thing and see how it works!
2 participants
Added new table
wiki_daily_metricsAdded a new
Wiki.phpclass in the metric folder for wiki related metricsAdded a Job scheduled to run daily and update metrics in the
wiki_daily_metrictable.Bug: T383421