Gutenboarding: Add TrainTracks events for domain picker #41173

andrewserong · 2020-04-16T08:07:59Z

This PR adds in TrainTracks events in the domain picker in Gutenboarding. It should fire off events when the search results are rendered in the domain picker, when the search results are updated, and an interact event when a user selects one of the domain suggestions.

More info: PCYsg-bor-p2

Changes proposed in this Pull Request

Add in TrainTracks events for render and interact in the domain picker following PCYsg-bor-p2 and referencing the logic in the existing domain step in the legacy signup flow.
I've made some assumptions about values to send in the render event, trying to keep it as close as possible to the existing behaviour in the domain step, but swapping out signup for gutenboarding in the vendor name.
Install @types/uuid

Testing instructions

Go to /new and open up the Domain Picker and examine requests in the network inspector in devtools
When you change the search query, analytics should fire for each search result
Check that the railcarId matches the pattern uuid-suggestion0 where the final number is incremented based on its position in the search results — this allows the interact event to map to the render event.

E.g. in the following screenshot, the highlighted event is domain_selected and has the same railcar id as the rendered suggestion

Questions:

Does this cover what we need? Are there additional things (or events) we need to add?
Is the behaviour here implemented in the way TrainTracks is meant to work? (I've done it slightly differently to how the existing domains step works, because it looks like the existing domains step doesn't use an iterator in the railcar ID which means the interact events won't map to the particular domain selected — here I've made sure it maps correctly, but is this correct?)
Do the TrainTracks events fire too frequently, too infrequently, or are they just right? 🐻

Part of #41059

matticbot · 2020-04-16T08:08:17Z

Test live: https://calypso.live/?branch=add/train-tracks-tracking-to-domain-popover-in-Gutenboarding

matticbot · 2020-04-16T08:08:55Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~813 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-gutenboarding      +2375 B  (+0.1%)     +813 B  (+0.1%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

ramonjd · 2020-04-16T08:37:38Z

client/landing/gutenboarding/components/domain-picker/suggestion-item.tsx

@@ -28,13 +40,47 @@ const DomainPickerSuggestionItem: FunctionComponent< Props > = ( {
 	const domainName = domain.slice( 0, dotPos );
 	const domainTld = domain.slice( dotPos );

+	const { domainSearch } = useSelect( select => select( STORE_KEY ).getState() );
+	const fetchAlgo = `/domains/search/${ DOMAIN_SUGGESTION_VENDOR }/${ FLOW_ID }`;


Just checking: this is enough to differentiate a gutenboarding domain search, right? (As opposed to a standard Calypso signup one).

That is, we won't need to send any additional flow info?

From re-reading PCYsg-bor-p2 I think we can pass in another param ui_algo to denote where in the flow the search results appear. So something like /gutenboarding/domain-popover might be a good identifier?

ramonjd · 2020-04-16T08:51:35Z

Is the behaviour here implemented in the way TrainTracks is meant to work?

Hi @hambai I pulled your name out of #38886 :)

Would you happen to know if we've implemented TrainTracks for domain searching correctly here? Thanks!

simison · 2020-04-16T08:57:49Z

Hi @daledupreez @klimeryk ! This PR is just the first pass of adding analytics to domain dropdown in new onboarding. We would love your insights from tracking the domains in general. Thanks!

simison · 2020-04-16T09:01:52Z

client/landing/gutenboarding/components/domain-picker/suggestion-item.tsx

+				query: domainSearch,
+				railcarId,
+				result: isRecommended ? domain + '#recommended' : domain,
+				uiPosition,


This is the domain's position on the list — to me, "ui position" sounds a little bit like the position of the whole suggestion list.

We'll have the same list appear in few places, in immediate future we'll have the domain dropdown (what we have now in master) and "more domains modal" (#41081). Possibly some additional domain steps and other places as well. We should likely include that info in tracking event?

Absolutely. I think if we add another param listed in PCYsg-bor-p2 ui_algo we can use that to denote the unique place where the list appears. E.g. we could have:

/gutenboarding/domain-popover

/gutenboarding/domain-expanded

/gutenboarding/domain-step

Etc.

ramonjd · 2020-04-16T09:21:56Z

client/landing/gutenboarding/components/domain-picker/suggestion-item.tsx

+
+	useEffect( () => {
+		// Only record TrainTracks render event when the domain name changes
+		if ( domain !== previousDomain || previousRailcarId !== railcarId ) {


Do the TrainTracks events fire too frequently, too infrequently, or are they just right?

I don't yet know what's causing it, or even if it's a problem, but it seems strange that making one request to v1.1/domains/suggestions (which yields five results) triggers 10 calypso_traintracks_render calls.

The first five 10 calypso_traintracks_render calls are for the default suggested domains, the next five are the query results. That makes sense.

But when I clear the domain search field, it triggers another 10 calypso_traintracks_render calls. This time the first five 10 calypso_traintracks_render calls are for the previous query results, the next five are the default suggested domains.

Seems like it's too many when I compare it with https://wordpress.com/start/domains

Thanks for testing! Yes, that sounds like too many. From what I've been looking at so far, for each rendered suggestion item we should have 1 network request. So the desired behaviour (I think!) is that once the debounce finishes and we have a fresh set of suggestions, we call calypso_traintracks_render, but then don't render again unless the results are fresh. I'll have another go.

ramonjd · 2020-04-16T09:27:32Z

The properties/values being sent look good to me.

The recommended domain gets a #recommended.

The vendor variation is passed to /domains/suggestions

Just a couple of questions, mainly around the render frequency

ockham · 2020-04-16T11:08:19Z

High-level question, is there any chance we can keep the actual train tracks logic out of the domain picker component?

@sirreal and I were at some point hoping to keep the domain picker generic and reusable, and eventually maybe even extract it into a "smart components" package (components that are connected to a data store) for better separation of concerns, and re-usability. To that end, it'd be great if we didn't embed analyitics stuff deeply in it.

I haven't deeply checked this diff, but would it be a lot of hassle to add some rather generic event handler-style props to the domain picker, and use those to pass in the railcar logic?

simison · 2020-04-16T11:11:01Z

@ockham I don't think there is an instance where we'd want it to shoot different events, or no events at all?

ockham · 2020-04-16T11:32:26Z

@ockham I don't think there is an instance where we'd want it to shoot different events, or no events at all?

Fair point. We normally have at least some amount of contextual information in our tracking (e.g. event names reflecting the area of Calypso an event is happening; here's an example for the present PR), but maybe it'd be enough to make those things (event names, or even just prefixes/infixes) into props.

I'd just rather avoid too-domain specific analytics get in the way of component reusability. The latter is IMO a problem that we've seen a lot in Calypso, and it'd be great if we could avoid it this time around.

ockham · 2020-04-16T11:37:48Z

Side note, the fact that TrainTracks expects an event upon render is... interesting (see PCYsg-bor-p2):

<source>_traintracks_render — A recommendation or search result is shown to the user.

That doesn't sound like something that's particularly easy to pull off in React; in order to avoid duplication, there needs to be component lifecycle logic in place that compares before/after props, and that's notoriously easy to miss when e.g. adding new props, making the whole system rather fragile. I wonder how reliable our TrainTracks data obtained from across Calypso is.

It might be better to replace render-based events firing by user input-based firing (changed search term etc), but that's an architectural question for TrainTracks.

Edit: Tentatively cc'ing @sirino

simison · 2020-04-16T11:42:41Z

We normally have at least some amount of contextual information in our tracking (e.g. event names reflecting the area of Calypso an event is happening

I agree that would be important — tho event names themself cannot be built from variables (so that it's possible to find them via code search), but additional event props would be possible and could be made required.

sirino · 2020-04-16T12:36:39Z

Tentatively cc'ing @sirino

@jsnmoon can you help with this? I think you have more experience here than I do!

sirreal · 2020-04-16T14:38:42Z

I've rebased this to fix the package-lock conflicts.

jsnmoon · 2020-04-16T21:50:28Z

That doesn't sound like something that's particularly easy to pull off in React; in order to avoid duplication, there needs to be component lifecycle logic in place that compares before/after props, and that's notoriously easy to miss when e.g. adding new props, making the whole system rather fragile. (@ockham)

This is spot-on. It's definitely possible to do this correctly, but it's much easier to get it completely wrong unless you're carefully examining your components' lifecycles. I'm not sure how to make it any easier at the moment, unfortunately.

It might be better to replace render-based events firing by user input-based firing (changed search term etc), but that's an architectural question for TrainTracks. (@ockham)

How might this work? Perhaps we could fire an event when we receive the search results instead of when we render the search results? I suppose we still have to contend with how we're going to relay values like ui_algo and ui_position.

andrewserong · 2020-04-17T06:10:45Z

Thanks for the great feedback and suggestions, everyone!

Today I've:

Added in a ui_algo prop to denote the particular flow / area in the UI where the suggestions are displayed using the pattern /gutenboarding/domain-popover — hopefully this should be enough to isolate the TrainTracks events particular to Gutenboarding, and then the area within Gutenboarding (allowing for a separate /gutenboarding/domain-step)
Updated the logic for when a railcarId is created and when the render event should fire within the SuggestionItem — it should now only fire a render event once for each set of search results (e.g. 5 render events if there are 5 search results) and the select / interaction event uses the previous railcarId set during the last render
I've made an attempt at generalising the analytics calls by passing down a recordAnalytics function via props. This hasn't been as successful as I'd hoped, as all the railcar logic is still shared between the DomainPicker and the SuggestionItem, and I'm not sure if this adds more indirection, which might make for a different issue in maintaining it. E.g. is it clear enough where to add in a new uiAlgo if someone's re-using the DomainPicker in a different step?

I'm on maintenance rotation for the next two weeks so likely won't have enough time to progress on this myself. I'll unassign myself from the PR, so feel free to pick this up anyone who would like to (@Automattic/ganon). Some next steps might include:

Look into the types used here — I rushed at the end today, so recordTrainTracksEvent is set to any for the payload prop, but this should probably be a union type (?) between TrainTracksRenderProps and TrainTracksInteractProps
Can we add in a unit test for the SuggestionItem component to verify that it only fires render events when we expect it to? Since we have concerns about the stability and reliability of firing these events, it'd be great if we can come up with a way to ensure they're tested. Even if it's just testing that the passed down recordAnalytics function is called the correct number of times.
Test in Tracks that the events are coming through correctly and can be analysed. @jsnmoon — I ran into some difficulty trying to find these events in Tracks / Hue today, so thought you might have a better idea on how we can analyse these results and then verify if we're firing the events correctly for whatever analysis we'd like to do in the future. (This discussion might be better suited to: pbAok1-Ao-p2)

I think that's all for now, I'll keep an eye on this PR where I can though, so please let me know if there's anything else I've missed!

yansern · 2020-04-17T13:20:06Z

If my PR gets merged first, I'll help rebase this PR. Let the race begins! #41212

yansern · 2020-04-17T14:48:00Z

Rebased with changes from #41212!

…cks events

The railcarId is now only updated when the domain suggestions change (and are not empty), and the domain suggestion itself has changed.

…ract away train tracks calls

Considered using function overloading so `recordTrainTracksEvent` would take a different payload depending on the first argument, but I think it's considered best practice not to use overloading in new code.

p-jackson · 2020-04-20T23:58:40Z

Added types to recordTrainTracksEvent
Added unit tests to check render events don't get sent when props change (tests can only do so much, e.g. we might add new props that aren't being tested)
Tested to see events are visible in a Funnel
Rebased to fix failing build

@andrewserong do you have time to quickly double check my work? I haven't had to really change the code much except for small things to make the typings work.

The calypso_traintracks_render event is being flagged as "neutral" instead of "valid" in the funnel, but I'll p2 that and we can follow up with any necessary changes.

andrewserong · 2020-04-21T01:00:23Z

Thanks so much for all this @p-jackson, the types and tests are looking great, and even though things will likely change in the future, this feels like a good level of test coverage so far and will be a useful point of reference for anyone making changes to this component in the future.

Manual testing still shows the events are being fired as I'd expect them to. Good idea P2ing the event firing behaviour. From my searches, it looks like the traintracks events are being hidden from live view, etc, but it sounds like they should still be queryable. I think we'll need some guidance on the best way to extract the data, but for now, I think it looks like everything is set up correctly according to the documentation.

The only change I think we still need to make is to move the analytics into lib/analytics instead of utils/analytics to be consistent with #41129, but otherwise this PR's looking good to me (I can't approve a PR I started, but once the analytics are moved it otherwise looks good to me!)

ramonjd

I've retested and can approve :)

Subject to rebase

p-jackson · 2020-04-21T01:06:58Z

I'm planning to move functions and delete utils/analytics.ts in a follow up PR, assuming that works for everyone.

@ramonjd what's the rebase for? My GH UI sais no conflicts.

ramonjd · 2020-04-21T01:09:52Z

what's the rebase for? My GH UI sais no conflicts.

Oh sorry @p-jackson 🤦 "rebase" was the wrong word. Andy said it right:

The only change I think we still need to make is to move the analytics into lib/analytics instead of utils/analytics to be consistent with #41129, but otherwise this PR's looking good to m

We moved analytics into /lib/analytics/index.

So you could copy pasta your stuff over there.

The suggestion was that utils contains functions that don't have any side effects.

Util functions should be side-effect free

p-jackson · 2020-04-21T02:01:19Z

The suggestion was that utils contains functions that don't have any side effects.

Makes sense. I've refactored the functions into lib/analytics

ockham · 2020-04-21T12:22:45Z

Thanks @jsnmoon for your reply, sorry for my delay in getting back to you!

[...]

It might be better to replace render-based events firing by user input-based firing (changed search term etc), but that's an architectural question for TrainTracks. (@ockham)

How might this work? Perhaps we could fire an event when we receive the search results instead of when we render the search results? I suppose we still have to contend with how we're going to relay values like ui_algo and ui_position.

Firing upon receiving the search results might be better if we can do that from Redux/@wordpress/data (instead of React), since we fire a discrete event upon receiving data from the network. (A middleware-based approach comes to mind.)

I'm not very familiar with TrainTracks, so I guess the big question is if we can cleanly pass information such as ui_position and ui_algo to the relevant Redux/wp/data actions. I'm a bit pessimistic, as I think it'd mean we'd require information about how data will be rendered in the UI at the time we only receive that data?

andrewserong requested review from a team as code owners April 16, 2020 08:07

andrewserong self-assigned this Apr 16, 2020

andrewserong requested a review from a team April 16, 2020 08:08

andrewserong added the [Goal] New Onboarding previously called Gutenboarding label Apr 16, 2020

andrewserong added the [Feature] Tracks Metrics & Monitoring Capturing analytics about user behavior on WordPress.com. label Apr 16, 2020

ramonjd mentioned this pull request Apr 16, 2020

[EPIC] Add tracking to events to Gutenboarding #41059

Closed

12 tasks

andrewserong added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 16, 2020

ramonjd reviewed Apr 16, 2020

View reviewed changes

simison requested review from klimeryk and daledupreez April 16, 2020 08:56

simison added the [Feature Group] Emails & Domains Features related to email integrations and domain management. label Apr 16, 2020

simison reviewed Apr 16, 2020

View reviewed changes

ramonjd reviewed Apr 16, 2020

View reviewed changes

sirreal mentioned this pull request Apr 16, 2020

Data-stores: Provide domain suggestions vendor #41177

Merged

sirreal force-pushed the add/train-tracks-tracking-to-domain-popover-in-Gutenboarding branch from 26e5c8f to 7ccaeec Compare April 16, 2020 14:38

andrewserong removed their assignment Apr 17, 2020

yansern force-pushed the add/train-tracks-tracking-to-domain-popover-in-Gutenboarding branch from 7432dcb to e4b34ef Compare April 17, 2020 14:46

p-jackson self-assigned this Apr 19, 2020

p-jackson force-pushed the add/train-tracks-tracking-to-domain-popover-in-Gutenboarding branch from 9f0fbd7 to b48db30 Compare April 20, 2020 03:01

andrewserong and others added 9 commits April 21, 2020 09:54

Add TrainTracks events for domain picker

f907a7a

Add uiAlgo to denote where the domain picker is displayed in TrainTra…

3b98f44

…cks events

Reduce the frequency with which the traintracks render events are fired.

52796c1

The railcarId is now only updated when the domain suggestions change (and are not empty), and the domain suggestion itself has changed.

Make uiAlgo optional

bb139a0

Pass in generic recordAnalytics prop to DomainPicker to (partly) abst…

0b24bd4

…ract away train tracks calls

Make recordAnalytics an optional prop

70aa05c

Tidied types so analytics payload isn't any

31058d1

Considered using function overloading so `recordTrainTracksEvent` would take a different payload depending on the first argument, but I think it's considered best practice not to use overloading in new code.

Tests: Only a domain name and railcar id send render event

cf18c67

Test "interact" railcar event

ac6c90c

p-jackson force-pushed the add/train-tracks-tracking-to-domain-popover-in-Gutenboarding branch from 8c32a75 to ac6c90c Compare April 20, 2020 22:08

Reformatted with new wp-prettier version

1cf6a2b

ramonjd approved these changes Apr 21, 2020

View reviewed changes

ramonjd added the [Status] Needs Rebase label Apr 21, 2020

Rebased analytics utils into libs/analytics

752d18e

Util functions should be side-effect free

p-jackson merged commit f72dc76 into master Apr 21, 2020

p-jackson deleted the add/train-tracks-tracking-to-domain-popover-in-Gutenboarding branch April 21, 2020 02:57

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gutenboarding: Add TrainTracks events for domain picker #41173

Gutenboarding: Add TrainTracks events for domain picker #41173

andrewserong commented Apr 16, 2020 •

edited

Loading

matticbot commented Apr 16, 2020

matticbot commented Apr 16, 2020 •

edited

Loading

ramonjd Apr 16, 2020

andrewserong Apr 17, 2020

ramonjd commented Apr 16, 2020

simison commented Apr 16, 2020

simison Apr 16, 2020

andrewserong Apr 17, 2020

ramonjd Apr 16, 2020

andrewserong Apr 17, 2020

ramonjd commented Apr 16, 2020

ockham commented Apr 16, 2020

simison commented Apr 16, 2020

ockham commented Apr 16, 2020

ockham commented Apr 16, 2020 •

edited

Loading

simison commented Apr 16, 2020 •

edited

Loading

sirino commented Apr 16, 2020 •

edited

Loading

sirreal commented Apr 16, 2020

jsnmoon commented Apr 16, 2020 •

edited

Loading

andrewserong commented Apr 17, 2020

yansern commented Apr 17, 2020

yansern commented Apr 17, 2020

p-jackson commented Apr 20, 2020

andrewserong commented Apr 21, 2020

ramonjd left a comment

p-jackson commented Apr 21, 2020

ramonjd commented Apr 21, 2020

p-jackson commented Apr 21, 2020

ockham commented Apr 21, 2020

Gutenboarding: Add TrainTracks events for domain picker #41173

Gutenboarding: Add TrainTracks events for domain picker #41173

Conversation

andrewserong commented Apr 16, 2020 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Apr 16, 2020

matticbot commented Apr 16, 2020 • edited Loading

ramonjd Apr 16, 2020

Choose a reason for hiding this comment

andrewserong Apr 17, 2020

Choose a reason for hiding this comment

ramonjd commented Apr 16, 2020

simison commented Apr 16, 2020

simison Apr 16, 2020

Choose a reason for hiding this comment

andrewserong Apr 17, 2020

Choose a reason for hiding this comment

ramonjd Apr 16, 2020

Choose a reason for hiding this comment

andrewserong Apr 17, 2020

Choose a reason for hiding this comment

ramonjd commented Apr 16, 2020

ockham commented Apr 16, 2020

simison commented Apr 16, 2020

ockham commented Apr 16, 2020

ockham commented Apr 16, 2020 • edited Loading

simison commented Apr 16, 2020 • edited Loading

sirino commented Apr 16, 2020 • edited Loading

sirreal commented Apr 16, 2020

jsnmoon commented Apr 16, 2020 • edited Loading

andrewserong commented Apr 17, 2020

yansern commented Apr 17, 2020

yansern commented Apr 17, 2020

p-jackson commented Apr 20, 2020

andrewserong commented Apr 21, 2020

ramonjd left a comment

Choose a reason for hiding this comment

p-jackson commented Apr 21, 2020

ramonjd commented Apr 21, 2020

p-jackson commented Apr 21, 2020

ockham commented Apr 21, 2020

andrewserong commented Apr 16, 2020 •

edited

Loading

matticbot commented Apr 16, 2020 •

edited

Loading

ockham commented Apr 16, 2020 •

edited

Loading

simison commented Apr 16, 2020 •

edited

Loading

sirino commented Apr 16, 2020 •

edited

Loading

jsnmoon commented Apr 16, 2020 •

edited

Loading