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

Instant Search: Add TrainTracks analytics #13730

Merged
merged 8 commits into from
Oct 15, 2019

Conversation

jsnmoon
Copy link
Contributor

@jsnmoon jsnmoon commented Oct 11, 2019

Changes proposed in this Pull Request:

This adds TrainTracks analytics calls to Instant Search results.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Yes, it adds analytics calls for rendering and interacting with Instant Search results.

Testing instructions:

  • Add define( "JETPACK_SEARCH_PROTOTYPE", true ); to your wp-config.php.
  • Ensure that your site has the Jetpack Pro plan and Jetpack Search enabled.
  • Add a Jetpack Search widget to the Search page sidebar.
  • Open your devtools network inspector and select a checkbox labeled Preserve Log.
  • Enter a query into the site's search widget.
  • Filter for stats.wp requests in the network inspector and ensure that you see a request for stats.wp.com/w.js?60.
  • Filter for t.gif requests in the network inspector and ensure that you see 10 calls for t.gif with _en param equaling jetpack_instant_search_traintracks_render.
  • Click on a search result link.
  • Ensure that you see one additional call for t.gif with _en param equaling jetpack_instant_search_traintracks_interact.

Proposed changelog entry for your changes:

None.

@jsnmoon jsnmoon added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Feature] Search For all things related to Search Instant Search labels Oct 11, 2019
@jsnmoon jsnmoon requested review from gibrown and a team October 11, 2019 20:19
@jsnmoon jsnmoon self-assigned this Oct 11, 2019
@jsnmoon jsnmoon requested a review from a team October 11, 2019 20:20
Copy link
Member

@gibrown gibrown left a comment

Choose a reason for hiding this comment

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

The jetpack_instant_search_traintracks_render events seem to have anon ids and the jetpack_instant_search_traintracks_interact events are picking up my wp.com id. I can't actually think of a reason why we need real ids associated with these events. I'm not even sure we need to fully persist the anon ids across page loads in a cookie but I think doing so would be good at the moment. But I don't want to do it if it means tracking folks across different sites.

I think we should remove all the parts that try to grab real user ids and entirely use anon ids since this is going to be fully on the front end of the sites. @xyu and @bperson can probably help point the way for this.

@bperson
Copy link

bperson commented Oct 14, 2019

I can't actually think of a reason why we need real ids associated with these events. I'm not even sure we need to fully persist the anon ids across page loads in a cookie but I think doing so would be good at the moment. But I don't want to do it if it means tracking folks across different sites.

Considering we are analysing visitors of jetpack sites ( and not just owners of jetpack sites ), we should be extremely careful as this is new territory for Tracks in Jetpack AFAIK.

Did we ever do "search sessions" analysis? By that I mean not only tweaking the search algorithms to improve the results for a specific search query, but tweaking the results so that the user gets what he searches for without having to re-tweak its search query to be more specific.

If we don't think of doing analysis across different search queries, I don't think there is any value in using a persisting ID, which means we MUST NOT do it: generate a random ID, store it in a variable or a global that will get discarded if the user leaves or reloads the page.

If we hope to do those analysis in a far fetch future, we should also NOT DO it and only persist those IDs once we are sure we'll actually use that data.

modules/search/instant-search/lib/tracks.js Outdated Show resolved Hide resolved
modules/search/class.jetpack-search.php Outdated Show resolved Hide resolved
modules/search/instant-search/lib/tracks.js Outdated Show resolved Hide resolved
@gibrown
Copy link
Member

gibrown commented Oct 14, 2019

Did we ever do "search sessions" analysis?

I want to do this yes. But... a session could also be considered just all the interactions on the front end up until the page gets reloaded. So if we generate a single id each time the search app gets loaded, then that should be sufficient to at least start doing some session analysis.

The only case I can think of where storing something may be good is for A/B testing and wanting to make sure we provide the end user with the same side of the A/B test after they get it once. I'm not sure that matters a lot though.

So ya, let's not store anything in any cookies to start off with.

@jsnmoon
Copy link
Contributor Author

jsnmoon commented Oct 14, 2019

@bperson @gibrown: I went ahead and removed parts of the code handling user identification. It now uses the default behavior from w.js.

@jsnmoon jsnmoon dismissed gibrown’s stale review October 14, 2019 19:34

User identification code removed.

@jsnmoon jsnmoon force-pushed the add/instant-search-tracking branch from 558ca43 to aadb531 Compare October 14, 2019 19:35
@jsnmoon jsnmoon requested review from bperson and gibrown October 14, 2019 19:36
@jsnmoon jsnmoon force-pushed the add/instant-search-tracking branch from 7554497 to a0c6a97 Compare October 14, 2019 22:04
@jsnmoon jsnmoon force-pushed the add/instant-search-tracking branch from a0c6a97 to 4054712 Compare October 15, 2019 19:14
@jsnmoon
Copy link
Contributor Author

jsnmoon commented Oct 15, 2019

I believe I've addressed all outstanding concerns, so I'll go ahead and merge this PR in an hour or so.

@jsnmoon jsnmoon merged commit 05bf313 into instant-search-master Oct 15, 2019
@jsnmoon jsnmoon deleted the add/instant-search-tracking branch October 15, 2019 20:25
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 15, 2019
jsnmoon added a commit that referenced this pull request Oct 23, 2019
* Implement minimal search results and spelling correction (#13365)
* Add filtering display (#13371)
* Fix search result display bugs and make improvements (#13393)
* Add rudimentary support for filtering on post types (#13430)
* Add support for filtering on categories and tags (#13505)
* Add instant search sorting based on the URL (#13377)
* Add support for filtering on dates (#13545)
* Add custom taxonomy filtering (#13605)
* add sort widget (#13614)
* fix many theme incompatibilities (#13602)
* Add infinite scrolling (#13684)
* Add caching to the api requests (#13714)
* Clean up some design bugs/issues (#13721)
* Fix labels for post types when we have them. (#13750)
* Add localization and formatting of all dates (#13748)
* search from any page on the site (#13713)
* Hook up default options (inc. sort) (#13742)
* Add TrainTracks analytics (#13730)
* Create PostTypeIcon component (#13790)
* Upgrade to Preact 10 (#13794)
* Add comments component (#13797)
* Address review feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Search For all things related to Search Touches WP.com Files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants