core(config): specify lighthouse channel #7312

mattzeunert · 2019-02-24T11:10:14Z

Summary

Determine whether Lighthouse was run through the CLI, npm module, Chrome extension, Chrome DevTools, or Lightrider.

Is settings the right place to put this? Feels like it's similar to locale.

Related Issues/PRs

Part of #4663

patrickhulce

nice! this turned out to be super clean with the bundling refactors! 🎉

I think this all seems good to me! I do worry a bit that someone out there was not treating lighthouseVersion as a complete, parsable semantic version and doing version.split('.') or something but I doubt they cared about the patch version component so we're good? 🤷‍♂️

types/externs.d.ts

exterkamp

Looks fine, just some minor questions.

lighthouse-core/test/results/sample_v2.json

proto/lighthouse-result.proto

brendankenny

so one thing we didn't really talk about in #4663 is what happens as the LHR gets separated from its original source. Based on the conversation there, the primary purpose (for now) is to know where traffic is coming from when clicking links in the report, but what happens when you save an LHR json from DevTools or PSI and open it in viewer? Is it still useful information to know the original source, or should it be utm_medium=viewer at that point? Especially if it's shared with multiple other people...none of them are touching the original source in that case.

types/externs.d.ts

lighthouse-core/index.js

brendankenny · 2019-02-26T00:02:54Z

lighthouse-core/test/results/sample_v2.json

@@ -3172,6 +3172,7 @@
    "disableStorageReset": false,
    "disableDeviceEmulation": false,
    "emulatedFormFactor": "mobile",
+    "channel": "cli",


should it also be in the top level of the LHR? Maybe inside environment, next to benchmarkIndex, etc?

Is there a benefit over using lhr.configSettings.channel?

Is there a benefit over using lhr.configSettings.channel?

because it's not really a setting, but now it kind of is, so that's fine

brendankenny · 2019-02-26T00:10:34Z

clients/devtools-entry.js

@@ -36,6 +36,7 @@ function getDefaultConfigForCategories(categoryIDs) {
 function runLighthouseInWorker(port, url, flags, categoryIDs) {
  // Default to 'info' logging level.
  flags.logLevel = flags.logLevel || 'info';
+  flags.channel = 'cdt';


do we have a good place to test any/all of these? It's going to be difficult to notice if something is accidentally falling back to the node default, but I don't know if we have a good place to check

Added a test case for the Chrome extension.

patrickhulce · 2019-02-26T00:45:11Z

Is it still useful information to know the original source, or should it be utm_medium=viewer at that point? Especially if it's shared with multiple other people...none of them are touching the original source in that case.

I think the original source is far more important for environment info than where they are currently looking at it. Maybe not the correct thing from utm medium perspective, but I think viewer is the only real case where there will be a mismatch and maybe viewer just overrides that with UI features or something?

mattzeunert · 2019-03-02T17:55:56Z

Any more feedback on this?

patrickhulce

LGTM!

exterkamp

I don't really think I'm satisfied with the field just being freeform string, I'd prefer a enumerated type on ts and on the proto. I like the longer names, but personally I want them enumerated.

mattzeunert · 2019-03-03T11:33:17Z

I don't really think I'm satisfied with the field just being freeform string, I'd prefer a enumerated type on ts and on the proto. I like the longer names, but personally I want them enumerated.

@exterkamp We allow CLI and node module consumers to pass in their own channel values. Does it still make sense to enumerate the possible values, since I think we need string anyway?

I feel it's similar on the Lightrider/proto side, but maybe we can put psi and webdotdev in the enum?

exterkamp · 2019-03-04T18:56:08Z

Hm, I might have missed this discussion (I don't see it in this PR?), but why do they get to pass their own channel? Wouldn't their channel be "cli" or "node-module"? Since that is the channel they were on? What's the use case for sending a custom run channel that we don't know about beforehand and have enumerated?

brendankenny · 2019-03-04T19:08:33Z

I think it comes down to what the purpose of the field is.

As helpful debug info after the fact, it makes sense to stick to just our "clients" (cli/node/devtools/extension/lr). That's the programmatic entry point, great.

However, #4663 was about getting a sense of what lead users to our docs. In that case it makes some sense to have it open so we can distinguish between e.g. the cli and PSI and WPT, which are all very different user experience entry points.

exterkamp · 2019-03-04T19:43:07Z

I've been convinced irl. sgtm!

mattzeunert · 2019-03-04T21:41:13Z

@exterkamp Should be good to merge now 🙂

brendankenny

ok, inspired by your extension test, left some suggestions for the other channels (except devtools)

brendankenny · 2019-03-05T23:02:30Z

clients/test/extension/extension-test.js

@@ -176,4 +179,8 @@ describe('Lighthouse chrome extension', function() {
    // this audit has regressed in the extension twice, so make sure it passes
    assert.ok(await extensionPage.$('#is-crawlable.lh-audit--pass'), 'did not pass is-crawlable');
  });
+
+  it('should specify the channel as extension', async () => {
+    assert.equal(lhr.configSettings.channel, 'extension');


brendankenny · 2019-03-05T23:08:50Z

clients/lightrider-entry.js

@@ -34,6 +34,7 @@ async function runLighthouseInLR(connection, url, flags, {lrDevice, categoryIDs,
  // disableStorageReset because it causes render server hang
  flags.disableStorageReset = true;
  flags.logLevel = flags.logLevel || 'info';
+  flags.channel = 'lr';


can assert this in lightrider-entry-test.js

brendankenny · 2019-03-05T23:10:26Z

lighthouse-cli/cli-flags.js

@@ -146,6 +147,7 @@ function getFlags(manualArgv) {
      .default('port', 0)
      .default('hostname', 'localhost')
      .default('enable-error-reporting', undefined) // Undefined so prompted by default
+      .default('channel', 'cli')


can assert this in 'runLighthouse completes a LH round trip' in run-test.js

brendankenny · 2019-03-05T23:13:12Z

lighthouse-core/config/constants.js

@@ -51,6 +51,7 @@ const defaultSettings = {
  disableStorageReset: false,
  disableDeviceEmulation: false,
  emulatedFormFactor: 'mobile',
+  channel: 'node',


and can assert this in lighthouse-core/test/index-test.js (basically just copy 'should return formatted LHR when given no categories' and delete most of it :)

brendankenny · 2019-03-05T23:13:47Z

lighthouse-core/test/results/sample_v2.json

@@ -3172,6 +3172,7 @@
    "disableStorageReset": false,
    "disableDeviceEmulation": false,
    "emulatedFormFactor": "mobile",
+    "channel": "cli",


Is there a benefit over using lhr.configSettings.channel?

because it's not really a setting, but now it kind of is, so that's fine

mattzeunert · 2019-03-06T08:23:16Z

@brendankenny Thanks for those suggestions, added them now!

brendankenny

ok, last thing (jest over sinon), sorry, but otherwise LGTM!

brendankenny · 2019-03-06T22:03:02Z

clients/test/lightrider-entry-test.js

@@ -56,5 +58,18 @@ describe('lightrider-entry', () => {
      assert.strictEqual(parsedResult.runtimeError.code, LHError.UNKNOWN_ERROR);
      assert.ok(parsedResult.runtimeError.message.includes(errorMsg));
    });
+
+    it('specifies the channel as lr', async () => {
+      const runStub = sinon.stub(Runner, 'run');


can you use jest mocks for this? I'm hoping we can drop sinon since we currently only use it in one place

oh +1 to using jest please :)

const runStub = Runner.run = jest.fn() ... const config = runStub.mock.calls[0][1].config

I think?

Good point, somehow I felt I needed to use sinon in mocha tests 🙂

brendankenny · 2019-03-06T22:06:46Z

lighthouse-core/test/index-test.js

@@ -132,6 +132,31 @@ describe('Module Tests', function() {
    });
  });

+  it('should specify the channel as node by default', function() {


not a big deal, but if making one last commit, could you make these async/await? no need to stick to the style of the file for that :)

brendankenny · 2019-03-07T19:31:31Z

thanks!

rviscomi · 2019-03-21T17:32:45Z

After this feature is released we'll also need to update the PSI and web.dev clients to specify their channel configs. @paulirish is there somewhere we can file an issue to do this?

paulirish · 2019-03-22T17:21:11Z

@rviscomi interesting... :) right now PSI webapp, PSI API clients, and web.dev will be tagged as 'lr'.

We don't have a straightforward way to distinguish between different PSI API consumers easily.

How do you want to make use of this channel data?

rviscomi · 2019-03-22T17:24:25Z

The web apps should include their channel in a UTM param appended to their learn more links so we can see where documentation visitors are coming from.

brendankenny · 2019-03-22T17:39:53Z

The web front ends could change the setting in the lhr before generating the report. Pretty gross, though :/

web.dev and the PSI web interface do have their own analytics (I assume), which would allow teasing out those sources, at least.

rviscomi · 2019-03-28T17:14:50Z

Yeah I assume we do have analytics on the tools themselves, this is just for the documentation on d.g.co to easily identify the LH channel that brought the user to the docs.

mattzeunert added 2 commits February 24, 2019 11:02

Specify lighthouse channel

b65cf2b

Update snapshot

53409d3

mattzeunert requested review from brendankenny, exterkamp, patrickhulce and paulirish as code owners February 24, 2019 11:10

mattzeunert changed the title ~~Specify lighthouse channel~~ core(config): specify lighthouse channel Feb 24, 2019

patrickhulce reviewed Feb 24, 2019

View reviewed changes

types/externs.d.ts Outdated Show resolved Hide resolved

Fix trailing */

670dd8a

exterkamp reviewed Feb 25, 2019

View reviewed changes

lighthouse-core/test/results/sample_v2.json Outdated Show resolved Hide resolved

lighthouse-core/test/results/sample_v2.json Show resolved Hide resolved

mattzeunert added 2 commits February 25, 2019 20:43

Don't add channel to version

128ebeb

Add channel to proto

0079086

exterkamp reviewed Feb 25, 2019

View reviewed changes

proto/lighthouse-result.proto Show resolved Hide resolved

brendankenny reviewed Feb 26, 2019

View reviewed changes

mattzeunert added 5 commits February 28, 2019 09:24

Longer names

f6f18a3

Remove LH_CHANNEL env override, add cli flag

918211d

Fix snapshot

57911e6

Merge branch 'master' into lighthouse-channel

3b1c12b

Add test for channel in extension

5400d8f

patrickhulce approved these changes Mar 2, 2019

View reviewed changes

exterkamp requested changes Mar 3, 2019

View reviewed changes

exterkamp approved these changes Mar 4, 2019

View reviewed changes

Merge branch 'master' into lighthouse-channel

80ba8ec

brendankenny reviewed Mar 5, 2019

View reviewed changes

mattzeunert added 3 commits March 6, 2019 08:02

Channel test case for Lightrider

86b7752

Channel assertion for CLI

3ae323a

Assert default channel as node

4b10b87

brendankenny approved these changes Mar 6, 2019

View reviewed changes

mattzeunert added 3 commits March 7, 2019 07:48

Use jest for stubs

ee65c61

Use async await for new tests

8cfe9a6

Merge branch 'master' into lighthouse-channel

88e0e2b

brendankenny merged commit d7c013f into master Mar 7, 2019

brendankenny deleted the lighthouse-channel branch March 7, 2019 19:32

brendankenny mentioned this pull request Oct 17, 2019

report: add channel to version string #9851

Closed

core(config): specify lighthouse channel #7312

core(config): specify lighthouse channel #7312

Conversation

mattzeunert commented Feb 24, 2019

patrickhulce left a comment

Choose a reason for hiding this comment

exterkamp left a comment

Choose a reason for hiding this comment

brendankenny 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

Choose a reason for hiding this comment

patrickhulce commented Feb 26, 2019

mattzeunert commented Mar 2, 2019

patrickhulce left a comment

Choose a reason for hiding this comment

exterkamp left a comment

Choose a reason for hiding this comment

mattzeunert commented Mar 3, 2019

exterkamp commented Mar 4, 2019

brendankenny commented Mar 4, 2019 • edited Loading

exterkamp commented Mar 4, 2019

mattzeunert commented Mar 4, 2019

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Mar 5, 2019 • edited Loading

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

mattzeunert commented Mar 6, 2019

brendankenny 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 commented Mar 7, 2019

rviscomi commented Mar 21, 2019

paulirish commented Mar 22, 2019

rviscomi commented Mar 22, 2019

brendankenny commented Mar 22, 2019 • edited Loading

rviscomi commented Mar 28, 2019

brendankenny commented Mar 4, 2019 •

edited

Loading

brendankenny Mar 5, 2019 •

edited

Loading

brendankenny commented Mar 22, 2019 •

edited

Loading