holman / ama

Ask @holman anything!
731 stars 277 forks source link

How do activity feeds on individual issues aggregate events on github? #722

Closed abitdodgy closed 9 years ago

abitdodgy commented 9 years ago

How does the individual issue activity feed aggregate events? I added two labels within a few seconds of each other, and each event immediately appeared on its own row. But then, when I refreshed the page, the two activities were grouped into one row. How does Github do this? Is each event recorded separately, and then grouped and cached at query time? Or are they recorded in groups if they happen within a certain period of time, such that the new activity is appended to an existing one that had happened within a given timeframe?

It looks like issue events, judging by the markup, are not rendered from AR objects. Maybe Redis? I'm just curious as to how the thing works.

gh_issue gh_issue_2

wadewilliams commented 9 years ago

:+1:

holman commented 9 years ago

Issues and pulls have many objects from the issue_events table. The table itself is kind of used as a dumb audit log that's never really updated, just read from after creation.

It's actually fairly complicated when you look at all of the possible events here:

Each of those have separate views that render them, but there's also a number of grouped views as well: labeled and unlabeled can be rolled up, as well assigned and unassigned and maybe milestones too I think.

When you load an issue, there's a method that combines all of the above issue events in chronological order along with comments (of which pulls can have many: Comments, ReviewComments, and a couple others I can't remember right now). With the finalized timeline, you'd grab the view for each and render them all on-page.

When I built GitHub Issues, I wanted to handle these grouped rollups because many people would do multiple events at once: remove two labels, add three labels, and so on. It would take a lot of vertical space that made the whole thing a bit confusing to look at.

So in that main method that combined everything, I split it out into an array of groups of like-minded rollups (labels and unlabels, assignment and unassignment, etc). You also need to look at the user_id on the rollup, since if you label something and then I label something we don't want to attribute both of those actions to you. I believe there's also a time component as well, so if you add a label, and then a month later you remove a label, those aren't simplified in a rollup (since they aren't really the same action that happened at the same time). All the permutations are pretty complicated (and not incredibly performant, so caching is done here as well).

screen shot 2015-09-01 at 11 37 28 am

To make matters a bit more complicated, we'd see people who, over the course of a couple minutes or so, would add and remove and add and remove and add and remove the same label (users, amirite?). We have some code to prevent adding the same label in a row, and removing the same label in a row, but not when it goes back and forth like that. Ultimately if it happens in such a short timespan you can probably safely say that they all cancel each other out, so visually those events just get simplified down to the basic add/remove event combination.

But then, when I refreshed the page, the two activities were grouped into one row.

This is technically a bug: the code to generate the grouping on the backend is complicated enough, and to have perfect knowledge of the DOM, duplicate all the views, build something that's smart enough to understand grouping, handle rollups of ungrouped events, deal with backstepping through previous events to make decisions about whether it should be grouped, and plugging all of this into the websockets backend already set up for live-updates on issue pages... well fuck, that's a non-trivial undertaking. Our punting was helped by the notion that new events — say you just added a label — are kind of better-suited to visually split them out initially and then roll them up on a hard page refresh, since otherwise you could argue there's not enough visual distinction that your action actually affected was reflected on-page.

My knowledge of the system is about six months removed, so things may have changed (although one of the amusing parts about having worked at GitHub is that you don't need to update many screenshots in your talks or diminish your working knowledge of the system, since it doesn't change much anymore). I think the most likely thing to have changed since then was the issue_events table, which I think was by far the biggest table on the site and was getting data thrown at it pretty mercilessly. There's a couple relatively easy ways to tackle that perf hotspot, and I wouldn't be surprised if there's been some work done there.

All in all, the conversation timeline was one of my favorite (and least favorite) things to work on during the Issues rebuild. Dealing with all the permutations took a couple of rewrites and headaches to get right, but it was fun adding all the events: there were really only a couple events beforehand, and I added about ten, including the dope issue rename events, which were sort of an afterthought. Also a big fan of the issue locking events, because people on the internet are assholes.

Anyway, hope that helps clarify a few things on the conversation timeline. Definitely a lot happening behind-the-scenes when you load even a humble issue.

abitdodgy commented 9 years ago

@holman thanks, this was really, really helpful.

jankeesvw commented 9 years ago

Thanks @holman I was wondering about this too!

rick commented 9 years ago

:heart:

josh commented 9 years ago

My knowledge of the system is about six months removed, so things may have changed

Nah, basically all the same. :D

But then, when I refreshed the page, the two activities were grouped into one row.

Yeah, this one is my fault. Live updates to the timeline only ever append new events (never rewrite previous events) for simplicity.

Some of the labeling events have slightly changed since. We now only apply labels when the menu is dismissed. So if you toggle "suuuuuuup" one and off ten times, we'll only apply state when its closed. Prevents accidental labeled/unlabeled events.

swmcc commented 9 years ago

Really good answer. Thanks @holman

dideler commented 9 years ago

Great question and answer (and update by josh).

lachlanjc commented 9 years ago

Thanks @holman and @josh!

@josh — even if you just wanted to keep it simple and not rewrite past events until reload, I think the "lazy" functionality is actually much easier to understand. As I'm tagging and assigning, I can easily see what I've done just now, without being confused as to the order of actions. Then, coming back to a long conversation, all the little status changes don't matter so much anymore, so grouping them is better. 👊