Add base telemetry library #5770

hanifn · 2024-08-05T10:03:10Z

Description

This PR adds a new generic Telemetry library consisting of base classes for Automattic's Tracks system integration. This is based heavily on the existing Telemetry package under the vip-parsely directory but meant to be more generic and can be used by other plugins.

Changelog Description

Added

Added new Telemetry lib for A8C Tracks integration

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

This is a base library so no concrete implementation to test yet.

codecov · 2024-08-05T10:19:05Z

Codecov Report

Attention: Patch coverage is 33.33333% with 158 lines in your changes missing coverage. Please review.

Project coverage is 29.70%. Comparing base (f18c1d1) to head (51802ea).

Files with missing lines	Patch %	Lines
telemetry/tracks/class-tracks-client.php	0.00%	96 Missing ⚠️
telemetry/tracks/class-tracks-event.php	58.77%	54 Missing ⚠️
telemetry/tracks/class-tracks.php	25.00%	6 Missing ⚠️
telemetry/class-telemetry-system.php	0.00%	2 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop    #5770      +/-   ##
=============================================
+ Coverage      29.64%   29.70%   +0.06%     
- Complexity      4760     4817      +57     
=============================================
  Files            281      285       +4     
  Lines          20534    20759     +225     
=============================================
+ Hits            6087     6167      +80     
- Misses         14447    14592     +145

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

rinatkhaziev · 2024-08-05T19:35:25Z

telemetry/Tracks/class-tracks-client.php

+	 */
+	public function __construct() {
+		// Track events asynchronously (inject pixels in the footer).
+		add_action( 'admin_footer', array( $this, 'render_tracking_pixels' ) );


This only tracks WP Admin requests, is that intentional?
for frontend we'd need to use wp_footer

Yes that is intentional. We're limiting this to WP admin for now. I'm also still trying to figure out a good way to make this part flexible so that where it renders the pixel is determined by the plugin implementation 🤔

Why would it be consequential to the plugins?

It'd be worth expanding this to the frontend as well imo. If we wanted, we could add a marker in the event to say when it's frontend or WP Admin and target the event recording based on that but I don't think it's worth it?

telemetry/Tracks/class-tracks-client.php

rinatkhaziev · 2024-08-05T19:50:33Z

Besides the inline feedback, let's drop go from vipgo.
Needs tests

alecgeatches · 2024-08-06T15:33:23Z

telemetry/Tracks/class-tracks-client.php

+	/**
+	 * Outputs a Tracks pixel for every registered event.
+	 */
+	public function render_tracking_pixels(): void {


@hanifn How are the actual pixel-style tracking pixels used? I'm more used to the server-side only pixel requests, so just want to understand. Is it:

Detect I'm on a page where I want to track something e.g. $pagenow === 'edit.php'

Call register_events( [ new Tracks_Event( 'edit_loaded', ... ) ] ) during page loads

Pixels automatically fire off on browser render

Is that correct?

@alecgeatches Yes that is about how it works although the register_events method no longer exist now. Any events that are triggered after the pixel rendered will be handled during the shutdown action by the record_remaining_events method

alecgeatches

Looks good to me! Agree with Rinat that tests would be great, especially as a demo of how to build an event and fire it off.

telemetry/Tracks/class-tracks-client.php

mjangda

Thanks for working on this @hanifn. I've added some comments / questions.

mjangda · 2024-08-07T05:54:39Z

telemetry/Tracks/class-tracks-client.php

+		$pixel_url = static::instance()->generate_pixel_url( $event );
+
+		if ( null === $pixel_url ) {
+			return new WP_Error(


How will know about these errors? We may want to use log2logstash to track them.

These errors would eventually be returned to the callbacks that are hooked into the filters we're using to track events. The idea is to let the plugins implementing this handle the errors however they want to.

I think we should handle this in the library rather than asking each plugin to handle the error.

We may want to use log2logstash to track them.
I second that, we can set feature telemetry and then pass the some of the properties from $event as $extra.
Here's an example:

vip-go-mu-plugins/config/class-sync.php

Lines 222 to 228 in 4d236a4

\Automattic\VIP\Logstash\log2logstash( array(

'severity' => $severity,

'feature' => self::LOG_FEATURE_NAME,

'message' => $message,

'extra' => $extra,

) );

}

Agreed with @rinatkhaziev and @mjangda, we should handle this in the library so errors are raised uniformly across all implementations if they originate from within the library

Added log2logstash in 1b308c7

mjangda · 2024-08-07T06:15:34Z

telemetry/Tracks/class-tracks-client.php

+		foreach ( $this->events as $event ) {
+			static::instance()->record_event_synchronously( $event );
+		}


The REST API supports bulk events which would be better since we could just make one remote request instead of multiples: https://github.com/Automattic/vip-cli/blob/2a8c90afa9ed2e5d784e820c5da20183094b6711/src/lib/analytics/clients/tracks.ts#L84-L105

Oh good idea! I'll look into that 👍

Changed in 7ae681a

mjangda · 2024-08-07T06:21:23Z

telemetry/Tracks/class-tracks-event.php

+	 * @param array<string, mixed>|array<empty> $event_properties Any event properties to be processed.
+	 * @return stdClass The resulting event object with processed properties.
+	 */
+	protected static function process_properties(


Minor: the use of static methods for this is a bit odd.

~~This was copied from the Parsely plugin so I'm not entirely sure the reason for it.~~

Okay I figured it out. This is because the method is called in the constructor when the object has not been instantiated yet.

mjangda · 2024-08-07T06:38:20Z

telemetry/Tracks/class-tracks-event.php

+		// Set event name.
+		$event->_en = preg_replace(
+			'/^(?:' . static::EVENT_NAME_PREFIX . ')?(.*)/',
+			static::EVENT_NAME_PREFIX . '\1',
+			$event_name
+		) ?? '';


It's not totally clear what the preg_replace is doing here.

We should also set a different prefix for local environments (e.g. vip_dev_.

mjangda · 2024-08-07T06:42:15Z

telemetry/Tracks/class-tracks-event.php

+			$app_id = constant( 'VIP_GO_APP_ID' );
+			if ( is_integer( $app_id ) && 0 < $app_id ) {
+				$event->_ui = $app_id . '_' . $wp_user_id;
+				$event->_ut = 'vip_go_app_wp_user';
+
+				return $event;
+			}


We need to use an identifier that is pseudo-anonymized but still allows tracking the user across different environments (e.g. if I'm working on wpvip.com and docs.wpvip.com, I should not be counted as two different users).

Maybe a hash of the email?

$salt = constant( 'VIP_TELEMETRY_SALT' ); // this would be private but globally shared across the platform $tracks_user_id = hash_hmac( 'sha256', $user->user_email, $salt );

Added this in 89b0248

mjangda · 2024-08-07T06:47:21Z

telemetry/Tracks/class-tracks-event.php

+		// Don't record events during unit tests and CI runs.
+		if ( 'wptests_capabilities' === wp_get_current_user()->cap_key ) {
+			return false;
+		}


This feels odd...

This is to stop events from being recorded during tests, is this not the correct way to do it? Or is it just unnecessary?

More just that it's happening in an unusual spot in the code. Feels like we should do this as late as possible (i.e. before the request is sent).

The is_recordable method is being called by methods like record_event_synchronously which are the methods that makes the request. This seems pretty late to me though 🤔

This is one way that we limited such tracking from happening (for MC Stats) to only production sites. This would be worth considering so it's done pretty early on.

mjangda · 2024-08-07T06:51:35Z

telemetry/Tracks/class-tracks.php

+		string $event_name,
+		array $event_properties = array()
+	) {
+		$event = new Tracks_Event( $event_name, $event_properties );


If the Tracks Event had some errors, we should log those somewhere so we have visibility into failures.

I think my changes in 1b308c7 should cover this

telemetry/class-telemetry-system.php

mjangda · 2024-08-07T06:55:43Z

telemetry/Tracks/class-tracks.php

+	 * Registers the events into their respective WordPress hooks, so they
+	 * can be recorded when the hook fires.
+	 */
+	protected function activate_tracking(): void {


I think we should get rid of this abstraction and make all our our tracking explicit via record_event() calls.

We would still need a way register all the callbacks to the appropriate filters and record_event() is specifically used by the callbacks to send the events to the Tracks system so I'm not sure to achieve what you're asking 🤔

What I mean is that rather than abstracting like so:

vip-go-mu-plugins/vip-parsely/vip-parsely.php

Lines 37 to 55 in 44e6293

$tracks = new Tracks();

$telemetry = new Telemetry( $tracks );

require_once __DIR__ . '/Telemetry/Events/track-settings-page-loaded.php';

$telemetry->register_event(

array(

'action_hook' => 'load-settings_page_parsely',

'callable' => 'Automattic\VIP\Parsely\Telemetry\track_settings_page_loaded',

)

);

require_once __DIR__ . '/Telemetry/Events/track-option-updated.php';

$telemetry->register_event(

array(

'action_hook' => 'update_option_parsely',

'callable' => 'Automattic\VIP\Parsely\Telemetry\track_option_updated',

'accepted_args' => 2,

)

);

With this approach the registration of the hook is separated too far away the actual tracking code which makes it harder to debug.

We should let plugins do the extra work needed to record what they need:

add_action( 'transition_post_status', function( $new, $old, $post ) { Automattic\VIP\Telemetry::get_instance()->record_event( 'post_status_changed', [ ... ] ); }, 10, 3 );

This requires more code but keeps the code more explicit and easier to read / follow / debug.

@Automattic/vip-platform-cantina what do y'all think?

I agree with @mjangda , and I don't even think this will be more code?

And if a particular plugin wants to enrich the data somehow it can do something like where it would make sure that the data has the necessary event properies

Automattic\VIP\Telemetry::get_instance()->record_event( 'post_status_changed', enrich_track_event( [ ... ] ) );

Agreed on the above.

This also leads to a good point - we should add a README for this library with an example of how to use it. That'll help to simplify it down once you see how a plugin would implement it. IMO, it should be as easy to follow as the send_pixel method.

Removed in 6378910

telemetry/Tracks/class-tracks-event.php

Tracking in the events would be more explicit this way

mjangda · 2024-08-15T16:41:22Z

Thanks for the fixes, @hanifn. Based on the latest changes, is this how a typical implementation would work?

$tracker = new Tracks( 'vip_bakery_' );
$tracker->record_event( 'bakery_opened' );

If we want to abstract a bit for easier re-usability, that could look something like this:

namespace Automattic\VIP\Bakery\Analytics;

use Automattic\VIP\Telemetry\Tracks;

function record_event( $event_name, $event_props = [] ) {
    static $tracker = new Tracks( 'vip_bakery_' ); // Our prefix for the events will be 'vip_bakery_'

    static $global_props = [
        'delicious' => 'always',
    ];


    $merged_props = array_merge( $global_props, event_props ); // We should probably allow passing global props to the `Tracks` constructor and handle this there.

    $tracker->record_event( $event_name, $merged_props );
}

Then, elsewhere in our code:

use function Automattic\VIP\Bakery\Analytics\record_event;

record_event( 'bakery_open' );

function bake_cookies( $number ) {
    // ...

    record_event( 'cookies_baked', [ 'number' => $number ] );
}

add_action( 'vip_bakery_eat_cookie', function( $cookie_type ) {
    record_event( 'cookies_eaten', [ 'cookie_type' => $cookie_type ] );
}, 10 );

Every plugin will need the same boilerplate but it's not so bad.

This should work better for cross-app tracking

hanifn · 2024-08-19T13:17:21Z

Based on the latest changes, is this how a typical implementation would work?

@mjangda Yes that looks about right. The client plugin would need to have appropriate functions or methods to hook into the events they want to track and those functions or methods has to call the Tracks->record_event() method. How they achieve that is entirely up to them since this new approach is pretty flexible

ingeniumed

Got a minor comment about the errors, but a bigger one about the dual pixel and shutdown approach that I think we should tackle.

ingeniumed · 2024-08-28T00:52:47Z

telemetry/tracks/class-tracks-client.php

+
+		if ( null === $pixel_url ) {
+			log2logstash( [
+				'severity' => 'error',


Should this be an error, or warning? Considering the status code is 400 wondering if that's better? Just don't want the customer's logs filled up with errors because the plugin didn't implement an event correctly.

If the library can't generate the proper pixel URL then the site won't be able to send events for AJAX requests. I think that's a pretty serious problem that should be logged IMHO

ingeniumed · 2024-08-28T00:55:03Z

telemetry/tracks/class-tracks-client.php

+				continue;
+			}
+
+			echo '<img style="position: fixed;" src="', esc_url( $pixel_url ), '" />';


I'm leaning towards starting with just doing this on shutdown, rather than the pixel approach and shutdown approach. I think we should try to minimize our impact on the page as much as possible. Considering this is telemetry, having it go on shutdown at the very should be fine imo.

Redundant since we already have batch recording on shutdown

sanmai · 2024-09-16T03:19:02Z

telemetry/README.md

+With the class above, you can then initiate event tracking in the main plugin file with these lines:
+
+```php
+$tracker = new Tracker();


Suggested change

$tracker = new Tracker();

$tracker = new MyPluginTracker();

sonarcloud · 2024-09-18T07:22:51Z

Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Add base telemetry and tracks classes

cb1d33b

hanifn added the [Status] Needs Review label Aug 5, 2024

hanifn requested a review from alecgeatches August 5, 2024 10:03

hanifn self-assigned this Aug 5, 2024

hanifn requested a review from a team as a code owner August 5, 2024 10:03

Replace the use of self with static

7128c67

rinatkhaziev reviewed Aug 5, 2024

View reviewed changes

telemetry/Tracks/class-tracks-client.php Outdated Show resolved Hide resolved

rinatkhaziev added [Status] Has Feedback [Status] Needs Tests and removed [Status] Needs Review labels Aug 5, 2024

hanifn and others added 2 commits August 6, 2024 09:35

Merge branch 'develop' into add/telemetry

7ceaf36

Change tracks client constructor to private

c745554

alecgeatches reviewed Aug 6, 2024

View reviewed changes

alecgeatches approved these changes Aug 6, 2024

View reviewed changes

mjangda reviewed Aug 6, 2024

View reviewed changes

telemetry/Tracks/class-tracks-client.php Outdated Show resolved Hide resolved

Merge branch 'develop' into add/telemetry

4f55033

mjangda reviewed Aug 7, 2024

View reviewed changes

hanifn added 10 commits August 7, 2024 17:31

Add tests for main Tracks class

ab17330

Change directory name to lower case

f0a77ff

Attempt to resync directory names step 1

0c573e0

Step 2 of resyncng the directories

3b0b29a

Fix previous resync commit

d5d2b58

Remove parsely filter

bdaaf5a

Remove reference to parsely

8b8b2bf

Add more event details

2ebe3ad

Allow plugins to set event name prefix

d9697a6

Update test

5fc1e69

hanifn and others added 5 commits August 11, 2024 22:12

Merge branch 'develop' into add/telemetry

a5e0ad9

Remove activate tracking method

6378910

Tracking in the events would be more explicit this way

Log errors to logstash

1b308c7

Use tracks batch record API for async recording

7ae681a

Use support user class to check for VIP users

e6a6436

Use hashed email as event user ID

89b0248

This should work better for cross-app tracking

Require the telemetry files

eba3100

hanifn mentioned this pull request Aug 21, 2024

Add Tracks integration Automattic/vip-workflow-plugin#21

Open

hanifn added 4 commits August 22, 2024 20:05

Remove debug code

6dd4bfc

Fix failing tests

3bbc8b8

Add readme

825f403

Fix incorrect path to tracks files

867d28b

hanifn requested review from mjangda, ingeniumed and rinatkhaziev August 26, 2024 01:35

hanifn added 3 commits August 27, 2024 15:17

Fix batch record request body format

e5befe3

User whitelisted user ID types

8125c68

Update tests

566baba

ingeniumed requested a review from alecgeatches August 28, 2024 00:44

ingeniumed reviewed Aug 28, 2024

View reviewed changes

hanifn added 2 commits August 28, 2024 22:01

Remove pixel rendering for events

86780e3

Redundant since we already have batch recording on shutdown

Change user ID type for VIP events

62e7560

hanifn requested a review from ingeniumed September 9, 2024 23:30

sanmai reviewed Sep 16, 2024

View reviewed changes

Merge branch 'develop' into add/telemetry

51802ea

sanmai mentioned this pull request Sep 18, 2024

Add base telemetry library (take 2) #5869

Open

7 tasks

These tests are broken

ea465be

	\Automattic\VIP\Logstash\log2logstash( array(
	'severity' => $severity,
	'feature' => self::LOG_FEATURE_NAME,
	'message' => $message,
	'extra' => $extra,
	) );
	}

	$tracks = new Tracks();
	$telemetry = new Telemetry( $tracks );

	require_once __DIR__ . '/Telemetry/Events/track-settings-page-loaded.php';
	$telemetry->register_event(
	array(
	'action_hook' => 'load-settings_page_parsely',
	'callable' => 'Automattic\VIP\Parsely\Telemetry\track_settings_page_loaded',
	)
	);

	require_once __DIR__ . '/Telemetry/Events/track-option-updated.php';
	$telemetry->register_event(
	array(
	'action_hook' => 'update_option_parsely',
	'callable' => 'Automattic\VIP\Parsely\Telemetry\track_option_updated',
	'accepted_args' => 2,
	)
	);

Add base telemetry library #5770

Are you sure you want to change the base?

Add base telemetry library #5770

Conversation

hanifn commented Aug 5, 2024

Description

Changelog Description

Added

Pre-review checklist

Pre-deploy checklist

Steps to Test

codecov bot commented Aug 5, 2024 • edited Loading

Codecov Report

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

rinatkhaziev commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecgeatches left a comment

Choose a reason for hiding this comment

mjangda 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

hanifn Aug 7, 2024 • edited Loading

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

mjangda Aug 7, 2024 • edited Loading

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

mjangda commented Aug 15, 2024 • edited Loading

hanifn commented Aug 19, 2024 • edited Loading

ingeniumed 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

sonarcloud bot commented Sep 18, 2024

Quality Gate passed

codecov bot commented Aug 5, 2024 •

edited

Loading

hanifn Aug 7, 2024 •

edited

Loading

mjangda Aug 7, 2024 •

edited

Loading

mjangda commented Aug 15, 2024 •

edited

Loading

hanifn commented Aug 19, 2024 •

edited

Loading