New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server side analytics #4131

Merged
merged 11 commits into from Jun 7, 2018

Conversation

Projects
None yet
3 participants
@davidfischer
Contributor

davidfischer commented May 23, 2018

This PR sends an event to Google Analytics from the server side when the Footer API is called.

This is one of our identified possible alternatives to client side GA. Sending data from the server side has many advantages such as being able to anonymize data before it is sent and not having clients directly connect to a site they may not want to. It comes with the downside of needing to have many web servers to handle all our analytics calls.

I am seeking feedback on the structure especially around the structure in terms of using signals/hooks to find when footer calls are made and whether this seems like a sane approach.

@davidfischer davidfischer requested review from ericholscher and agjohnson May 23, 2018

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented May 23, 2018

I'm expecting a lint error here around the naming of the log variable but I believe that means our linting is a bit overzealous.

davidfischer added some commits May 23, 2018

@ericholscher

👍 Looks great with some gating implemented, unless I'm missing it.

For this use case, I think it might actually be more useful to have a sample that isn't project specific, since our initial goal is to compare ad rates across all projects. However, we don't have a way to easily handle that from the DB, so I don't want scaling to require a deploy to change the % of traffic to send.

@@ -0,0 +1 @@
"""Intentionally blank"""

This comment has been minimized.

@ericholscher

ericholscher May 24, 2018

Member

I don't think this is necessary with the AppConfig.

'ua': request.META.get('HTTP_USER_AGENT'),
'uip': get_client_ip(request),
}

This comment has been minimized.

@ericholscher

ericholscher May 24, 2018

Member

It doesn't look like this is gated by the feature flag, so it would start sending all footer data.

This comment has been minimized.

@davidfischer

davidfischer May 25, 2018

Contributor

Correct, it is not yet.

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented May 25, 2018

For this use case, I think it might actually be more useful to have a sample that isn't project specific

How about sending ad clicks to GA server side rather than client side? That would be hundreds (but not thousands) of clicks per day.

davidfischer added some commits May 29, 2018

@ericholscher

Looks great. I just mentioned a couple small nits.

'ec': None, # Event category (required)
'ea': None, # Event action (required)
'el': None, # Event label
'ev': None, # Event value (numeric)

This comment has been minimized.

@ericholscher

ericholscher Jun 5, 2018

Member

Should we be setting these to None by default? It seems like we should be a bit more defensive here, and make sure the incoming event_data contains them.

log = logging.getLogger(__name__) # noqa
# Used to anonymize an IP by zero-ing out the last 2 bytes
MASK = int('0xFFFFFFFFFFFFFFFFFFFFFFFFFFFF0000', 16)

This comment has been minimized.

@ericholscher

ericholscher Jun 5, 2018

Member

Should this just live in the anonymize_ip_address function? Not sure if we need it elsewhere.

'https://www.google-analytics.com/collect',
data=data,
)
except requests.Timeout:

This comment has been minimized.

@ericholscher

ericholscher Jun 5, 2018

Member

What is the default timeout here? We should probably set it to something really low.

['rtfd._trackEvent', 'Promo', 'Click', data.id]
);
}
};

This comment has been minimized.

@ericholscher

ericholscher Jun 5, 2018

Member

I wonder about leaving this logic in, until we can properly confirm the server-side stuff is working. Not sure how important it is, if it's better to possible double count or to just lose the data.

davidfischer added some commits Jun 6, 2018

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Jun 6, 2018

I made the updates based on the feedback including making it more obvious what is required in terms of analytics.

@ericholscher ericholscher merged commit 732b37c into rtfd:master Jun 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ericholscher

This comment has been minimized.

Member

ericholscher commented Jun 7, 2018

💯

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