Send analytics in background - #7615
Conversation
|
/snapit |
972e250 to
f72a8d8
Compare
f72a8d8 to
ddc22d9
Compare
|
This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. |
375d96d to
a23f004
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
0427849 to
603e207
Compare
|
/snapit |
603e207 to
f022390
Compare
|
/snapit |
|
🫰✨ Thanks @gonzaloriestra! Your snapshot has been published to npm. Test the snapshot by installing your package globally: pnpm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260727091503Caution After installing, validate the version by running |
f022390 to
aed6ae1
Compare
65e2599 to
4990f80
Compare
| prompt, | ||
| }, | ||
| ) | ||
| await writeFile(payloadPath, JSON.stringify(fullPayload), {encoding: 'utf8', mode: 0o600, flag: 'wx'}) |
There was a problem hiding this comment.
this is writing sensitive data to a file... there is a small posibility that the clenaup fails, or the send-analytics invocation fails... something fails and this file is never deleted.
What can we do to prevent that? 🤔
There was a problem hiding this comment.
In theory we delete even on failure paths.
But if that fails, we are reducing the risk by:
- Creating the file in the tmp folder, that should eventually be removed
- Setting the file permissions so that only the user running the CLI can access it (0600)
- Using a random filename
Do you think it's enough? Alternatively, we could try to encrypt or pass the data in a different way.
There was a problem hiding this comment.
Another option is to clean all the CLI payloads after every run (or old ones at least)
There was a problem hiding this comment.
um, cleaning all files wouldn't be compatible with multiple CLIs running in different terminals (one might delete another's file).
maybe ask Claude/Codex if there is reliable way to encrypt? if not I guess there isn't much we can do
There was a problem hiding this comment.
Yeah, that's why I suggested to only remove the old ones.
Encrypting looks complex because we need to keep a key somehow. But let me try by passing the data through stdin instead.
f8326f2 to
853bb21
Compare
853bb21 to
f4834c1
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/private/node/otel-metrics.d.ts@@ -2,7 +2,7 @@ import { OtelService } from '../../public/node/vendor/otel-js/service/types.js';
import { DefaultOtelServiceOptions } from '../../public/node/vendor/otel-js/service/DefaultOtelService/DefaultOtelService.js';
type MetricRecorder = 'console' | {
type: 'otel';
- otel: Pick<OtelService, 'record'>;
+ otel: Pick<OtelService, 'getMeterProvider' | 'record'>;
};
interface Timing {
active: number;
packages/cli-kit/dist/public/node/analytics.d.ts@@ -6,6 +6,7 @@ interface ReportAnalyticsEventOptions {
errorMessage?: string;
exitMode: CommandExitMode;
}
+export declare function sendAnalyticsEventFromStdin(): Promise<void>;
/**
* Report an analytics event, sending it off to Monorail -- Shopify's internal analytics service.
*
packages/cli-kit/dist/public/node/fs.d.ts@@ -86,6 +86,8 @@ export declare function appendFile(path: string, content: string): Promise<void>
export declare function appendFileSync(path: string, data: string): void;
export interface WriteOptions {
encoding: BufferEncoding;
+ mode?: number;
+ flag?: string;
}
/**
* Writes content to file at path.
packages/cli-kit/dist/public/node/vendor/otel-js/service/types.d.ts@@ -1,5 +1,5 @@
-import type { Counter, Histogram, MeterProvider, MetricAttributes, MetricOptions, UpDownCounter } from '@opentelemetry/api';
-import type { ViewOptions } from '@opentelemetry/sdk-metrics';
+import type { Counter, Histogram, MetricAttributes, MetricOptions, UpDownCounter } from '@opentelemetry/api';
+import type { MeterProvider, ViewOptions } from '@opentelemetry/sdk-metrics';
export type CustomMetricLabels<TLabels extends Record<TKeys, MetricAttributes>, TKeys extends string = keyof TLabels & string> = {
[P in TKeys]: TLabels[P] extends MetricAttributes ? TLabels[P] : never;
};
|

WHY are these changes introduced?
HackDays project: https://vault.shopify.io/hackdays/154/projects/24279-Shopify-CLI-UX-improvements
Analytics delivery currently blocks command completion, adding noticeable latency to every CLI invocation.
WHAT is this pull request doing?
Writes the completed analytics payload to a temporary file and sends it from a detached internal command, allowing the original command to exit immediately.
This works for Mac and Linux, but on Windows we still have to wait for the process because of a known issue. We also wait on CI to avoid issues with the job teardown.
Benchmark for
shopify version:4.5.0snapshotdemo.mp4
How to test your changes?
pnpm i -g @shopify/cli@0.0.0-snapshot-20260727091503shopify version --verboseChecklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add