Add MC stats for import / export to track usage by bcotrim · Pull Request #983 · Automattic/studio

bcotrim · 2025-02-26T13:11:04Z

Related issues

Fixes https://github.com/Automattic/dotcom-forge/issues/9027

Proposed Changes

Add typed constants for stats groups and metrics
Replace string literals with typed constants in bump-stats functions
Update tests to use the new typed constants
Add stats tracking for import and export operations (success/failure)
Added tracking for import file types
Added tracking for export content types

Testing Instructions

Run the test suite to verify that the bump-stats tests pass with the new typed implementation
Verify that stats are properly tracked for import and export operations:
- Run the app in development mode and check the console logs for "Would have bumped stat: studio-app-import=success" when importing a site
- Run the app in development mode and check the console logs for bumped stats when exporting a site. For example
- Verify that failure stats are logged when operations fail
Check that app launch stats continue to work as expected:
- Run the app in development mode and verify console logs show "Would have bumped stat: studio-app-launch-total=darwin" (or your platform) on startup
- Verify weekly unique stats are logged with "Would have bumped stat: local-environment-launch-uniques=darwin"
Confirm that the TypeScript type safety prevents using invalid stat groups or metrics

Example Testing logs

 Would have bumped stat: studio-app-export=success
 Would have bumped stat: studio-app-export-content=database
 Would have bumped stat: studio-app-export-content=uploads
 Would have bumped stat: studio-app-export-content=plugins
 Would have bumped stat: studio-app-export-content=themes
 Would have bumped stat: studio-app-export-content=mu-plugins

Would have bumped stat: studio-app-import=success
Would have bumped stat: studio-app-import-type=application/zip

Pre-merge Checklist

Have you checked for TypeScript, React or other console errors?

sejas

Code looks good.
I suggest tracking clicks on the export buttons to make the underlying logic easier to understand. Export entire site vs Export database.

sejas · 2025-02-27T10:02:20Z

+			// Track which content types are being included in the export
+			if ( options.includes ) {
+				if ( options.includes.database ) {
+					bumpStat( STATS_GROUP.STUDIO_EXPORT_CONTENT, STATS_METRIC.DATABASE );
+				}
+				if ( options.includes.uploads ) {
+					bumpStat( STATS_GROUP.STUDIO_EXPORT_CONTENT, STATS_METRIC.UPLOADS );
+				}
+				if ( options.includes.plugins ) {
+					bumpStat( STATS_GROUP.STUDIO_EXPORT_CONTENT, STATS_METRIC.PLUGINS );
+				}
+				if ( options.includes.themes ) {
+					bumpStat( STATS_GROUP.STUDIO_EXPORT_CONTENT, STATS_METRIC.THEMES );
+				}
+				if ( options.includes.muPlugins ) {
+					bumpStat( STATS_GROUP.STUDIO_EXPORT_CONTENT, STATS_METRIC.MU_PLUGINS );
+				}
+			}


I don't think we need this granularity in the export.
We have only two buttons in the UI.
One to export the whole site and another to export the database.
What if we expose the bumpStat to ipc-handlers and call it onClick on the UI?

studio/src/components/content-tab-import-export.tsx

Lines 77 to 94 in edf000c

<Button

onClick={ () => handleExport( exportFullSite ) }

variant="primary"

disabled={ isExportDisabled }

>

{ __( 'Export entire site' ) }

</Button>

<Button

onClick={ () => handleExport( exportDatabase ) }

type="submit"

variant="secondary"

className={ cx(

isExportDisabled ? '' : '!text-a8c-blueberry !shadow-a8c-blueberry'

) }

disabled={ isExportDisabled }

>

{ __( 'Export database' ) }

</Button>

sejas · 2025-02-27T10:10:15Z

+		bumpStat( STATS_GROUP.STUDIO_IMPORT, STATS_METRIC.SUCCESS );
+		bumpStat( STATS_GROUP.STUDIO_IMPORT_TYPE, backupFile.type );


I believe backupFile.type will indicate whether the file is a zip, SQL, or tar.gz format. However, it might be more interesting to differentiate between jetpack, playground, local, etc.

fredrikekelund · 2025-02-28T14:55:14Z

Heads up that I just landed #988 which modified this file in its previous location. Should be an easy conflict to resolve

bcotrim · 2025-02-28T15:32:19Z

@sejas @fredrikekelund Thanks for the reviews!
I've made some changes to address the comments, please let me know what you think!

fredrikekelund

Using typed arguments for bumpStat functions is great, and the code mostly works well, but I left a few we should adress before merging.

fredrikekelund · 2025-03-05T14:57:52Z


 	const exportFullSite = useCallback(
 		async ( selectedSite: SiteDetails ): Promise< string | undefined > => {
+			getIpcApi().ipcBumpStat( STATS_GROUP.STUDIO_EXPORT, STATS_METRIC.FULL_SITE );


Could we submit the bump stat from exportSite in src/ipc-handlers.ts instead? Aside from making it so we don't have to expose another IPC method, there's also an issue with submitting the bump stat here because it happens before the actual export operation. It's probably not very common for users to click the export button and then click Cancel in the Save as… dialog, but if they do, we still submit a bump stat right now.

Tracking the event in ipc handler was my initial approach, changed later by @sejas suggestion.
My suggestion would be to actually use both. On the UI side we can track the user click and action, on the ipc handler we track the success/failure of the request.
What do you think @sejas @fredrikekelund ?

Hmm, tracking both operations seems like overkill to me. If we want to understand which errors users encounter (if any), Sentry is a better tool than bump stats.

I think your original implementation is clearer, even if I also agree with @sejas that we don't need that much granularity when submitting the bump stat. It's fine to distinguish only between full site exports and database exports, but we could do that in the exportSite function by comparing the full shape of options.includes

I moved all the logic to the IPC now.
The following stats should be bumped now.

Import - studio-app-import group

importer type if the request is successful

failure if the request is unsuccessful

export - studio-app-export

full-site or database-only if successful and depending on the options

failure if the request is unsuccessful

I believe this will allow us to have an idea of usage, while still providing some insights into the number of failed requests.

@sejas @fredrikekelund please let me know what you think about this approach.

fredrikekelund · 2025-03-05T14:58:32Z

-		return await importer.import( site.path, site.id );
+		const result = await importer.import( site.path, site.id );
+		return result;


This looks like a leftover, we could probably revert it 🙂

Thanks for catching that!

fredrikekelund · 2025-03-05T15:04:33Z

+		bumpStat( STATS_GROUP.STUDIO_IMPORT, STATS_METRIC.SUCCESS );
+		bumpStat( STATS_GROUP.STUDIO_IMPORT, result.importerType || 'unknown' );


Do we intentionally use the same group name for both these calls?

I did.
I wasn't sure about the best approach for this, should we have everything regarding import in the same group and track success, failure and type, or have a distinct group for each, like import_result and import_type. What do you think it's the best?

fredrikekelund · 2025-03-05T15:08:33Z

Adding explicit types for bump groups and metrics like this is great 👍 I would argue enums are a great fit here, though.

This seems like a relatively simple change aside from that we have to fix the importSite function in ipc-handlers, where we're currently passing a dynamic value as the second argument to bumpStat. Maybe we could add a bumpStat prop to the ImporterResult type to fix this…

Consider this a recommendation, not a hard blocker.

Done, let me know how it looks!

fredrikekelund · 2025-03-06T09:17:19Z

@@ -0,0 +1,29 @@
+export enum STATS_GROUP {


Suggested change

export enum STATS_GROUP {

export enum StatsGroup {

The nice thing about enums is that we get types and values kind of rolled into one. No need to export a separate StatsGroup type.

fredrikekelund · 2025-03-06T09:52:33Z

+	PLAYGROUND_IMPORTER = 'PlaygroundImporter',
+	WPRESS_IMPORTER = 'WpressImporter',
+	UNKNOWN = 'unknown',
+}


Suggested change

}

DARWIN = 'darwin',

LINUX = 'linux',

WINDOWS = 'win32',

}

export function getPlatform(): StatsMetric {

switch ( process.platform ) {

case 'darwin':

return STATS_METRIC.DARWIN;

case 'linux':

return STATS_METRIC.LINUX;

case 'win32':

return STATS_METRIC.WINDOWS;

default:

throw new Error( `Unsupported platform: ${ process.platform }` );

}

}

Something like this would allow us not to export a separate type for StatsMetric. Kind of a nit, but would be nice to address, IMO.

fredrikekelund · 2025-03-06T09:54:11Z

 				wpContentDirectory: this.backup.wpContentDirectory,
 				wpConfig: this.backup.wpConfig,
 				meta: this.meta,
+				importerType: this.constructor.name,


Suggested change

importerType: this.constructor.name,

importerType: StatsMetric.SQL_IMPORTER,

Defining the value by referencing the enum directly seems more understandable.

I find it strange to reference stats props in the importer class.
What do you think about using a similar approach to the one we implemented for getPlatform ( check getImporterMetric )?

I like the new approach 👍

fredrikekelund

Nice! Looks good and works as expected 👍

fredrikekelund · 2025-03-06T14:43:22Z

+			bumpStat(
+				StatsGroup.STUDIO_EXPORT,
+				options.includes.database &&
+					! options.includes.uploads &&
+					! options.includes.plugins &&
+					! options.includes.themes &&
+					! options.includes.muPlugins
+					? StatsMetric.DATABASE_ONLY
+					: StatsMetric.FULL_SITE
+			);


Suggested change

bumpStat(

StatsGroup.STUDIO_EXPORT,

options.includes.database &&

! options.includes.uploads &&

! options.includes.plugins &&

! options.includes.themes &&

! options.includes.muPlugins

? StatsMetric.DATABASE_ONLY

: StatsMetric.FULL_SITE

);

const isDatabaseOnly =

options.includes.database &&

! options.includes.uploads &&

! options.includes.plugins &&

! options.includes.themes &&

! options.includes.muPlugins;

bumpStat(

StatsGroup.STUDIO_EXPORT,

isDatabaseOnly ? StatsMetric.DATABASE_ONLY : StatsMetric.FULL_SITE

);

Nit, but this would read better if we assigned the condition to a variable.

fredrikekelund · 2025-03-06T14:45:07Z

+		case 'JetpackImporter':
+			return StatsMetric.JETPACK_IMPORTER;
+		case 'LocalImporter':
+			return StatsMetric.LOCAL_IMPORTER;
+		case 'SQLImporter':
+			return StatsMetric.SQL_IMPORTER;
+		case 'PlaygroundImporter':
+			return StatsMetric.PLAYGROUND_IMPORTER;
+		case 'WpressImporter':


Suggested change

case 'JetpackImporter':

return StatsMetric.JETPACK_IMPORTER;

case 'LocalImporter':

return StatsMetric.LOCAL_IMPORTER;

case 'SQLImporter':

return StatsMetric.SQL_IMPORTER;

case 'PlaygroundImporter':

return StatsMetric.PLAYGROUND_IMPORTER;

case 'WpressImporter':

case JetpackImporter.name:

return StatsMetric.JETPACK_IMPORTER;

case LocalImporter.name:

return StatsMetric.LOCAL_IMPORTER;

case SQLImporter.name:

return StatsMetric.SQL_IMPORTER;

case PlaygroundImporter.name:

return StatsMetric.PLAYGROUND_IMPORTER;

case WpressImporter.name:

Optional nit, but this would help clarify what we're referencing.

Nice, that's perfect! 👍

fredrikekelund · 2025-03-06T14:48:01Z

 				wpContentDirectory: this.backup.wpContentDirectory,
 				wpConfig: this.backup.wpConfig,
 				meta: this.meta,
+				importerType: this.constructor.name,


I like the new approach 👍

bcotrim · 2025-03-07T10:35:12Z

@sejas since you created the original issue and made some suggestion for different approaches I would love to have your feedback. Could I ask you for another review? 🙇

sejas

@bcotrim , looks great! thanks for making the changes.
I left a couple of comments, like removing the error in getPlatformMetric and the result check in export site.

I tested it by running npm start and observing the the bumpStat logs:

Would have bumped stat: studio-app-export=full-site
Would have bumped stat: studio-app-export=database-only
Would have bumped stat: studio-app-import=SQLImporter
Would have bumped stat: studio-app-import=JetpackImporter
Would have bumped stat: studio-app-import=PlaygroundImporter

sejas · 2025-03-07T11:31:54Z

 import { loadUserData, saveUserData } from 'src/storage/user-data';
-
-export type AggregateInterval = 'daily' | 'weekly' | 'monthly';
+import { AggregateInterval, StatsGroup, StatsMetric } from './types';


Should we use an absolute path?

Suggested change

import { AggregateInterval, StatsGroup, StatsMetric } from './types';

import { AggregateInterval, StatsGroup, StatsMetric } from 'src/lib/bump-stats/types';

sejas · 2025-03-07T11:42:43Z

+		default:
+			throw new Error( `Unsupported platform: ${ process.platform }` );


Not sure if this Error could crash the app in some systems. It's not very likely but maybe some unix systems return a different platform string. Maybe returning 'unknown' is safer?

sejas · 2025-03-07T11:51:30Z

+
+		const result = await exportBackup( options, onEvent );
+
+		if ( result ) {


Why do we need this result check? Or if we leave it should we consider the else as a failure?

bcotrim requested a review from a team February 26, 2025 13:11

bcotrim marked this pull request as draft February 26, 2025 13:16

bcotrim marked this pull request as ready for review February 26, 2025 15:25

sejas reviewed Feb 27, 2025

View reviewed changes

bcotrim force-pushed the add/import_export_tracks branch from d35d9f3 to a7ff1b5 Compare February 28, 2025 13:27

fredrikekelund reviewed Feb 28, 2025

View reviewed changes

bcotrim added 9 commits February 28, 2025 15:09

add bump stats to import/export

76160ba

success and failure tracking

286981b

add bump stats types

ff7f92c

add more stats

0a9e15b

fix & improve tests

576fcb5

cleanup metrics type

83f1a6a

add bumpstat to ipc and update export calls

1eb9d09

add bumpstat to ipc and update export calls

8635700

add changes to bump stat

6750cca

bcotrim force-pushed the add/import_export_tracks branch from b6dfbdc to 6750cca Compare February 28, 2025 15:12

change import type tracking metrics

c1ef719

cleanup bump stat metrics type

9f282c8

bcotrim requested review from fredrikekelund and sejas February 28, 2025 15:40

bcotrim changed the title ~~Add/import export tracks~~ Feb 28, 2025

fredrikekelund reviewed Mar 5, 2025

View reviewed changes

bcotrim added 2 commits March 5, 2025 17:30

cleanup

0151f7c

change stats metrics types to enum

7af766d

fredrikekelund reviewed Mar 6, 2025

View reviewed changes

bcotrim added 3 commits March 6, 2025 10:23

move all bump stats logic to the ipc

27cf741

fix tests and lint

2554f8b

improve bump stats types

50602ee

fredrikekelund approved these changes Mar 6, 2025

View reviewed changes

improve types

d931f32

sejas approved these changes Mar 7, 2025

View reviewed changes

pr feedback

733725c

bcotrim merged commit b336da7 into trunk Mar 7, 2025

bcotrim deleted the add/import_export_tracks branch March 7, 2025 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add MC stats for import / export to track usage - #983

Add MC stats for import / export to track usage#983
bcotrim merged 18 commits into
trunkfrom
add/import_export_tracks

bcotrim commented Feb 26, 2025 •

edited

Loading

sejas left a comment

sejas Feb 27, 2025

sejas Feb 27, 2025 •

edited

Loading

fredrikekelund Feb 28, 2025

bcotrim commented Feb 28, 2025

fredrikekelund left a comment

fredrikekelund Mar 5, 2025

bcotrim Mar 5, 2025

fredrikekelund Mar 6, 2025

bcotrim Mar 6, 2025

fredrikekelund Mar 5, 2025

bcotrim Mar 5, 2025

fredrikekelund Mar 5, 2025

bcotrim Mar 5, 2025

fredrikekelund Mar 5, 2025

bcotrim Mar 5, 2025

fredrikekelund Mar 6, 2025

fredrikekelund Mar 6, 2025

fredrikekelund Mar 6, 2025

bcotrim Mar 6, 2025

fredrikekelund Mar 6, 2025

fredrikekelund left a comment

fredrikekelund Mar 6, 2025

fredrikekelund Mar 6, 2025

bcotrim Mar 6, 2025

fredrikekelund Mar 6, 2025

bcotrim commented Mar 7, 2025

sejas left a comment

sejas Mar 7, 2025

sejas Mar 7, 2025

sejas Mar 7, 2025 •

edited

Loading

Labels

3 participants

	<Button
	onClick={ () => handleExport( exportFullSite ) }
	variant="primary"
	disabled={ isExportDisabled }
	>
	{ __( 'Export entire site' ) }
	</Button>
	<Button
	onClick={ () => handleExport( exportDatabase ) }
	type="submit"
	variant="secondary"
	className={ cx(
	isExportDisabled ? '' : '!text-a8c-blueberry !shadow-a8c-blueberry'
	) }
	disabled={ isExportDisabled }
	>
	{ __( 'Export database' ) }
	</Button>

		bumpStat( STATS_GROUP.STUDIO_IMPORT, STATS_METRIC.SUCCESS );
		bumpStat( STATS_GROUP.STUDIO_IMPORT_TYPE, backupFile.type );

		bumpStat( STATS_GROUP.STUDIO_IMPORT, STATS_METRIC.SUCCESS );
		bumpStat( STATS_GROUP.STUDIO_IMPORT, result.importerType \|\| 'unknown' );

-}
+	DARWIN = 'darwin',
+	LINUX = 'linux',
+	WINDOWS = 'win32',
+}
+export function getPlatform(): StatsMetric {
+	switch ( process.platform ) {
+		case 'darwin':
+			return STATS_METRIC.DARWIN;
+		case 'linux':
+			return STATS_METRIC.LINUX;
+		case 'win32':
+			return STATS_METRIC.WINDOWS;
+		default:
+			throw new Error( `Unsupported platform: ${ process.platform }` );
+	}
+}

	importerType: this.constructor.name,
	importerType: StatsMetric.SQL_IMPORTER,

	import { AggregateInterval, StatsGroup, StatsMetric } from './types';
	import { AggregateInterval, StatsGroup, StatsMetric } from 'src/lib/bump-stats/types';

		default:
		throw new Error( `Unsupported platform: ${ process.platform }` );


		const result = await exportBackup( options, onEvent );

		if ( result ) {

Uh oh!

Conversation

bcotrim commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

Proposed Changes

Testing Instructions

Example Testing logs

Pre-merge Checklist

sejas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sejas Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcotrim commented Feb 28, 2025

fredrikekelund left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekelund left a 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

bcotrim commented Mar 7, 2025

sejas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sejas Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Labels

3 participants

bcotrim commented Feb 26, 2025 •

edited

Loading

sejas Feb 27, 2025 •

edited

Loading

sejas Mar 7, 2025 •

edited

Loading