Skip to content

Send analytics in background - #7615

Merged
gonzaloriestra merged 1 commit into
mainfrom
background-analytics
Aug 14, 2026
Merged

Send analytics in background#7615
gonzaloriestra merged 1 commit into
mainfrom
background-analytics

Conversation

@gonzaloriestra

@gonzaloriestra gonzaloriestra commented May 22, 2026

Copy link
Copy Markdown
Contributor

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?

Passes the completed analytics payload over stdin to 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:

Version Average time (10 runs)
4.5.0 1.352s
snapshot 0.248s
demo.mp4

How to test your changes?

  • pnpm i -g @shopify/cli@0.0.0-snapshot-20260811121607
  • shopify version --verbose
  • shopify deploy --wrong-flag-analytics-test
  • Check that the payload was processed:
SELECT *
FROM `sdp-ingest.monorail.monorail_app_cli3_command_1`
WHERE event_timestamp >= TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL 1 HOUR) AND CONTAINS_SUBSTR(
    payload.error_message,
    'Nonexistent flag: --wrong-flag-analytics-test'
  )

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've considered analytics changes to measure impact
  • The change is user-facing — I've identified the correct bump type (patch for bug fixes · minor for new features · major for breaking changes) and added a changeset with pnpm changeset add

@gonzaloriestra

Copy link
Copy Markdown
Contributor Author

/snapit

@github-actions github-actions Bot added the no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users. label May 22, 2026
@gonzaloriestra
gonzaloriestra force-pushed the background-analytics branch 3 times, most recently from 972e250 to f72a8d8 Compare May 29, 2026 11:40
@github-actions

Copy link
Copy Markdown
Contributor

This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action.
→ If there's no activity within a week, then a bot will automatically close this.
Thanks for helping to improve Shopify's dev tooling and experience.

@gonzaloriestra
gonzaloriestra force-pushed the background-analytics branch 3 times, most recently from 375d96d to a23f004 Compare July 21, 2026 09:45

gonzaloriestra commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

This was referenced Jul 21, 2026
@gonzaloriestra
gonzaloriestra force-pushed the background-analytics branch 2 times, most recently from 0427849 to 603e207 Compare July 27, 2026 08:32
@gonzaloriestra

Copy link
Copy Markdown
Contributor Author

/snapit

@gonzaloriestra

Copy link
Copy Markdown
Contributor Author

/snapit

@github-actions

Copy link
Copy Markdown
Contributor

🫰✨ 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-20260727091503

Caution

After installing, validate the version by running shopify version in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

@Shopify Shopify deleted a comment from github-actions Bot Jul 27, 2026
@Shopify Shopify deleted a comment from github-actions Bot Jul 27, 2026
@gonzaloriestra
gonzaloriestra marked this pull request as ready for review July 27, 2026 09:24
@gonzaloriestra
gonzaloriestra requested review from a team as code owners July 27, 2026 09:24
Comment thread packages/cli-kit/src/public/node/analytics.ts Outdated
Comment thread packages/cli-kit/src/public/node/analytics.ts Outdated
Comment thread packages/cli-kit/src/public/node/analytics.ts
Comment thread packages/cli-kit/src/public/node/analytics.ts Outdated
Comment thread packages/cli-kit/src/public/node/analytics.ts Outdated
@github-actions github-actions Bot added no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users. and removed Area: @shopify/app @shopify/app package issues labels Aug 11, 2026
@isaacroldan

Copy link
Copy Markdown
Contributor

I think we should replace these command-name and launcher exceptions with command metadata.

For example, add static requiresSyncAnalytics = false to BaseCommand, then set it to true on app init and app dev. We can capture that value in startAnalytics, where the command class is available, and store it with the internal command metadata so that reportAnalyticsEvent can select synchronous delivery.

This also covers app dev stop: it is derived from the app dev command through analyticsStopCommand(), so it can keep the same delivery requirement.

This keeps command lifecycle knowledge on the command and removes the create-app, app dev, and app dev stop checks from the analytics transport.

Copy link
Copy Markdown
Contributor Author

@isaacroldan Good point. That's something we need to improve for other things like notifications, but it's better to do it now. Fixed!

@gonzaloriestra
gonzaloriestra force-pushed the background-analytics branch 2 times, most recently from a610c01 to 91a8307 Compare August 12, 2026 08:39
@isaacroldan

Copy link
Copy Markdown
Contributor

/snapit

@github-actions

Copy link
Copy Markdown
Contributor

🫰✨ Thanks @isaacroldan! 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-20260812085842

Caution

After installing, validate the version by running shopify version in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

@isaacroldan isaacroldan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One stale change remains from the earlier file-based implementation.


🤖 Generated by the review-pr skill

Comment thread packages/cli-kit/src/public/node/fs.ts Outdated

@isaacroldan isaacroldan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more command shutdown paths need synchronous analytics.


🤖 Generated by the review-pr skill

Comment thread packages/cli-kit/src/public/node/analytics.ts Outdated
@gonzaloriestra
gonzaloriestra force-pushed the background-analytics branch 2 times, most recently from 355a0e0 to d8c60f1 Compare August 13, 2026 10:02

@isaacroldan isaacroldan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One remaining issue with background delivery.


🤖 Generated by the review-pr skill

Comment thread packages/cli-kit/src/public/node/analytics.ts Outdated
Comment thread packages/cli-kit/src/public/node/system.ts

@isaacroldan isaacroldan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving! great work, wasn't an easy change. We should keep an eye on analytics after the next release

@github-actions

Copy link
Copy Markdown
Contributor

Differences in type declarations

We 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:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/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/base-command.d.ts
@@ -10,6 +10,7 @@ export interface NonTTYFlagRequirement {
 }
 declare abstract class BaseCommand extends Command {
     static baseFlags: FlagInput<{}>;
+    static get requiresSyncAnalytics(): boolean;
     static nonTTYFlagRequirements(_flags: FlagOutput): NonTTYFlagRequirement[];
     static descriptionWithoutMarkdown(): string | undefined;
     static analyticsNameOverride(): string | undefined;
packages/cli-kit/dist/public/node/metadata.d.ts
@@ -42,6 +42,7 @@ declare const coreData: RuntimeMetadataManager<CmdFieldsFromMonorail, {
         startCommand: string;
         startTopic?: string;
         startArgs: string[];
+        requiresSyncAnalytics?: boolean;
     };
 } & {
     environmentFlags: string;
@@ -63,6 +64,7 @@ export declare const getAllPublicMetadata: () => Partial<CmdFieldsFromMonorail>,
         startCommand: string;
         startTopic?: string;
         startArgs: string[];
+        requiresSyncAnalytics?: boolean;
     };
 } & {
     environmentFlags: string;
@@ -83,6 +85,7 @@ export declare const getAllPublicMetadata: () => Partial<CmdFieldsFromMonorail>,
         startCommand: string;
         startTopic?: string;
         startArgs: string[];
+        requiresSyncAnalytics?: boolean;
     };
 } & {
     environmentFlags: string;
packages/cli-kit/dist/public/node/system.d.ts
@@ -128,6 +128,12 @@ interface WslDetectionOverrides {
  * @returns True if the current environment is a WSL environment.
  */
 export declare function isWsl(overrides?: WslDetectionOverrides): Promise<boolean>;
+/**
+ * Check if the current process is running inside a container.
+ *
+ * @returns True if the current process is running inside a container.
+ */
+export declare function isInsideContainer(): boolean;
 /**
  * Check if stdin has piped data available.
  * This distinguishes between actual piped input (e.g., `echo "query" | cmd`)
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;
 };

@gonzaloriestra
gonzaloriestra added this pull request to the merge queue Aug 14, 2026
Merged via the queue into main with commit 40285a9 Aug 14, 2026
30 checks passed
@gonzaloriestra
gonzaloriestra deleted the background-analytics branch August 14, 2026 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants