refactor(Telemetry): Ensure to report projectId for interactive
#10406
Conversation
pgrzesik
force-pushed
the
report-stack-creation
branch
from
fbba60c
to
437fd10
Compare
Codecov Report
@@ Coverage Diff @@
## master #10406 +/- ##
==========================================
- Coverage 85.37% 85.37% -0.01%
==========================================
Files 340 340
Lines 14005 14011 +6
==========================================
+ Hits 11957 11962 +5
- Misses 2048 2049 +1
Continue to review full report at Codecov.
|
pgrzesik
force-pushed
the
report-stack-creation
branch
from
437fd10
to
dfb9b2d
Compare
scripts/serverless.js
Outdated
| @@ -558,6 +559,7 @@ const processSpanPromise = (async () => { | |||
| configuration, | |||
| commandUsage, | |||
| variableSources: variableSourcesInConfig, | |||
| interactiveContext, | |||
There was a problem hiding this comment.
Wouldn't it be cleaner to expose serverless instance on interactive CLI context, and pass it to .generateTelemetryPayload (having that we probably don't have to make any new changes in generateTelemetryPayload)?
There was a problem hiding this comment.
That's definitely an option and might be a better solution overall 👍
pgrzesik
force-pushed
the
report-stack-creation
branch
from
dfb9b2d
to
2d2d8de
Compare
| if (serviceName && accountId) { | ||
| payload.projectId = crypto | ||
| .createHash('sha256') | ||
| .update(`${serviceName}-${accountId}`) | ||
| .digest('base64'); | ||
| } | ||
|
|
||
| payload.didCreateStack = Boolean(serverless && serverless.getProvider('aws').didCreateStack); |
There was a problem hiding this comment.
Slightly related: we mentioned the stack deletion as well, and we said "we can look at remove commands" -> but these don't have the projectId property in those events. Should we add command === 'remove' above in the if too? (or maybe in a separate PR if that's cleaner)
There was a problem hiding this comment.
That's a great point - I will add that in a separate PR
scripts/serverless.js
Outdated
| @@ -514,6 +514,7 @@ const processSpanPromise = (async () => { | |||
| const isStandaloneCommand = notIntegratedCommands.has(command); | |||
|
|
|||
| if (!isHelpRequest && (isInteractiveSetup || isStandaloneCommand)) { | |||
| let interactiveContext; | |||
There was a problem hiding this comment.
It'll be nicer to reuse the top level serverless variable (same as we reuse configuration one).
pgrzesik
force-pushed
the
report-stack-creation
branch
from
2d2d8de
to
7bb89c9
Compare
| if (serviceName && accountId) { | ||
| payload.projectId = crypto | ||
| .createHash('sha256') | ||
| .update(`${serviceName}-${accountId}`) | ||
| .digest('base64'); | ||
| } | ||
|
|
||
| payload.didCreateStack = Boolean(serverless && serverless.getProvider('aws').didCreateStack); |
There was a problem hiding this comment.
In platform PR I've suggested that maybe it'll be better to give it a more agnostic name as didCreateService, so if eventually later we will support non-CF-related deployments, this name stays accurate.
WDYT?
There was a problem hiding this comment.
Not sure if that applies to the metric name as well, but for the metric name +1 to have it a bit more generic
There was a problem hiding this comment.
Great suggestion @medikoo 👍
pgrzesik
force-pushed
the
report-stack-creation
branch
from
7bb89c9
to
ccc214b
Compare
Reported internally:
projectIdfor interactive flow deployments