feat(cli): [STENCIL-12] Adding Telemetry and CLI features. #2964

splitinfinities · 2021-07-14T14:55:25Z

Adds Telemetry options to the CLI. Try it out by running any of these commands in your linked project:

npx stencil telemetry
npx stencil telemetry on|off

The debug flag will print the telemetry results, e.g:
npx stencil generate a-component --debug --verbose

Satisfies ADR-1

src/cli/telemetry/ipc.ts

src/cli/telemetry/telemetry.ts

package.json

src/declarations/stencil-public-compiler.ts

src/cli/telemetry/telemetry.ts

src/cli/task-telemetry.ts

src/cli/telemetry/telemetry.ts

rwaskiewicz · 2021-07-23T22:57:29Z

@splitinfinities setting a reminder for both of us to reset the target branch on Monday. Have a great weekend!

…Telemetry

rwaskiewicz · 2021-07-27T16:45:38Z

src/cli/ionic-config.ts

  const config = await readConfig();
-  await writeConfig(Object.assign(config, newOptions));
+  return await writeConfig(Object.assign(config, newOptions));


Can we please add tests on the return value now that we're returning a primitive? We should be able to assert that we return true when writeConfig is successful, and false when it fails

Oh, great point! Yes.

@splitinfinities did we get this under test?

Is this adequate? https://github.com/ionic-team/stencil/pull/2964/files#diff-a2d702e7c67979d0440b8322026a5249d8206bbeaf3e73f7c84df87cfdd2d07cR69

I think this is fine ATM - normally I'd like us to get a little more granular there with respect to what we're testing (calling the ionic-config exposed functions directly) and splitting the tests out based on the assertion, but I think we can leave that for a future day

src/cli/task-help.ts

rwaskiewicz · 2021-07-27T17:00:18Z

src/cli/task-help.ts


-export const taskHelp = (sys: CompilerSystem, logger: Logger) => {
-  const p = logger.dim(sys.details.platform === 'windows' ? '>' : '$');
+export const taskHelp = async (config: Config) => {


Is there a need to pass Config as an argument here, could we instead call getCompilerSystem() and getLogger() on stencil-cli-config.ts? What's the preferred interface to use for this singleton?

I would love to migrate to use the singleton in more cases like what you point out - I just didn't want to change too much. I personally enjoy the ergonomics of function calls without any props, as you can tell from the code I added.

For the singleton, my thought process was, as we come in and make modifications, let's refactor code to use the singleton more often. I'm happy to go farther, but want to keep this PR scoped closer to Telemetry than general cleanup and ergonomics.

See #2964 (comment) - I switched this out.

src/cli/task-telemetry.ts

rwaskiewicz · 2021-07-27T17:34:10Z

src/cli/telemetry/telemetry.ts

+    arguments: flags.args,
+    task: flags.task,


Is it possible that flags can be undefined? I thought that's why we used optional chaining here. Maybe I'm missing something (likely)? Otherwise I'd be concerned the compiler is missing something there 🤔

I've removed that optional chain.

Does that solve the issue though? We define _flags as:

export default class StencilCLIConfig { static instance: StencilCLIConfig; private _flags: ConfigFlags | undefined; private constructor(options: StencilCLIConfigArgs) { this._flags = options?.flags || undefined; } public static getInstance(options?: StencilCLIConfigArgs): StencilCLIConfig { if (!StencilCLIConfig.instance) { StencilCLIConfig.instance = new StencilCLIConfig(options); } return StencilCLIConfig.instance; } public get flags() { return this._flags; } public set flags(flags: ConfigFlags) { this._flags = flags; }

so it's possible we called the ctor for the first time with an empty object literal

const baz = getInstance({});

which sets _flags to undefined right? Barring setting that somewhere else, this would be an unsafe access

Considering that use case you mentioned couldn't be possible as the code is written today, I don't think we'll experience that issue.

src/cli/telemetry/telemetry.ts

src/cli/telemetry/test/telemetry.spec.ts

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

src/cli/run.ts

src/cli/task-help.ts

src/cli/telemetry/telemetry.ts

Co-authored-by: Lars Mikkelsen <lars@ionic.io>

src/cli/telemetry/telemetry.ts

…clobber other fetch definitions.

Co-authored-by: Lars Mikkelsen <lars@ionic.io>

src/cli/ionic-config.ts

ltm · 2021-07-29T18:55:19Z

src/cli/telemetry/telemetry.ts

+    if (type === 'www' && (!!target?.serviceWorker || target.baseUrl !== '/')) {
+      type = `${type} (PWA/App)`;
+    }


Rather than overloading the output targets, I'd add a separate property (e.g. has_app_target) which you might be able to implement as:

function hasAppTarget(config: Config): boolean { return config.outputTargets.some(target => target.type === 'www' && (!!target.serviceWorker || target.baseUrl !== '/')); }

Ohhh, that's a great idea!

Added it as has_app_pwa_config on TrackableData.

See c251962

Co-authored-by: Lars Mikkelsen <lars@ionic.io>

…rget return value.

src/cli/test/ionic-config.spec.ts

rwaskiewicz · 2021-07-30T16:53:52Z

src/cli/ionic-config.ts

@@ -21,6 +21,10 @@ export async function readConfig(): Promise<TelemetryConfig> {
    };

    await writeConfig(config);
+  } else if (!!config && !config['tokens.telemetry'].match(UUID_REGEX)) {
+    const newUuid = uuidv4();
+    await writeConfig(Object.assign(config, { 'tokens.telemetry': newUuid }));


FYI/no action required - from a TypeScript typing perspective, there is a subtle difference between using Object.assign and the spread operator. Object.assign is typed such that the return type is the intersection of the two args:

assign<T, U>(target: T, source: U): T & U;

if we were to mess up the name of the key in U, the compiler wouldn't catch the error:

// compiles just fine await writeConfig(Object.assign(config, { 'tokens.teleasdf': newUuid }));

whereas spreading the object will catch an error

// errors! we can't assign to type `TelemetryConfig` await writeConfig({ ...config, 'tokens.teleasdf': newUuid });

There are some subtle differences between the two (see 'Differences Versus Object.assign()') but I don't think they apply here

Thank you. I did this because I wanted to modify the object in place, but the TypeScript side effect is news to me... Would you like to add prefer-object-spread: error to our ESLint rules? Open to either option.

Maybe WRT prefer-object-spread, I need to start having that conversation with Lars this week about long term linting & formatting plans

rwaskiewicz · 2021-07-30T16:54:43Z

src/cli/telemetry/telemetry.ts

@@ -141,21 +144,18 @@ export async function telemetryAction(action?: TelemetryCallback) {
  }
 }

+function hasAppTarget(): boolean {


Can we get this under test please?

I was under the impression that you only wanted exported members while we're getting started on coverage. Is that your goal?

That's true in that I don't believe we should be exposing functions solely for the sake of testing (there's always exceptions to the rule 😛). But! The data we're sending has now changed: we have this has_app_pwa_config field whose value is predicated on the evaluation of this function body:

getStencilCLIConfig().validatedConfig.config.outputTargets.some( target => target.type === 'www' && (!!target.serviceWorker || target.baseUrl !== '/'), );

So we should be able to assert that given some outputTargets on the config, that this function's various branches set the desired result in has_app_pwa_config. e.g.

false when the output target list is empty

false when there is no target type of 'www'

false when there is a target type of 'www' & both target.serviceWorker is false and baseUrl is '/'

truewhen there is a target type of 'www' & both target.serviceWorker is true and baseUrl is '/'

truewhen there is a target type of 'www' & both target.serviceWorker is false and baseUrl is not '/'

rwaskiewicz

Approved with a few minor asks

rwaskiewicz · 2021-08-03T14:27:23Z

src/cli/telemetry/telemetry.ts

+export const prepareData = async (duration_ms: number, component_count: number = undefined): Promise<TrackableData> => {
+  const { flags, sys } = getStencilCLIConfig();
+  const { typescript, rollup } = getCoreCompiler()?.versions || { typescript: 'unknown', rollup: 'unknown' };
+  const packages = await getInstalledPackages() || [];


I think this always returns an array, and don't think we need the || [] here

rwaskiewicz · 2021-08-03T14:30:01Z

src/cli/telemetry/test/telemetry.spec.ts

+  });
+});
+
+describe('hasAppTarget', () => {


This outer level describe matches the one on line 80. Did you mean 'prepareData'?

rwaskiewicz · 2021-08-03T14:31:17Z

src/cli/telemetry/test/telemetry.spec.ts

+    expect(telemetry.hasAppTarget()).toBe(true)
+  });
+
+  it('Result is correct when outputTargets contains www with serviceWorker', () => {


I think this test name needs to be updated, there's no service worker field set in this test

rwaskiewicz · 2021-08-03T14:31:35Z

src/cli/telemetry/test/telemetry.spec.ts

+    expect(telemetry.hasAppTarget()).toBe(true)
+  });
+
+  it('Result is correct when outputTargets contains www with serviceWorker', () => {


I think this test name needs to be updated, it mimics the two previous test names

* users may opt-out at any time * provide a CLI interface for checking current telemetry status, toggling participation * measure tasks that the Stencil CLI performs today, anonymize data, and send to Ionic for aggregation

ltm reviewed Jul 14, 2021

View reviewed changes

src/cli/telemetry/ipc.ts Outdated Show resolved Hide resolved

splitinfinities force-pushed the chore/telemetry/STENCIL-12-add-telemetry branch 2 times, most recently from 0ec5bbd to 276a2fb Compare July 14, 2021 19:14

ltm reviewed Jul 15, 2021

View reviewed changes

src/cli/telemetry/telemetry.ts Outdated Show resolved Hide resolved

splitinfinities force-pushed the chore/telemetry/STENCIL-12-add-telemetry branch 4 times, most recently from 5e24abc to 50b2792 Compare July 21, 2021 14:58

rwaskiewicz reviewed Jul 21, 2021

View reviewed changes

splitinfinities added the work-in-progress: do not review label Jul 26, 2021

splitinfinities added 2 commits July 26, 2021 10:26

feat(cli): [STENCIL-12] Adding Telemetry and CLI for users to toggle …

f1deae7

…Telemetry

Fixes from Ryan

628c0c7

splitinfinities force-pushed the chore/telemetry/STENCIL-12-add-telemetry branch from 50b2792 to 628c0c7 Compare July 26, 2021 18:26

splitinfinities changed the base branch from chore/telemetry/STENCIL-33-writing-and-reading-config to master July 27, 2021 16:15

Fixes from Ryan + better interop by using fetch instead of request.

d52a7d1

splitinfinities force-pushed the chore/telemetry/STENCIL-12-add-telemetry branch from 8aee630 to d52a7d1 Compare July 27, 2021 16:33

splitinfinities added work-in-progress: please review and removed work-in-progress: do not review labels Jul 27, 2021

splitinfinities requested review from ltm and rwaskiewicz July 27, 2021 16:33

rwaskiewicz suggested changes Jul 27, 2021

View reviewed changes

splitinfinities and others added 6 commits July 27, 2021 13:33

Update src/cli/task-telemetry.ts

f524372

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

Add missed new line in telemetry

d889830

Update src/cli/telemetry/shouldTrack.ts

c635329

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

Update src/cli/telemetry/telemetry.ts

739e62c

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

Update src/cli/telemetry/telemetry.ts

cdf1879

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

Feedback from Ryan

dbbb0cb

ltm reviewed Jul 28, 2021

View reviewed changes

src/cli/run.ts Outdated Show resolved Hide resolved

src/cli/run.ts Show resolved Hide resolved

src/cli/task-help.ts Outdated Show resolved Hide resolved

ltm suggested changes Jul 28, 2021

View reviewed changes

src/cli/telemetry/telemetry.ts Outdated Show resolved Hide resolved

src/cli/telemetry/telemetry.ts Outdated Show resolved Hide resolved

Update src/cli/telemetry/telemetry.ts

3d466ed

Co-authored-by: Lars Mikkelsen <lars@ionic.io>

splitinfinities requested a review from a team July 28, 2021 15:52

Update src/cli/telemetry/telemetry.ts

871ac40

Co-authored-by: Lars Mikkelsen <lars@ionic.io>

ltm reviewed Jul 28, 2021

View reviewed changes

src/cli/telemetry/telemetry.ts Outdated Show resolved Hide resolved

ltm suggested changes Jul 28, 2021

View reviewed changes

src/cli/telemetry/telemetry.ts Outdated Show resolved Hide resolved

src/cli/telemetry/telemetry.ts Show resolved Hide resolved

src/cli/telemetry/telemetry.ts Outdated Show resolved Hide resolved

splitinfinities and others added 4 commits July 28, 2021 13:18

Fix ts issues and revert the global pointer for fetch because it may …

60865ea

…clobber other fetch definitions.

Update src/cli/telemetry/telemetry.ts

bce8319

Co-authored-by: Lars Mikkelsen <lars@ionic.io>

Feedback from Lars

6448a29

Add session to the Metric as opposed to the data sent

250f28c

ltm reviewed Jul 29, 2021

View reviewed changes

src/cli/ionic-config.ts Outdated Show resolved Hide resolved

ltm reviewed Jul 29, 2021

View reviewed changes

splitinfinities and others added 3 commits July 30, 2021 09:42

Update src/cli/ionic-config.ts to match Ionic's CLI

66c8019

Co-authored-by: Lars Mikkelsen <lars@ionic.io>

Feedback from Lars, adding the pwa prop instead of overloading the ta…

c251962

…rget return value.

Add validation on reading the telemetry token

45f5a11

splitinfinities requested review from ltm and rwaskiewicz July 30, 2021 15:41

rwaskiewicz suggested changes Jul 30, 2021

View reviewed changes

ltm approved these changes Jul 30, 2021

View reviewed changes

splitinfinities added 2 commits August 2, 2021 10:34

Write tests to a temporary file.

2e01743

Improve testability of Telemetry code

b8986c7

splitinfinities requested a review from rwaskiewicz August 3, 2021 14:35

rwaskiewicz approved these changes Aug 3, 2021

View reviewed changes

splitinfinities added 2 commits August 3, 2021 10:09

Fix from Ryan

f9644b8

Fixes from Ryan

063cc1c

rwaskiewicz merged commit 1381cc7 into master Aug 4, 2021

rwaskiewicz deleted the chore/telemetry/STENCIL-12-add-telemetry branch August 4, 2021 16:48

splitinfinities removed the work-in-progress: please review label Aug 19, 2021

zanettin mentioned this pull request Sep 3, 2021

Not compatible with @stencil/core@2.8.0 pkg nxext/nx-extensions#303

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): [STENCIL-12] Adding Telemetry and CLI features. #2964

feat(cli): [STENCIL-12] Adding Telemetry and CLI features. #2964

splitinfinities commented Jul 14, 2021 •

edited

Loading

rwaskiewicz commented Jul 23, 2021

rwaskiewicz Jul 27, 2021

splitinfinities Jul 27, 2021

rwaskiewicz Jul 28, 2021

splitinfinities Jul 28, 2021

rwaskiewicz Jul 29, 2021

rwaskiewicz Jul 27, 2021

splitinfinities Jul 27, 2021

splitinfinities Jul 28, 2021

rwaskiewicz Jul 27, 2021

splitinfinities Jul 27, 2021

rwaskiewicz Jul 28, 2021 •

edited

Loading

splitinfinities Jul 30, 2021

ltm Jul 29, 2021

splitinfinities Jul 30, 2021

splitinfinities Jul 30, 2021

splitinfinities Jul 30, 2021

rwaskiewicz Jul 30, 2021

splitinfinities Aug 2, 2021

rwaskiewicz Aug 2, 2021

rwaskiewicz Jul 30, 2021

splitinfinities Aug 2, 2021

rwaskiewicz Aug 2, 2021

rwaskiewicz left a comment

rwaskiewicz Aug 3, 2021

rwaskiewicz Aug 3, 2021

splitinfinities Aug 3, 2021

rwaskiewicz Aug 3, 2021

rwaskiewicz Aug 3, 2021

feat(cli): [STENCIL-12] Adding Telemetry and CLI features. #2964

feat(cli): [STENCIL-12] Adding Telemetry and CLI features. #2964

Conversation

splitinfinities commented Jul 14, 2021 • edited Loading

rwaskiewicz commented Jul 23, 2021

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

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

rwaskiewicz Jul 28, 2021 • 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

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

Choose a reason for hiding this comment

rwaskiewicz 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

splitinfinities commented Jul 14, 2021 •

edited

Loading

rwaskiewicz Jul 28, 2021 •

edited

Loading