Skip to content
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

Refactor analytics #443

Merged
merged 1 commit into from Sep 2, 2016
Merged

Refactor analytics #443

merged 1 commit into from Sep 2, 2016

Conversation

monfresh
Copy link
Contributor

@monfresh monfresh commented Sep 1, 2016

Why: To make it easier to use, and to allow for further
simplification down the road.

How:

  • Allow the track_event method to take a hash as a second argument,
    which allows adding various properties, rather than having to use the
    event title as a differentiator. For example, instead of having two
    event names to track success and failure scenarios, we only have one
    event name with different attributes. For example:
    track_event(:auth, result: 'success') and
    track_event(:auth, result: 'failed').
  • By default, the track_event method assumes the user is the
    current_user or an instance of AnonymousUser. If you need to pass in
    a specific user, pass in a hash with the key user_id and the user's
    uuid as the value, such as
    track_event(:password_reset, user_id: user_from_token.uuid).
  • Remove the track_anonymous_event method since it's no longer
    needed.
  • Remove the staccato gem and server-side Google Analytics since it
    doesn't support hash attributes for events. There are other services
    that can better suit our needs, such as Keen.io, or even storing the
    data in our own DB.

**Why**: To make it easier to use, and to allow for further
simplification down the road.

**How**:
- Allow the `track_event` method to take a hash as a second argument,
which allows adding various properties, rather than having to use the
event title as a differentiator. For example, instead of having two
event names to track success and failure scenarios, we only have one
event name with different attributes. For example:
`track_event(:auth, result: 'success')` and
`track_event(:auth, result: 'failed')`.

- By default, the `track_event` method assumes the user is the
current_user or an instance of AnonymousUser. If you need to pass in
a specific user, pass in a hash with the key `user_id` and the user's
uuid as the value, such as
`track_event(:password_reset, user_id: user_from_token.uuid)`.

- Remove the `track_anonymous_event` method since it's no longer
needed.

- Remove the staccato gem and server-side Google Analytics since it
doesn't support hash attributes for events. There are other services
that can better suit our needs, such as Keen.io, or even storing the
data in our own DB.
@pkarman
Copy link
Contributor

pkarman commented Sep 2, 2016

It's not clear to me reading the diff whether anonymous events are tracked with a user_id just like authenticated events. If so, I expect that will be confusing when looking at the logs, seeing a user_id and not being able to reconcile it to the database.

@monfresh
Copy link
Contributor Author

monfresh commented Sep 2, 2016

By default, the user_id will be that for the current_user if they are signed in. No need to explicitly pass in the user_id to the track_event method in that case. Otherwise, if the user is not signed in, but we can look them up in the database (via a password reset or email confirmation token for example), then we explicitly pass in the user_id to the track_event method. Otherwise, if we don't know who the user is, the Analytics class will automatically use the uuid of the AnonymousUser instance, which is just the string anonymous-uuid, which will be clear in the logs.

@monfresh
Copy link
Contributor Author

monfresh commented Sep 2, 2016

This was the same behavior as before. This PR did not change the behavior. It just made it possible to have richer and clearer information in the logs, and easier to collect that information.

@monfresh
Copy link
Contributor Author

monfresh commented Sep 2, 2016

The benefit of this attributes hash approach is that we can design our Service Objects that controllers interact with such that they return a hash of attributes about the request, which will allow us to call the track_event method only once, as opposed to multiple times inside various conditionals based on the result of the request.

For example, let's say a user is trying to reset their password. There are 2 ways this could fail in the first step:

  • the user could enter an invalid token
  • the user could enter an expired token

Instead of calling track_event 2 times for each scenario, we can design a Service Object that returns a hash that includes the error message, and we can dynamically include that in the event hash with only one call, such as:

result_hash = ServiceObject.new(params).result
analytics.track_event(:password_reset, result_hash)

In the logs, this would show something like:

{ 
  name: 'password_reset',
  user_id: 'd7979d00-ef75-4d6f-8a5b-33d53af25c67', 
  errors: 'token_expired' 
}

@pkarman
Copy link
Contributor

pkarman commented Sep 2, 2016

Thanks for the details. The string anonymous-uuid addresses my concern.

@pkarman
Copy link
Contributor

pkarman commented Sep 2, 2016

net loss of 70 lines of code! lovely.

@pkarman pkarman merged commit 3a7d2f9 into master Sep 2, 2016
@pkarman pkarman deleted the refactor-analytics branch September 2, 2016 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants