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

Add source parameter to event logging #2700

Merged
merged 2 commits into from May 29, 2019
Merged

Conversation

alexsanford
Copy link
Contributor

As per our Slack conversation, this PR aims to provide a source parameter to all logged events that we can later use for querying.

Currently, these sources are logged:

  • wp-admin (includes all AJAX requests)
  • rest-api (note that events fired through direct interaction with the block editor may have this source)
  • frontend
  • data-import
  • js-event (for JS calls to sensei_log_event)

Testing instructions

wp-admin

Create a module from WP Admin. Use both the Module page, and the Course page metabox. Ensure the source for the events is wp-admin.

rest-api

Add the following code snippet:

add_action( 'rest_api_init', function() {
	sensei_log_event( 'rest_api_request' );
} );

Perform a REST API request (e.g. http://my.site.com/wp-json/wp/v2/courses). Ensure the source for the sensei_rest_api_request event is rest-api.

frontend

Add a call to sensei_log_event to a template and then load that template (e.g. add sensei_log_event( 'rendering_course' ); to single-course.php and then view a course). Ensure the source is frontend.

data-import

Import data with modules (here is a data file you can use #). Ensure the sensei_module_add event is tracked twice (once for each module) and that the source is data-import.

js-event

View the Sensei settings page. Ensure that the event logged has source js-event.

@alexsanford alexsanford added this to the 2.1.0 milestone May 24, 2019
@alexsanford alexsanford self-assigned this May 24, 2019
Copy link
Collaborator

@donnapep donnapep left a comment

Choose a reason for hiding this comment

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

Tested and works great! 👍

@jom
Copy link
Member

jom commented May 24, 2019

Out of curiosity I cherry-picked these commits on top of #2687. One thing is for CPT related events, events from the block editor will trigger source='rest-api' but classic editor will trigger source='wp-admin'. Not sure this is an issue, just something worth noting for querying.

Screen Shot 2019-05-24 at 7 46 27 PM

@alexsanford
Copy link
Contributor Author

One thing is for CPT related events, events from the block editor will trigger source='rest-api' but classic editor will trigger source='wp-admin'.

Yes, I thought this would probably crop up. @donnapep is this going to be an issue for querying?

One option, if it makes it easier, would be to add another source for block-editor. So then for queries we could filter on wp-admin and block-editor without getting generic REST API requests. I'm not sure if this is enough of an issue though?

@donnapep
Copy link
Collaborator

One option, if it makes it easier, would be to add another source for block-editor. So then for queries we could filter on wp-admin and block-editor without getting generic REST API requests. I'm not sure if this is enough of an issue though?

That sounds good, although it would be good to timebox the effort in case it gets complicated. If it does, then I wouldn't worry about it for now.

@alexsanford
Copy link
Contributor Author

That sounds good, although it would be good to timebox the effort in case it gets complicated. If it does, then I wouldn't worry about it for now.

Ok, this is going to be trickier than I thought. I guess I assumed that there would be some simple way for us to detect whether the REST API request is coming from the block editor, but after digging into the requests, they are almost indistinguishable from any other REST API request.

If this is an important distinction for us to make in the source property, then there are two options I can see:

  1. We could use the apiFetch middleware API to add a special header to all of the REST API requests from the block editor page. Then we can use that to inform our source attribute on the server side. This is less hacky than option 2 below, but I'm not sure that adding an extra header to all those requests is a good solution.

  2. We do have a "Referer" header that gives us the URL of the page from which the REST API request originated. But I think that trying to accurately detect whether the referer URL represents a block editor page is going to get hacky.

Given that, I'm thinking that the "I wouldn't worry about it for now" suggestion sounds like the way to go 🙂

That said, option 1 above would likely not take long to implement if we wanted to see a proof-of-concept.

@donnapep
Copy link
Collaborator

I'm down with leaving it as-is. I don't think it's a big enough deal to warrant the extra work.

@alexsanford alexsanford merged commit f08000c into master May 29, 2019
@alexsanford alexsanford deleted the add/event-logging-source branch May 29, 2019 15:59
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

3 participants