Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add VIP_GO_APP_ID to telemetry event data #5922

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

alecgeatches
Copy link
Contributor

Description

This PR extends the recent Tracks telemetry work (#5869, #5921) to include VIP app IDs in a separate field on events. We already utilize app IDs for user identification:

if ( defined( 'VIP_GO_APP_ID' ) ) {
$app_id = constant( 'VIP_GO_APP_ID' );
if ( is_integer( $app_id ) && $app_id > 0 ) {
$event->_ui = sprintf( '%s_%s', $app_id, $wp_user->ID );
$event->_ut = 'vip_go_app_wp_user';
return $event;
}
}

This tracks userids with values in the format {{site_id}}_{{wp_user_id}}. This is less useful for tracking per-app events, since we'd require parsing and preprocessing event data. This PR serves to explicitly track app IDs in a separate data point to make filtering easier. Note that we already track the app environment (e.g. local/develop) as well as organization ID, so this fits right in.

Changelog Description

Changed

  • Added VIP_GO_APP_ID to Tracks telemetry events

Pre-review checklist

Please make sure the items below have been covered before requesting a review:

  • This change works and has been tested locally or in Codespaces (or has an appropriate fallback).
  • This change works and has been tested on a sandbox.
  • This change has relevant unit tests (if applicable).
  • This change uses a rollout method to ease with deployment (if applicable - especially for large scale actions that require writes).
  • This change has relevant documentation additions / updates (if applicable).
  • I've created a changelog description that aligns with the provided examples.

Pre-deploy checklist

  • VIP staff: Ensure any alerts added/updated conform to internal standards (see internal documentation).

Steps to Test

Run unit tests:

./bin/test.sh --filter Tracks_Event_Test

@alecgeatches alecgeatches self-assigned this Oct 11, 2024
@alecgeatches alecgeatches requested a review from a team as a code owner October 11, 2024 17:36
@alecgeatches
Copy link
Contributor Author

I branched these changes from #5921, so probably best to merge those first.

@alecgeatches alecgeatches requested review from sanmai and removed request for a team October 11, 2024 17:38
@mjangda
Copy link
Member

mjangda commented Oct 11, 2024

We already utilize app IDs for user identification

Should we just remove that block? On VIP, the user id should be set based on the hashed email (the VIP_TELEMETRY_SALT block above).

Copy link
Member

@mjangda mjangda left a comment

Choose a reason for hiding this comment

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

Left some suggestions on this.

It would be nice to cleanup the other props as well (i.e. change any references of vipgo_ to vip_ (e.g. vipgo_env), although I realize that is increasing the scope of this PR :)

Separately, what do you think about adding the home or site URL as a prop? (vip_home_url). The ID is helpful, but the URL would make sorting through data easier.

// Set VIP organization if it exists.
if ( defined( 'VIP_ORG_ID' ) ) {
$org_id = constant( 'VIP_ORG_ID' );
if ( is_string( $org_id ) && '' !== $org_id ) {
if ( is_integer( $org_id ) && $org_id > 0 ) {
$event->vipgo_org = $org_id;
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the app change, let's rename this:

Suggested change
$event->vipgo_org = $org_id;
$event->vip_org_id = $org_id;

telemetry/tracks/class-tracks-event.php Outdated Show resolved Hide resolved
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 30.29%. Comparing base (4f8794e) to head (1a1c445).

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #5922      +/-   ##
=============================================
+ Coverage      30.26%   30.29%   +0.03%     
- Complexity      4812     4815       +3     
=============================================
  Files            287      287              
  Lines          20707    20699       -8     
=============================================
+ Hits            6267     6271       +4     
+ Misses         14440    14428      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sanmai sanmai requested a review from mjangda October 15, 2024 00:37
Copy link
Member

@sanmai sanmai left a comment

Choose a reason for hiding this comment

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

It sounds like we addressed all @mjangda's suggestions.

Copy link

sonarcloud bot commented Oct 15, 2024

@sanmai sanmai requested a review from a team October 15, 2024 00:38
@sanmai
Copy link
Member

sanmai commented Oct 15, 2024

WP nightly tests are broken, probably unrelated to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants