Track app detail page visits (Bug 965965) #1745

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

nearlyfreeapps commented Feb 10, 2014

From Bug 965965, this adds a new table to facilitate tracking app details page views for creating better recommendations.

+
+ user = getattr(self.request, 'amo_user', None)
+ if user:
+ DetailsViewed.objects.get_or_create(addon=app, user=user)
@cvan

cvan Feb 10, 2014

Member

so for non-authenticated users, we're creating only one row, so that makes these counts accurate only for authenticated users

@diox

diox Feb 10, 2014

Member

I think that's fine to not count anonymous users (though @robhudson might disagree), but... Do we want:

  1. Only the first visit ?
  2. Only the last visit ?
  3. All visits ? (i.e., a counter)

Options 2. and 3. require additional code.

@robhudson

robhudson Feb 10, 2014

Member

For now the purpose of this is to feed recommendations and we only need to know that the user visited the app detail at least once. It's a signal to the recommendation engine that the user showed some interest in the app. So we don't care about how many times they visited or when. So as-is this looks good to me.

@@ -1901,6 +1901,16 @@ def add_uuid(sender, **kw):
install.save()
+class DetailsViewed(amo.models.ModelBase):
@cvan

cvan Feb 10, 2014

Member

it saddens me that we need to create a new MySQL table. this is a perfect use for a key/value store.

@diox

diox Feb 10, 2014

Member

👍, but do we have any key-value store currently available that we can use for this ? Also keep in mind that we might have to be able to dump all entries at some point.

@robhudson

robhudson Feb 10, 2014

Member

I don't think a new SQL table is sad.

@cvan

cvan Feb 10, 2014

Member

we are already using redis for django-cache-machine. a new SQL table is not sad, but when it's to store a key and a value, yes.

+ UNIQUE (addon_id, user_id)
+) ENGINE=InnoDB CHARACTER SET utf8 COLLATE utf8_general_ci;
+
+ALTER TABLE users_details_view ADD CONSTRAINT users_details_view_addon_fk FOREIGN KEY (addon_id) REFERENCES addons (id);
@cvan

cvan Feb 10, 2014

Member

add ON DELETE CASCADE

Member

diox commented Feb 10, 2014

Sorry, we can't do it like that. Here is why:

  • If you go to the app details page through search, which is what 99% of users will do, no call to the app API is made - fireplace already has the model in its cache.
  • This can slow down the app API and we want it to be fast. The insertion/update should be done asynchronously in a separate API request.
  • This is an action and shouldn't be done via GET. GET can be cached.
  • We are working on getting rid of user-dependent stuff in search and app API to get better cacheability.
@@ -1901,6 +1901,16 @@ def add_uuid(sender, **kw):
install.save()
+class DetailsViewed(amo.models.ModelBase):
+ """Track WebApp details page views."""
@robhudson

robhudson Feb 10, 2014

Member

Perhaps an extra comment as to the why of this table?

Member

robhudson commented Feb 10, 2014

If you go to the app details page through search, which is what 99% of users will do, no call to the app API is made - fireplace already has the model in its cache.

Good point. I hadn't considered that.

This can slow down the app API and we want it to be fast. The insertion/update should be done asynchronously in a separate API request.

Fair enough. (I'm curious tho if there is any overhead and what it might be to insert a task into a celery queue)

This is an action and shouldn't be done via GET. GET can be cached.

I'd be fine with recording these on GETs but the caching part is the main point here.

It seems like this isn't going to work especially because of the 1st and 3rd points. What other options are there to record app detail views?

Member

diox commented Feb 10, 2014

My suggestion was to create a new, separate API and have it called from fireplace asynchronously, once everything has been loaded.

Member

robhudson commented Feb 10, 2014

Closing. We're going to rework this as an API fireplace calls.

@robhudson robhudson closed this Feb 10, 2014

@nearlyfreeapps nearlyfreeapps deleted the nearlyfreeapps:bug-965965 branch Feb 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment