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

Projects: Publishing A/B test log to firehose #24322

Merged
merged 2 commits into from Aug 22, 2018

Conversation

Erin007
Copy link
Contributor

@Erin007 Erin007 commented Aug 16, 2018

Follow up to #24257 and #24291.

This PR logs to Redshift when a the user (un)publishes via the buttons in the button variant of the project publishing A/B test and when the user (un)publishes via the quick action in the dropdown menu in the chevron variant. Poorva requested that user id also be included in the logs; part of accomplishing that is passing a userId prop into the relevant child components.

Next, we'd like to set up tracking clicks on the chevron icon itself which will be slightly more complicated as the QuickActionsCell is used in multiple tables throughout the site.

Example data being logged:
screen shot 2018-08-22 at 10 48 49 am

study: 'project-publish',
study_group: 'publish-button',
event: 'publish',
user_id: this.props.userId,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't done this kind of logging yet. 1.) can I just add in extra fields like this? The examples I've found (mostly #22486) don't. 2.) Is there a way to check locally that this logging is working or do I just ask Poorva to check Redshift once the change hits staging?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.) So this should actually work, but it was probably not meant to work this way.

You can't normally just add new fields like this, you would first need to add columns to the table that this logs to. The available fields are here:

* firehoseClient.putRecord(
* {
* study: 'underwater basket weaving', // REQUIRED
* study_group: 'control', // OPTIONAL
* event: 'drowning', // REQUIRED
* data_int: 2 // OPTIONAL
* data_float: 0.31 // OPTIONAL
* data_string: 'code.org rocks' // OPTIONAL
* data_json: JSON.stringify(x) // OPTIONAL
* }
* );

However user_id actually exists already, but it's not meant to be specified in the data argument to the putRecord call. It's a field that is automatically added to the firehose record if you include includeUserId: true in the options argument to putRecord:

putRecord(data, options = {alwaysPut: false, includeUserId: false, callback: null}) {

Unfortunately, you then run into the problem that the pageConstants reducer isn't used on non-puzzle pages, so the firehose code can't actually look up the user_id when you're in the project gallery:

const constants = state.pageConstants;
if (constants) {
data['user_id'] = constants.userId;
}

The way that addCommonValues is written though, your code should actually just work. It won't touch your manually specified user_id, and firehose/redshift won't know the difference between this and a normal automatically populated user_id.

I think you have two options:

  1. Add the pageConstants reducer to the redux store in the project gallery, and populate the user_id so that the includeUserId option works as intended.
  2. Update the usage comment for putRecord to officially allow a user_id property (but also mention the includeUserId option in the comment), and update the code above so that it doesn't override a manually specified user_id with the one from redux. You probably want to do this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2.) I think that you can pass the alwaysPut option to putRecord to force it to log to firehose in development mode, but you'd still have to ask Poorva to check redshift to see if the record was added (or set up your own access to redshift). If my memory serves, you also have to wait a day or so for the record to actually show up. Turns out it shows up in Redshift within minutes.

I usually don't bother with that. In development mode putRecord logs to the JS console instead of to firehose, I just make sure it does that.

study: 'project-publish',
study_group: 'publish-button',
event: 'publish',
user_id: this.props.userId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.) So this should actually work, but it was probably not meant to work this way.

You can't normally just add new fields like this, you would first need to add columns to the table that this logs to. The available fields are here:

* firehoseClient.putRecord(
* {
* study: 'underwater basket weaving', // REQUIRED
* study_group: 'control', // OPTIONAL
* event: 'drowning', // REQUIRED
* data_int: 2 // OPTIONAL
* data_float: 0.31 // OPTIONAL
* data_string: 'code.org rocks' // OPTIONAL
* data_json: JSON.stringify(x) // OPTIONAL
* }
* );

However user_id actually exists already, but it's not meant to be specified in the data argument to the putRecord call. It's a field that is automatically added to the firehose record if you include includeUserId: true in the options argument to putRecord:

putRecord(data, options = {alwaysPut: false, includeUserId: false, callback: null}) {

Unfortunately, you then run into the problem that the pageConstants reducer isn't used on non-puzzle pages, so the firehose code can't actually look up the user_id when you're in the project gallery:

const constants = state.pageConstants;
if (constants) {
data['user_id'] = constants.userId;
}

The way that addCommonValues is written though, your code should actually just work. It won't touch your manually specified user_id, and firehose/redshift won't know the difference between this and a normal automatically populated user_id.

I think you have two options:

  1. Add the pageConstants reducer to the redux store in the project gallery, and populate the user_id so that the includeUserId option works as intended.
  2. Update the usage comment for putRecord to officially allow a user_id property (but also mention the includeUserId option in the comment), and update the code above so that it doesn't override a manually specified user_id with the one from redux. You probably want to do this.

study_group: 'publish-button',
event: 'unpublish',
user_id: this.props.userId,
channel_id: this.props.projectId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For new fields other than user_id, I've just used the data_json field, so something like:

data_json: JSON.stringify({ channel_id: this.props.projectId }),

study: 'project-publish',
study_group: 'publish-button',
event: 'publish',
user_id: this.props.userId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2.) I think that you can pass the alwaysPut option to putRecord to force it to log to firehose in development mode, but you'd still have to ask Poorva to check redshift to see if the record was added (or set up your own access to redshift). If my memory serves, you also have to wait a day or so for the record to actually show up. Turns out it shows up in Redshift within minutes.

I usually don't bother with that. In development mode putRecord logs to the JS console instead of to firehose, I just make sure it does that.

@Erin007 Erin007 changed the base branch from project-experiment-logging to staging August 16, 2018 02:59
@Erin007
Copy link
Contributor Author

Erin007 commented Aug 22, 2018

@balderdash I added in explanatory comments regarding user_id, put channel_id in the data blob and updated the description to include confirmation that the expected log entry is being outputted. Anything else you recommend?

Copy link
Contributor

@balderdash balderdash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@Erin007 Erin007 merged commit 8a0849b into staging Aug 22, 2018
@balderdash balderdash deleted the project-experiment-log-to-firehose branch September 20, 2018 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants