feat(Telemetry): Add `projectId` to payload #10180

pgrzesik · 2021-11-01T14:00:18Z

Reported internally, add projectId to telemetry

Before releasing, related backend PR needs to be merged

codecov · 2021-11-01T14:05:40Z

Codecov Report

Merging #10180 (db9cc19) into master (254e70c) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #10180      +/-   ##
==========================================
- Coverage   85.38%   85.35%   -0.04%     
==========================================
  Files         339      339              
  Lines       13819    13831      +12     
==========================================
+ Hits        11800    11806       +6     
- Misses       2019     2025       +6

Impacted Files	Coverage Δ
lib/plugins/aws/deploy/index.js	`98.68% <100.00%> (+0.05%)`	⬆️
lib/plugins/aws/provider.js	`94.31% <100.00%> (+0.01%)`	⬆️
lib/utils/telemetry/generatePayload.js	`90.90% <100.00%> (+0.43%)`	⬆️
lib/Serverless.js	`66.96% <0.00%> (-2.51%)`	⬇️
lib/plugins/aws/info/index.js	`97.22% <0.00%> (+0.07%)`	⬆️

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 254e70c...db9cc19. Read the comment docs.

medikoo · 2021-11-02T08:59:16Z

lib/utils/telemetry/generatePayload.js

@@ -128,7 +129,7 @@ const getServiceConfig = ({ configuration, options, variableSources }) => {

 // This method is explicitly kept as synchronous. The reason for it being the fact that it needs to
 // be executed in such manner due to its use in `process.on('SIGINT')` handler.
-module.exports = ({
+module.exports = async ({


This method needs to stay sync, see comment you've added above previously :)

Changed the approach 👍

medikoo · 2021-11-02T12:55:05Z

lib/plugins/aws/deploy/index.js

@@ -80,6 +84,12 @@ class AwsDeploy {
              .getStage()} ${style.aside(`(${this.serverless.getProvider('aws').getRegion()})`)}`
          );
          log.info(); // Ensure gap between verbose logging
+
+          try {


This try/catch is ineffective (async functions cannot crash sync way)

What I had in mind, was something as:

this.serverless.provider.getAccountId().then( accountId => this.accountId = accountId, () => { /* Ignore any error */} );

And I wouldn't further handle result promise. I don't think there's any need to do that.
Use case would be, if this request, could take longer than deployment, but I don't think it can be the case

medikoo · 2021-11-02T12:55:22Z

lib/plugins/aws/provider.js

@@ -1437,6 +1437,9 @@ class AwsProvider {
    // Store credentials in this variable to avoid creating them several times (messes up MFA).
    this.cachedCredentials = null;

+    // Store accountId to be used in `generateTelemetry` logic
+    this.cachedAccountId = null;


Why not this.accountId ?

No particular reason other than using similar naming to cachedCredentials above, but I don't have a strong opinion - I'll change it

medikoo · 2021-11-02T15:09:26Z

lib/plugins/aws/deploy/index.js

+          this.serverless
+            .getProvider('aws')
+            .getAccountId()
+            .then((accountId) => (this.serverless.getProvider('aws').accountId = accountId));


this.serverless .getProvider('aws') is accessible at this.provider why not rely on that?

getAccountId() resolves with accountId, why not simply take it from there?

Let's silence eventual rejection (otherwise it'll throw as unhandled in case of e.g. missing credentials)

Good point

I'm not sure what do you mean by that - could you explain?

of course 🤦

I'm not sure what do you mean by that - could you explain?

Sorry, I misread the code, ignore this one

medikoo · 2021-11-02T15:09:52Z

lib/plugins/aws/deploy/index.js

@@ -165,7 +172,7 @@ class AwsDeploy {
        }
      },

-      'finalize': () => {
+      'finalize': async () => {


I think this change is not needed

medikoo · 2021-11-02T15:14:34Z

lib/utils/telemetry/generatePayload.js

@@ -298,6 +299,17 @@ module.exports = ({
    payload.config = getServiceConfig({ configuration, options, variableSources });
    payload.isConfigValid = getConfigurationValidationResult(configuration);
    payload.dashboard.orgUid = serverless && serverless.service.orgUid;
+
+    if (command === 'deploy' && isAwsProvider) {
+      const serviceName = configuration && configuration.service.name;


I think condifugration cannot be falsy here.

Also service name, can be set directly at service I think (e.g. in case where configurationInput is passed)

medikoo · 2021-11-02T15:15:29Z

lib/utils/telemetry/generatePayload.js

+
+    if (command === 'deploy' && isAwsProvider) {
+      const serviceName = configuration && configuration.service.name;
+      const accountId = serverless && serverless.getProvider('aws').accountId;


I would probably put && serverless in main condition, and assume it unconditionally here.

medikoo

Looks great 👍

pgrzesik added enhancement cat/telemetry labels Nov 1, 2021

pgrzesik self-assigned this Nov 1, 2021

pgrzesik requested a review from medikoo November 2, 2021 08:04

medikoo reviewed Nov 2, 2021

View reviewed changes

pgrzesik force-pushed the introduce-project-id-tracking branch from 7b8a241 to 2e79e8d Compare November 2, 2021 11:39

pgrzesik requested a review from medikoo November 2, 2021 11:53

medikoo reviewed Nov 2, 2021

View reviewed changes

pgrzesik force-pushed the introduce-project-id-tracking branch 2 times, most recently from ee00e97 to 6f23ea6 Compare November 2, 2021 14:52

pgrzesik requested a review from medikoo November 2, 2021 15:06

medikoo reviewed Nov 2, 2021

View reviewed changes

feat(Telemetry): Add projectId to payload

db9cc19

pgrzesik force-pushed the introduce-project-id-tracking branch from 6f23ea6 to db9cc19 Compare November 2, 2021 15:29

pgrzesik requested a review from medikoo November 3, 2021 08:28

medikoo approved these changes Nov 3, 2021

View reviewed changes

pgrzesik merged commit cc7d7e4 into master Nov 3, 2021

pgrzesik deleted the introduce-project-id-tracking branch November 3, 2021 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Telemetry): Add `projectId` to payload #10180

feat(Telemetry): Add `projectId` to payload #10180

pgrzesik commented Nov 1, 2021 •

edited

codecov bot commented Nov 1, 2021 •

edited

medikoo Nov 2, 2021

pgrzesik Nov 2, 2021

medikoo Nov 2, 2021

medikoo Nov 2, 2021

pgrzesik Nov 2, 2021

medikoo Nov 2, 2021

pgrzesik Nov 3, 2021

medikoo Nov 3, 2021

medikoo Nov 2, 2021

pgrzesik Nov 3, 2021

medikoo Nov 2, 2021

pgrzesik Nov 3, 2021

medikoo Nov 2, 2021

pgrzesik Nov 3, 2021

medikoo left a comment

feat(Telemetry): Add projectId to payload #10180

feat(Telemetry): Add projectId to payload #10180

Conversation

pgrzesik commented Nov 1, 2021 • edited

codecov bot commented Nov 1, 2021 • edited

Codecov Report

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

medikoo left a comment

Choose a reason for hiding this comment

feat(Telemetry): Add `projectId` to payload #10180

feat(Telemetry): Add `projectId` to payload #10180

pgrzesik commented Nov 1, 2021 •

edited

codecov bot commented Nov 1, 2021 •

edited