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

Adds tracking lib skeleton for Block Editor #34655

Merged
merged 1 commit into from
Jul 19, 2019

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Jul 15, 2019

At the moment the Block Editor within WPCom is something of a black hole for events and stats. This PR aims to provide a basic skeleton for tracking events based on event delegation and DOM interrogation.

This is as discussed in paYJgx-5g-p2.

Changes proposed in this Pull Request

  • Add centralized "library" tracking.js to enable easy addition of events to be tracked.
  • Removes bespoke tracking-search.js and moves tracking into main tracking.js lib.
  • Tracks a set of events (described below)
  • Passes Blog ID with tracking

Set of events to track

  • gutenberg_headerbar_selected_back / wpcom_block_editor_close_click
  • gutenberg_headerbar_performed_undo / wpcom_block_editor_undo_performed
  • gutenberg_headerbar_performed_redo / wpcom_block_editor_redo_performed
  • gutenberg_blockpicker_selected_block / wpcom_block_inserted
  • gutenberg_blockoptions_deleted_block / wpcom_block_deleted
  • gutenberg_blockarrangement_moved_block_up / wpcom_block_moved_up
  • gutenberg_blockarrangement_moved_block_down / wpcom_block_moved_down
  • gutenberg_blockarrangement_dragged_block / wpcom_block_moved_via_dragging

How to test

Follow the instructions in PCYsg-l4k-p2 to pull the changes in this PR onto your sandbox.
Sandbox widgets.wp.com and a test site, and go to create a new post.
In your devtool's console, run the command localStorage.debug = '*'.
Reload the editor.
If you're testing from wordpress.com, set the debug scope in your devtool's console to your test site (default is "top").
Start inserting and moving blocks.
Then, you will be able to see logs as the following image shows:

image

Some notes: When the event to track is detected via event delegation, it shows the triggering "<event-type>" in the logline. If it's via redux, shows action "<action-name>" (light orange squares).

Questions

  • Should we throttle the events so repeated clicks/resize/keyboard events don't fire tracks events too often? A good case in point is the block picker search event which fires on each key stroke. That's a tonne of events!
  • How best to add the blog_id property?

Development

To work with this code you will need to build the app (tip: use --watch to run on change) as per instructions into your sandbox:

You should set the output directory to target the widgets.wp.com/wpcom-block-editor directory which is part of the WPCom codebase. You then need to sandbox widgets.wp.com which is where the code is served from.

Fixes: #34656.

@getdave getdave requested a review from a team July 15, 2019 15:55
@getdave getdave self-assigned this Jul 15, 2019
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@getdave
Copy link
Contributor Author

getdave commented Jul 17, 2019

@ockham If you did want to take a closer look at this PR, now would be a good point. We'd value any comments or feedback you felt able to provide.

@getdave
Copy link
Contributor Author

getdave commented Jul 17, 2019

@retrofox Failing test may require a rebase against master.

@retrofox
Copy link
Contributor

@retrofox Failing test may require a rebase against master.

Go for it whenever you want.

@retrofox retrofox force-pushed the add/tracking-to-wpcom-block-editor branch 2 times, most recently from 23de87d to 017c67a Compare July 17, 2019 22:49
blocks_replaced: false,
} );
} );
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Block insertion is the event we're most interested in for the purpose of our UpgradeNudge, specifically to build funnels from block insertion to clicking the 'Upgrade' CTA (for which we already have an event, jetpack_editor_block_upgrade_click, see Automattic/jetpack#12965).

Since we made the UpgradeNudge for Gutenberg blocks whose code lives in the Jetpack repo, and ship both with Jetpack and WP.com, our click event is prefixed with jetpack_. This sheds light on an interesting question: Even though we have activated the UpgradeNudge only on WP.com for now, we plan to eventually also activate it on Jetpack.

In that case, we'd obviously also like to build the same funnel, starting at block insertion! This means it'd be utterly useful to us if this event was also available inside Jetpack.

This can mean various things:

  • Ideally, this code will eventually be available to both WP.com and Jetpack (npm etc, I'll elaborate later). But that's not even the most pressing question, since we can always move it around later.
  • Naming is really important here! IMO, it'd kind of suck to have to build a funnel
    • from wpcom_block_picker_block_inserted to jetpack_editor_block_upgrade_click on WP.com, and a different one
    • from jetpack_block_picker_block_inserted to jetpack_editor_block_upgrade_click on Jetpack
      (Remember that the 'bottom-of-the-funnel' event is the same (prefixed with jetpack_) in both cases!)

I think this would make analyzing our data considerably harder, and working with tracking much more confusing for anyone wanting to add events.

I think we have a number of options:

  1. You folks stick with wpcom_block_picker_block_inserted (and we also call it that once we move it to Jetpack), we stick with jetpack_editor_block_upgrade_click. Clearly the most awkward, since funnels will be from wpcom_ to jetpack_ in both environments 🤦‍♂️ Super un-intuitive 🚫
  2. You folks change yours to jetpack_block_picker_block_inserted, both on WP.com, and on Jetpack (and we stick with ours). Means consistency, but obviously awkward on WP.com. However, not more awkward than what we're doing with jetpack_editor_block_upgrade_click. For us, it was fairly easy to rationalize this decision, since after all, WP.com is Powered by JP, and more concretely, the same set of Gutenberg Blocks coming from Jetpack. Not sure you folks can get behind the same kind of reasoning 🙂
  3. We make the prefixes dynamic:
  • wpcom_block_picker_block_inserted and wpcom_editor_block_upgrade_click on WP.com
  • jetpack_block_picker_block_inserted to jetpack_editor_block_upgrade_click on Jetpack.

And to be clear, wpcom_block_picker_block_inserted just happens to be the one event at hand that we already know we're going to need, but I'm not suggesting special treatment for that event -- whatever option we choose, we need to apply it to all events we're adding to the block editor, or it'll be one giant mess with seemingly arbitrary prefixes.

So IMO, we should pick either option 2. or 3. I think it's easy to lean towards 3. (as I do, even though it means we have to change our event name post-launch 🙂 ), but we should really inform that decision by what is best for data analysis in the long run.

/cc @AnnaMag @sirreal @apeatling

Copy link
Member

Choose a reason for hiding this comment

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

From PCYsg-4S2-p2 (recommended reading):

Don’t programmatically create event names. They should be hardcoded so they are easily searchable in the code base.

I'd propose that events generated by the same code under the same circumstances are the same event. Whether they happen in WP Admin on a Simple, Atomic, or Jetpack site, they represent a user doing the same thing.

I'd propose that we use event properties to distinguish the type of site it's coming from. The tracking library may want to include a discriminating property with all events as part of its core functionality: { "site_type": "simple" | "atomic" | "jetpack" }.

What should the source (the first section of the event name) be for these events? I'm not sure. According to PCYsg-4S2-p2:

Prefix event names with the codebase or service generating the events […]

We don't have a single unified codebase. We'll need to decide on a tradeoff between knowing which codebase generates an event and folks working with funnels knowing that (to be determined)_ tracks block insertion, while upgrade nudge clicks come from jetpack_.

Getting a new source whitelisted doesn't seem to be a problem.

Copy link
Contributor

@marekhrabe marekhrabe Jul 18, 2019

Choose a reason for hiding this comment

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

Thanks for your feedback on this - naming things is hard in the dev world. We already had long discussions with Ajax and with the team behind Tracks. We initially wanted to have a new context whitelisted (gutenberg_) which was not approved.

wpcom was a suggestion to go with for reasons where this code runs today and it matched the precedent there is in the codebase, which is the block search tracking.

The current plan is to go with wpcom as a source. If the tracking code ever makes it to other platforms, it will also use the exact same event name.

We are already sending through the blog id which can be used to determine the platform (wpcom/jetpack/atomic) as suggested by the Tracks team. I'm not sure if the tools you use for analysis allow you to simply filter platforms based on the blog id. If not, I think adding an additional property with the platform name is a good idea.

I'd propose that we use event properties to distinguish the type of site it's coming from. The tracking library may want to include a discriminating property with all events as part of its core functionality: { "site_type": "simple" | "atomic" | "jetpack" }.

This would be perfect I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

@retrofox has added site_type prop to our events with the value of get_site_type( $current_site_id ). At this moment "simple" is the only possible value.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that everything in the common folder will be served off of widgets.wp.com and is available for all site types (Simple, Jetpack, Atomic) in wp-admin + WordPress.com/block-editor

Copy link
Contributor

Choose a reason for hiding this comment

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

Verified that I see tracking events in Simple and Jetpack with the patch applied. Caches are invalidated daily, but this will be served to all site types. See:

https://github.com/Automattic/jetpack/blob/bb9aaeb930ada902d698060f5f6e45b689df3d55/modules/wpcom-block-editor/class-jetpack-wpcom-block-editor.php#L259

Copy link
Contributor

@gwwar gwwar Jul 18, 2019

Choose a reason for hiding this comment

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

In the long run I think it may make more sense to

  1. Pull out the analytics npm package as part of the Calypso monorepo
  2. Remove direct Block-Editor tracking calls in Jetpack
  3. Setup an analytics custom store that can listen for redux events. Components can fire a redux event, and redux middleware can make the actual tracking call.
  4. In terms of tracks namespace, let's pick something consistent, it doesn't really matter where the tracking code lives. +1 on Jon's suggestion to provide a site type source + blog id and for searchable names.

cc @stambizzle in case you had more suggestions on names / best practices.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore most of this last comment :D I wrote this when I wasn't far down enough in the PR 👍

},
};

// Intercept dispatch function and add tracking for actions that need it.
Copy link
Contributor

Choose a reason for hiding this comment

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

In the long run, it would be nice to encapsulate this tracking logic into an npm package, so it can also be used by other consumers, e.g. Jetpack, and eventually maybe even graduate to become a @wordpress/tracking package, since it seems to be pretty close to what @youknowriad was suggesting, if I'm not mistaken.

(To be clear, such a package should only contain the logic allowing to add tracking to certain Redux actions, but not any actual Tracks/Google Analytics/etc tracking libs.)

We have some precedent of such a process: @sirreal's @automattic/wordpress-external-dependencies-plugin npm started as a package in this repo's packages/ dir, before graduating to become @wordpress/dependency-extraction-webpack-plugin (living in the Gutenberg repo). (We're currently in the process of deprecating the @automattic/ version, #34122.)

Let us know if we can help with that. To re-iterate, we'd love to have this functionality available in Jetpack so we can track block insertion there ❤️

@marekhrabe marekhrabe marked this pull request as ready for review July 18, 2019 16:12
@marekhrabe marekhrabe requested review from a team as code owners July 18, 2019 16:12
@retrofox retrofox force-pushed the add/tracking-to-wpcom-block-editor branch from e9360af to ae55897 Compare July 18, 2019 17:48
@gwwar gwwar added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 18, 2019
@gwwar
Copy link
Contributor

gwwar commented Jul 18, 2019

Don't forget to add "Needs Review" label to PRs. I believe certain Circle Workflows won't be triggered otherwise.

const tracksRecordEvent = function( eventName, eventProperties ) {

export default ( eventName, eventProperties ) => {
if ( undefined === window || undefined === window._currentSiteId ) {
Copy link
Member

@obenland obenland Jul 18, 2019

Choose a reason for hiding this comment

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

We should test window._currentSiteType here, too. See p1563482008221900-slack-ajax.

@gwwar
Copy link
Contributor

gwwar commented Jul 18, 2019

Should we throttle the events so repeated clicks/resize/keyboard events don't fire tracks events too often?

Since we're listening to events, we can debounce and choose when to fire a track event. For example only triggering a search even after a user has stopped typing. (eg debounce the keydown event) For other redux actions we can do something similar if it triggers very often.

Here's a pic of a debounce example, since I always get this mixed up with throttle.

Screen Shot 2019-07-18 at 4 39 47 PM

https://css-tricks.com/debouncing-throttling-explained-examples/#article-header-id-0

How best to add the blog_id property?

The current approach of adding information to the global window object is okay, noting for future naming preferences to avoid the _ prefix. The original window._currentSiteId was a used to map public-api API endpoints in the first Gutenberg integration (by Team Tinker)

Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

Nice work! I like that we have the ability to listen to events, but still have a handler to fire off a tracks event for our own needs.

For folks reviewing, events are expected to fire for Simple / Atomic, only. Standalone Jetpack sites won't dispatch these events since siteId/siteType are not available. Support can be added in the future if needed.

I verified tracking behavior on Simple/Atomic:

atomic

simple

Before :shipit: remember to:

  • Update the phab diff on wpcom
  • Land the atomic diff
  • Merge this PR and don't forget to deploy.

Caches for the common file clear daily (#), so we should see this live once the wpcom patch lands.

If y'all make minor adjustments to code, no need to ask for re-approval, but please smoke test your changes.

@retrofox retrofox force-pushed the add/tracking-to-wpcom-block-editor branch 2 times, most recently from 26fd58f to ab8478e Compare July 19, 2019 14:54
@retrofox retrofox force-pushed the add/tracking-to-wpcom-block-editor branch from ab8478e to 7042a2d Compare July 19, 2019 15:05
Copy link
Contributor

@marekhrabe marekhrabe left a comment

Choose a reason for hiding this comment

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

Newest feedback is addressed. Let's get this in and iterate on the feature in separate PRs.

@retrofox retrofox merged commit 6e4f01d into master Jul 19, 2019
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 19, 2019
@marekhrabe marekhrabe deleted the add/tracking-to-wpcom-block-editor branch July 19, 2019 15:48
@gwwar
Copy link
Contributor

gwwar commented Jul 19, 2019

Adding a follow up note for future iterations:

cc @Automattic/ajax

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.

FSE: Create mini-little tracking library in wp-com-block-editor
9 participants