report(links): utm params for all developer.google.com links #4972

kdzwinel · 2018-04-15T21:28:03Z

Appends additional params to all developer.google.com links for easier tracking:

utm_source - hardcoded to 'lighthouse'
utm_medium - "cli", "ext" (extension), "cdt" (devtools), "nm" (node module) depending on how LH was run
utm_content - "pass", "fail" depending on audit result (if available)

Example:

Explanation

As discussed in April, channel is now part of the lighthouseVersion in the output object (e.g. '3.0.3+cli'). Applications can pass channel value via LH_CHANNEL environment variable.

The algorithm is as follows:

if LH is run via extension, LH_CHANNEL is hardcoded to 'ext'
if LH is run via Chrome DevTools, LH_CHANNEL is hardcoded to 'cdt'
if LH_CHANNEL environment variable is set, it's used
if LH_CHANNEL is not set:
- and if LH is run via lighthouse-cli/index.js, LH_CHANNEL is set to 'cli'
- and if LH is not run via lighthouse-cli/index.js, LH_CHANNEL is set to 'nm'

kdzwinel · 2018-04-15T21:34:50Z

lighthouse-core/environment/cli.js

+
+module.exports = {
+  name: 'CLI',
+};


I was also considering having one const ENVIRONMENT = 'replace-me' in the runner.js instead of three environment/ files, but replacing imports instead of values seemed a tiny bit cleaner.

yeah I agree abstractly, unfortunately it doesn't seem as easy as I expected in browserify 😆

is there any simpler way with less manual? I'm no browserify expert, maybe something similar to the webpack define plugin or simple shim replacement without custom transform?

with the audits/gatherers we manually do something fancy without a transform which is what makes me hopeful :D

webpack define plugin

that was a good tip, thanks! (after a lot of googling) I found similar solution for browserify 👌

kdzwinel · 2018-04-15T21:35:10Z

docs/understanding-results.md

@@ -10,7 +10,8 @@ The top-level Lighthouse Result object (LHR) is what the lighthouse node module

 | Name | Description |
 | - | - |
-| lighthouseVersion | The version of Lighthouse with which this result were generated. |
+| lighthouseVersion | The version of Lighthouse with which this result was generated. |
+| lighthouseEnvironment | The environment in which Lighthouse result was generated (CLI, extension, DevTools). |


I'll be happy to change that name, I couldn't figure out anything better

we should figure out where this fits into the LHR reshuffling. Like the discussion on runtimeConfig, maybe this should move into the settings property (whatever we end up calling it) that's copied over from the config settings and has all the flags, throttling settings, etc, used in this run

kdzwinel · 2018-04-15T21:37:33Z

lighthouse-core/runner.js

@@ -110,6 +110,7 @@ class Runner {
      // Entering: conclusion of the lighthouse result object
      // @ts-ignore - Needs json require() support
      const lighthouseVersion = /** @type {string} */ (require('../package.json').version);
+      const lighthouseEnvironment = /** @type {string} */ (require('./environment/cli').name);


'./environment/cli' this part gets replaced by browserify

patrickhulce · 2018-04-16T16:53:45Z

lighthouse-core/environment/extension.js

+'use strict';
+
+module.exports = {
+  name: 'extension',


since these seems to be display-oriented how about 'Extension' alternatively we make all these lowercase and just switch in the report to compute the display version

either seems ok with me

Yeah, changing this to 'Extension' SGTM 👍

kdzwinel · 2018-04-17T10:43:26Z

lighthouse-extension/gulpfile.js

+      const environment = file.path.includes('-ext-') ? 'Extension' : 'DevTools';
+      let bundle = browserify(file.path, {
+        insertGlobalVars: {
+          'global.ENVIRONMENT': () => `"${environment}"`,


I won't be able to answer any questions starting with "why" regarding this line. It's just how it's done in browserify e.g. we can't just have () => environment here because we will end up with {"ENVIRONMENT": DevTools} (note missing quotes) in the result code 🤷‍♂️

hurray much easier! :D 🎉

yeah webpack works similarly where it's basically straight up string replacement, FWIW I've usually seen JSON.stringify(environment) to try to get this message across but I don't have a strong opinion

@brendankenny @paulirish either way make more sense to you?

to add another possibility, we could use https://github.com/hughsk/envify and then use process.env.LH_ENVIRONMENT or whatever. That would avoid the mystery global variable and allow overriding on the CLI side if for some reason you'd want to do that (WPT or something?)

sticking with insertGlobalVars is in my opinion a better ideas as it's native browserify. We can still use env vars with process.env which makes it a bit more verbose. I would try to use JSON.stringify as it's more common due webpacks implementation as patrick suggested.

kdzwinel · 2018-04-17T11:36:16Z

@patrickhulce I replaced custom browserify transform with something less manual + added tests for convertMarkdownLinkSnippets. PTAL

kdzwinel · 2018-04-17T12:17:21Z

lighthouse-core/runner.js

@@ -110,6 +110,8 @@ class Runner {
      // Entering: conclusion of the lighthouse result object
      // @ts-ignore - Needs json require() support
      const lighthouseVersion = /** @type {string} */ (require('../package.json').version);
+      // @ts-ignore - ENVIRONMENT varialble is not declared


@brendankenny not sure how to best handle that. Should we have a separate ts file with interface Global?

we're already stomping all over the global scope in

lighthouse/typings/externs.d.ts

Lines 11 to 17 in 4b1f2bb

declare global {

// Augment global Error type to include node's optional `code` property

// see https://nodejs.org/api/errors.html#errors_error_code

interface Error {

code?: string;

}

:) Not sure if you can just declare it there or have to augment global (or Window, or however they do the global mapping for node) like Error is being augmented in those lines

paulirish · 2018-04-25T17:17:08Z

cli, ext, cdt, nm, wpt, lr
LH_CHANNEL
3.0.0+cli

rviscomi · 2018-07-20T19:40:53Z

What's the status of this PR?

…to 'channel'.

kdzwinel · 2018-07-26T15:58:16Z

lighthouse-extension/gulpfile.js

  // Fix an issue with imported speedline code that doesn't brfs well.
  return bundle.transform('./fs-transform', {global: true})
+  // Replace LH_CHANNEL enviroment variable with a string
+  .transform('./lh-channel-transform', {channel, global: true})


This is a custom find&replace transform. I couldn't use envify because it's based on esprima 4 (latest) which doesn't support object rest/spread :(

kdzwinel · 2018-07-26T16:01:59Z

@rviscomi just updated!

@patrickhulce @paulirish Sorry it took so long, but I updated this PR as discussed in April (see the description)

lighthouse-core/report/html/renderer/dom.js

rviscomi · 2018-08-09T15:55:04Z

lighthouse-extension/gulpfile.js

  // Fix an issue with imported speedline code that doesn't brfs well.
  return bundle.transform('./fs-transform', {global: true})
+  // Replace LH_CHANNEL enviroment variable with a string


nit: s/enviroment/environment/

rviscomi

LGTM. I'll defer to @patrickhulce @brendankenny or @brendankenny if they have any technical feedback.

patrickhulce

LGTM thanks @kdzwinel!!

patrickhulce · 2018-08-23T16:26:05Z

lighthouse-core/runner.js

@@ -106,7 +106,12 @@ class Runner {
      }

      // Entering: conclusion of the lighthouse result object
-      const lighthouseVersion = require('../package.json').version;
+
+      // @ts-ignore - Needs json require() support


@brendankenny do you have any tips for him here?

not sure why this would be be necessary now when it wasn't before 🤔

I think this is just a bad merge or rebase. I just removed this ts-ignore in the last week or two

Good call, I removed it 👍

patrickhulce · 2018-08-23T16:31:41Z

lighthouse-core/report/html/renderer/dom.js

+          if (metadata && metadata.channel) {
+            url.searchParams.set('utm_medium', metadata.channel);
+          }
+          if (metadata && metadata.rating) {


oh my, I'm really regretting us calling that pass/fail string rating now 😆

brendankenny

Just a few last things.

Can you also add a check to extension-test.js, so it gets a test through the full pipeline? It should be straightforward to grab a description or two and make sure the URL has the utm params and channel is set to ext

Thanks for sticking with this!!

lighthouse-core/report/html/renderer/dom.js

lighthouse-core/test/report/html/renderer/dom-test.js

lighthouse-extension/lh-channel-transform.js

googlebot · 2018-10-03T10:26:18Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

mattzeunert · 2018-10-03T12:04:44Z

@brendankenny Hey, I'm taking over this PR from Konrad. I've addressed your comments, can you take a look?

brendankenny · 2018-10-11T20:42:21Z

@mattzeunert Sorry for being slow. Also sent you a message out of band, but I think we're going to move this to chillin and wait on it for a little while.

There's been a lot of churn in the bundle/channel entry points, file names, and control flow, so things have changed a good bit in this area since the PR started. The changes needed for utm params could probably get simpler (e.g. maybe no browserify transform) but also needs to handle new stuff (like the lr channel). Rather than fixing this PR and then changing it a few times while things continue to churn before CDS, let's just put it off for a little while.

brendankenny · 2019-01-30T01:18:18Z

Let's close instead of chilling indefinitely. I'll leave a note on #4663

kdzwinel requested review from patrickhulce, paulirish and vinamratasingal-zz as code owners April 15, 2018 21:28

devtools-bot added the waiting4reviewer label Apr 15, 2018

kdzwinel commented Apr 15, 2018

View reviewed changes

patrickhulce reviewed Apr 16, 2018

View reviewed changes

kdzwinel commented Apr 17, 2018

View reviewed changes

paulirish removed the waiting4reviewer label Apr 30, 2018

Pass run channel via environment variable. Rename from 'environment' …

3e006b9

…to 'channel'.

kdzwinel force-pushed the docs-links-utm-params branch from 7c310ad to 3e006b9 Compare July 26, 2018 15:18

kdzwinel requested a review from ebidel as a code owner July 26, 2018 15:18

kdzwinel added 2 commits July 26, 2018 17:39

Pass audit result via utm

112fa91

Make typescript happy.

db27a63

kdzwinel commented Jul 26, 2018

View reviewed changes

Merge branch 'master' into docs-links-utm-params

8346057

rviscomi reviewed Aug 9, 2018

View reviewed changes

lighthouse-core/report/html/renderer/dom.js Show resolved Hide resolved

rviscomi reviewed Aug 9, 2018

View reviewed changes

Spelling

87aee7b

rviscomi approved these changes Aug 16, 2018

View reviewed changes

Merge branch 'master' into docs-links-utm-params

cc3a6c4

patrickhulce approved these changes Aug 23, 2018

View reviewed changes

Remove, no longer needed, ts-ignore

d344c8c

brendankenny requested changes Aug 23, 2018

View reviewed changes

Add test checking only dev site gets utm params

330d539

mattzeunert added 4 commits October 3, 2018 12:02

Brendan tweaks

234cdad

Test that final result description URLs have utm parameters

3288bb1

Merge master

0d4725f

Fix formatting

afefc4d

brendankenny added the chillin ❄ label Oct 11, 2018

brendankenny closed this Jan 30, 2019

brendankenny mentioned this pull request Jan 30, 2019

When linking to LH docs, include utm parameters for analytics #4663

Closed

mattzeunert mentioned this pull request Feb 25, 2019

core(config): specify lighthouse channel #7312

Merged

mattzeunert mentioned this pull request Mar 10, 2019

report(links): include utm params in links to docs #7441

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report(links): utm params for all developer.google.com links #4972

report(links): utm params for all developer.google.com links #4972

kdzwinel commented Apr 15, 2018 •

edited

Loading

kdzwinel Apr 15, 2018

patrickhulce Apr 16, 2018

kdzwinel Apr 17, 2018

kdzwinel Apr 15, 2018

brendankenny Apr 18, 2018

kdzwinel Apr 15, 2018

patrickhulce Apr 16, 2018 •

edited

Loading

kdzwinel Apr 17, 2018

kdzwinel Apr 17, 2018

patrickhulce Apr 17, 2018 •

edited

Loading

brendankenny Apr 18, 2018

wardpeet Apr 22, 2018

kdzwinel commented Apr 17, 2018

kdzwinel Apr 17, 2018

brendankenny Apr 18, 2018 •

edited

Loading

paulirish commented Apr 25, 2018 •

edited

Loading

rviscomi commented Jul 20, 2018

kdzwinel Jul 26, 2018 •

edited

Loading

kdzwinel commented Jul 26, 2018

rviscomi Aug 9, 2018

rviscomi left a comment

patrickhulce left a comment

patrickhulce Aug 23, 2018

brendankenny Aug 23, 2018

kdzwinel Aug 23, 2018

patrickhulce Aug 23, 2018

brendankenny left a comment

googlebot commented Oct 3, 2018

mattzeunert commented Oct 3, 2018

brendankenny commented Oct 11, 2018

brendankenny commented Jan 30, 2019

	declare global {
	// Augment global Error type to include node's optional `code` property
	// see https://nodejs.org/api/errors.html#errors_error_code
	interface Error {
	code?: string;
	}

report(links): utm params for all developer.google.com links #4972

report(links): utm params for all developer.google.com links #4972

Conversation

kdzwinel commented Apr 15, 2018 • edited Loading

Explanation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdzwinel commented Apr 17, 2018

Choose a reason for hiding this comment

brendankenny Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

paulirish commented Apr 25, 2018 • edited Loading

rviscomi commented Jul 20, 2018

kdzwinel Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

kdzwinel commented Jul 26, 2018

Choose a reason for hiding this comment

rviscomi left a comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

googlebot commented Oct 3, 2018

mattzeunert commented Oct 3, 2018

brendankenny commented Oct 11, 2018

brendankenny commented Jan 30, 2019

kdzwinel commented Apr 15, 2018 •

edited

Loading

patrickhulce Apr 16, 2018 •

edited

Loading

patrickhulce Apr 17, 2018 •

edited

Loading

brendankenny Apr 18, 2018 •

edited

Loading

paulirish commented Apr 25, 2018 •

edited

Loading

kdzwinel Jul 26, 2018 •

edited

Loading