Automattic / studio

Studio by WordPress.com, a free desktop app that helps developers streamline their local WordPress development workflow.
https://developer.wordpress.com/studio/
GNU General Public License v2.0
138 stars 12 forks source link

Iterate on launch stats, to include tracking for all and first-time launches #221

Closed SiobhyB closed 2 weeks ago

SiobhyB commented 2 weeks ago

Related to 7578-gh-Automattic/dotcom-forge

Proposed Changes

[!NOTE] I'm keen to hear thoughts around naming. I've spent a bit of time looking closely at this problem, so newer eyes will likely better spot any names that could be made clearer.

Testing Instructions

diff --git a/src/lib/bump-stats.ts b/src/lib/bump-stats.ts
index 8154cec..05bd77c 100644
--- a/src/lib/bump-stats.ts
+++ b/src/lib/bump-stats.ts
@@ -49,12 +49,15 @@ export function bumpAggregatedUniqueStat(
 }

 // Returns true if we attempted to bump the stat
+// eslint-disable-next-line @typescript-eslint/no-unused-vars
 export function bumpStat( group: string, stat: string, bumpInDev = false ) {
-   if ( process.env.E2E || ( process.env.NODE_ENV === 'development' && ! bumpInDev ) ) {
+   if ( process.env.E2E ) {
        console.info( `Would have bumped stat: ${ group }=${ stat }` );
        return false;
    }

+   console.log( `Bumped stat: ${ group }=${ stat }` );
+
    const url = new URL( 'https://pixel.wp.com/b.gif?v=wpcom-no-pv' );
    url.searchParams.append( `x_${ group }`, stat );

[!NOTE] An MC page for each stat will be automatically created, but you may not see your stat reflected immediately.

Pre-merge Checklist

danielbachhuber commented 2 weeks ago

Will this break our existing stat? If so, I'd prefer to keep the existing naming so we have the continuity of data in place.

SiobhyB commented 2 weeks ago

Will this break our existing stat? If so, I'd prefer to keep the existing naming so we have the continuity of data in place.

@danielbachhuber, it wouldn't break the existing stat, no. There's a process for renaming MC stats outlined in the FG, though we'd just need to be careful about timing if we go ahead with that. I've added a [Status] Not Ready for Merge label so folks know not to merge this PR before we're ready.

I'm also keen to hear thoughts or other suggestions on the naming of the stats. I obviously don't want to get in the way if the current weekly stat name works, but now feels like a good opportunity for reviewing and making sure it's as clear as possible.

danielbachhuber commented 2 weeks ago

@SiobhyB Sounds good, thanks for clarifying

wojtekn commented 2 weeks ago

I've added a [Status] Not Ready for Merge label so folks know not to merge this PR before we're ready.

If we have a process for renaming, it would be good to do so, as it would keep those names consistent and avoid confusion. However, as Studio is an app with a release schedule, and there is a possibility that users won't upgrade it, we may have both old and new stats being logged in parallel. Will the rename still work in our case?

SiobhyB commented 2 weeks ago

However, as Studio is an app with a release schedule, and there is a possibility that users won't upgrade it, we may have both old and new stats being logged in parallel. Will the rename still work in our case?

@wojtekn, that's a good point, my assumption was that we'd be able to set up a redirect for the old http://pixel.wp.com/b.gif... URL, but I'll ask about to confirm whether this is possible and follow up.

SiobhyB commented 2 weeks ago

@wojtekn, it seems that the most straightforward approach to renaming would be to keep going back to merge the old stats. As I can't commit to this at the moment, due to my upcoming sabbatical, I've gone ahead to revert back to the old name in https://github.com/Automattic/studio/pull/221/commits/5e9a33c8fef39dca7c7430af49839cf610b39bd0. So, now this PR proposes the following:

Looking forward to hearing throughts!

I'll also create a separate GitHub issue to detail what needs to be done to rename local-environment-launch-uniques, in case someone else would want to pick that up. I see we can add descriptions at the top of the MC stat pages, so will do that too.

Slack discussion related to renaming for context: p1718037250491319-slack-C029GN3KD

danielbachhuber commented 2 weeks ago

I'm personally fine with the separate naming. I don't think it's worth the engineering effort to try to standardize.

wojtekn commented 2 weeks ago

Thanks, @SiobhyB, for exploring the possibilities. I agree with @danielbachhuber's assessment that it makes more sense to keep that simple with inconsistent naming when compared to maintaining the name change over time.