feat:usage-diagnoses #3641

ammarkarachi · 2020-03-10T23:41:34Z

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

lgtm-com · 2020-05-19T21:38:20Z

This pull request introduces 1 alert when merging 81bc4d1dfe18f2cee473b5a25350d68f905ab31a into 39870f1 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

codecov · 2020-06-05T22:25:11Z

Codecov Report

Merging #3641 into master will decrease coverage by 0.99%.
The diff coverage is 72.50%.

@@            Coverage Diff             @@
##           master    #3641      +/-   ##
==========================================
- Coverage   61.57%   60.57%   -1.00%     
==========================================
  Files         333      349      +16     
  Lines       14550    15133     +583     
  Branches     2764     2865     +101     
==========================================
+ Hits         8959     9167     +208     
- Misses       5164     5478     +314     
- Partials      427      488      +61

Impacted Files	Coverage Δ
packages/amplify-appsync-simulator/src/index.ts	`68.36% <0.00%> (-0.71%)`	⬇️
...ages/amplify-category-auth/commands/auth/remove.js	`80.00% <0.00%> (-5.72%)`	⬇️
.../lib/S3AndCloudFront/helpers/cloudfront-manager.js	`87.50% <0.00%> (ø)`
...g/lib/S3AndCloudFront/helpers/configure-Publish.js	`86.58% <0.00%> (-3.29%)`	⬇️
...sting/lib/S3AndCloudFront/helpers/file-uploader.js	`78.43% <0.00%> (ø)`
.../amplify-category-notifications/lib/auth-helper.js	`0.00% <0.00%> (ø)`
packages/amplify-category-xr/commands/xr/push.js	`33.33% <0.00%> (-4.17%)`	⬇️
packages/amplify-category-xr/lib/xr-manager.js	`13.23% <0.00%> (-0.10%)`	⬇️
packages/amplify-cli/src/domain/context.ts	`100.00% <ø> (ø)`
...amplify-codegen-appsync-model-plugin/src/preset.ts	`0.00% <0.00%> (ø)`
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3afb335...2a7d318. Read the comment docs.

lgtm-com · 2020-06-05T22:39:41Z

This pull request introduces 1 alert when merging 37fe3118e30516b6a90b41aa38fa988e33270054 into 493e631 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

packages/amplify-cli/scripts/post-install.js

attilah · 2020-06-11T14:26:16Z

packages/amplify-cli/scripts/post-install.js

@@ -12,18 +13,30 @@ try {
 } catch (e) {
  // could not delete the cache directory but don't want to fail the installation
 }
-console.log('\n');
+console.log(EOL);


nit: Why are we printing EOLs, to have 2 empty lines? I think we'd be good just a simple empty line.

I wanted to make sure it was platform independent
windows it's '\n\r'

I meant that how is it different from an empty console.log() statement. We want 2 empty lines or what is the reason?

Oh I am sorry I misunderstood, it's just to keep separation more obvious

packages/amplify-cli/scripts/post-install.js

packages/amplify-cli/src/config-steps/c0-analyzeProject.js

.circleci/config.base.yml

packages/amplify-cli/src/telemetry/config.ts

packages/amplify-cli/src/telemetry/getConfig.ts

packages/amplify-cli/src/telemetry/index.ts

packages/amplify-provider-awscloudformation/lib/configuration-manager.js

lgtm-com · 2020-06-15T07:30:49Z

This pull request introduces 2 alerts when merging b93457174354cbb4f74121e35204bd72ffe1aeef into f45d32b - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

lgtm-com · 2020-06-15T07:54:56Z

This pull request introduces 2 alerts when merging d330a9411dcc5b6ff0a1db959f01414a28e698f4 into f45d32b - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

lgtm-com · 2020-06-15T17:26:40Z

This pull request introduces 2 alerts when merging 097235d045969a46cf993c637e90302a9db2ab15 into f45d32b - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

attilah

Thanks for addressing the previous comments, added a few new ones.

packages/amplify-cli/src/__tests__/telemetry.test.ts

attilah · 2020-06-24T23:52:39Z

packages/amplify-cli/src/__tests__/telemetry.test.ts

+
+describe('test telemetry', () => {
+  beforeAll(() => {
+    process.env = Object.assign(process.env, { AMPLIFY_CLI_BETA_USAGE_TRACKING_URL: originalUrl });


Why do we have the BETA in it? I think that we need to use the same variable: AMPLIFY_CLI_USAGE_TRACKING_URL and its value should point to different endpoint during testing.

What do you mean by testing? e2e tests?

We shouldn't be exposing a beta url to the public also.

packages/amplify-cli/src/__tests__/telemetry.test.ts

attilah · 2020-06-25T00:15:47Z

packages/amplify-cli/src/app-config/config.ts

+
+export function write(context: Context, keyValues: Object) {
+  Config.Instance.setValues(keyValues);
+  fs.writeFileSync(getPath(context), JSON.stringify(Config.Instance));


I think this can fail if ~/.amplify directory does not exist, not?

~/.amplify the directory is created during a plugin scan.

packages/amplify-cli/src/domain/amplify-telemetry/Telemetry.ts

attilah · 2020-06-25T00:24:22Z

packages/amplify-cli/src/domain/amplify-telemetry/VersionManager.ts

@@ -0,0 +1,23 @@
+import semver from 'semver';
+
+const APIVersionToPayloadVersion = new Map<string, Array<string>>([['v1.0', ['1.0.0']]]);


What is the design here? a given CLI version just reports 1 version of the payload what we change over time. So the CLI does not have to support multiple versions.

On the other hand it seems that the getLatestApiVersion() is doing the version calculation on every call which seems to be suboptimal as this code is on the hot path if telemetry is enabled, if you decide to keep the multiple versions here, can we cache it between invocations?

I want the API version to be mapped to payload version. This way it's explicit if there are any major changes we would have to make sure the API works with the new payload. No we are not caching it. It's kind of excessive to cache something that's been called twice in a lifetime

attilah · 2020-06-25T00:27:25Z

packages/amplify-cli/src/domain/amplify-telemetry/getTelemetryUrl.ts

+import { getLatestApiVersion } from './VersionManager';
+
+const version = getLatestApiVersion();
+const prodUrl = `https://aws-amplify-cli-telemetry.us-east-1.amazonaws.com/${version}/metrics`;


It seems that the payload version and the API version for telemetry is coupled together?

Yes the payload version is coupled together with the API version

attilah · 2020-06-25T00:28:22Z

packages/amplify-cli/src/domain/amplify-telemetry/getTelemetryUrl.ts

+
+const version = getLatestApiVersion();
+const prodUrl = `https://aws-amplify-cli-telemetry.us-east-1.amazonaws.com/${version}/metrics`;
+export function getUrl(): UrlWithStringQuery {


Are we caching the result of this function between invocations?

No we are not caching it. It's kind of excessive to cache something that's been called twice in a lifetime when the software is running

attilah · 2020-06-25T00:32:40Z

packages/amplify-cli/src/domain/amplify-telemetry/Telemetry.ts

+    this.input = redactInput(input, true);
+  }
+
+  static get Instance(): ITelemetry {


Why do we need static instance variable in the ITelemetry implementations?

This only accessed through context, which already has an instance of those classes.

This is how a singleton is created

attilah · 2020-06-25T00:34:10Z

packages/amplify-cli/src/domain/amplify-telemetry/NoTelemetry.ts

+  }
+  init(installationUuid: String, version: String, input: any): void {}
+
+  private static instance: NoTelemetry;


Same as for the other ITelemetry implementation.

Same as telemetry

This reverts commit 30a7fe7.

This reverts commit a755863.

github-actions · 2021-06-26T01:44:43Z

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.

ammarkarachi requested a review from kaustavghosh06 March 10, 2020 23:41

kaustavghosh06 self-assigned this Mar 12, 2020

ammarkarachi force-pushed the feature/telemetry branch 2 times, most recently from 142b5b4 to 6c4a60b Compare April 10, 2020 22:54

ammarkarachi requested a review from undefobj May 19, 2020 18:21

ammarkarachi force-pushed the feature/telemetry branch from 2ac1aac to 81bc4d1 Compare May 19, 2020 21:23

ammarkarachi force-pushed the feature/telemetry branch from 81bc4d1 to 37fe311 Compare June 5, 2020 22:25

ammarkarachi changed the title ~~Feature/telemetry~~ feat:usage-tracking Jun 5, 2020

ammarkarachi marked this pull request as ready for review June 7, 2020 06:02

ammarkarachi requested a review from attilah June 9, 2020 21:31

attilah suggested changes Jun 11, 2020

View reviewed changes

ammarkarachi force-pushed the feature/telemetry branch from 5b15646 to b934571 Compare June 15, 2020 07:18

ammarkarachi changed the title ~~feat:usage-tracking~~ feat:usage-diagnoses Jun 15, 2020

ammarkarachi requested review from attilah and removed request for undefobj and kaustavghosh06 June 15, 2020 17:34

attilah suggested changes Jun 25, 2020

View reviewed changes

ammarkarachi requested a review from attilah June 25, 2020 17:32

attilah approved these changes Jun 25, 2020

View reviewed changes

feat(cli): usage measurement

2a7d318

ammarkarachi force-pushed the feature/telemetry branch from e45aae2 to 2a7d318 Compare June 25, 2020 21:13

ammarkarachi merged commit 30a7fe7 into aws-amplify:master Jun 25, 2020

ammarkarachi added a commit that referenced this pull request Jun 25, 2020

Revert "feat(cli): usage measurement (#3641)"

7ca1c96

This reverts commit 30a7fe7.

ammarkarachi mentioned this pull request Jun 25, 2020

Revert "feat:usage-diagnoses" #4679

Closed

ammarkarachi added a commit that referenced this pull request Jul 27, 2020

Revert "feat(cli): usage measurement (#3641)"

6a6122b

This reverts commit a755863.

nikhname pushed a commit to nikhname/amplify-cli that referenced this pull request Jul 27, 2020

feat(cli): usage measurement (aws-amplify#3641)

a755863

github-actions bot locked as resolved and limited conversation to collaborators Jun 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat:usage-diagnoses #3641

feat:usage-diagnoses #3641

ammarkarachi commented Mar 10, 2020

lgtm-com bot commented May 19, 2020

codecov bot commented Jun 5, 2020 •

edited

Loading

lgtm-com bot commented Jun 5, 2020

attilah Jun 11, 2020

ammarkarachi Jun 11, 2020 •

edited

Loading

attilah Jun 24, 2020

ammarkarachi Jun 24, 2020

lgtm-com bot commented Jun 15, 2020

lgtm-com bot commented Jun 15, 2020

lgtm-com bot commented Jun 15, 2020

attilah left a comment

attilah Jun 24, 2020

ammarkarachi Jun 25, 2020 •

edited

Loading

ammarkarachi Jun 25, 2020

attilah Jun 25, 2020

ammarkarachi Jun 25, 2020

attilah Jun 25, 2020

ammarkarachi Jun 25, 2020

attilah Jun 25, 2020

ammarkarachi Jun 25, 2020

attilah Jun 25, 2020

ammarkarachi Jun 25, 2020

attilah Jun 25, 2020

ammarkarachi Jun 25, 2020

attilah Jun 25, 2020

ammarkarachi Jun 25, 2020

github-actions bot commented Jun 26, 2021

		@@ -0,0 +1,23 @@
		import semver from 'semver';

		const APIVersionToPayloadVersion = new Map<string, Array<string>>([['v1.0', ['1.0.0']]]);

feat:usage-diagnoses #3641

feat:usage-diagnoses #3641

Conversation

ammarkarachi commented Mar 10, 2020

lgtm-com bot commented May 19, 2020

codecov bot commented Jun 5, 2020 • edited Loading

Codecov Report

lgtm-com bot commented Jun 5, 2020

Choose a reason for hiding this comment

ammarkarachi Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 15, 2020

lgtm-com bot commented Jun 15, 2020

lgtm-com bot commented Jun 15, 2020

attilah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ammarkarachi Jun 25, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 26, 2021

codecov bot commented Jun 5, 2020 •

edited

Loading

ammarkarachi Jun 11, 2020 •

edited

Loading

ammarkarachi Jun 25, 2020 •

edited

Loading