wp-media / wp-rocket

Performance optimization plugin for WordPress
https://wp-rocket.me
GNU General Public License v2.0
700 stars 219 forks source link

Implement Data Collection for DJE Safe Mode Usage #7057

Open Miraeld opened 4 weeks ago

Miraeld commented 4 weeks ago

Metrics Collection: Implement Mixpanel tracking to collect data on the usage of the DJE safe mode checkbox.

Miraeld commented 3 weeks ago

Scope a Solution

Outdated 2 approaches.

If https://github.com/wp-media/wp-rocket/issues/7058 hasn't been merged yet, we can add a checkbox to the form file_optimization with the following code:

~~Click to expand the code~~ ``` php 'delay_js_execution_safe_mode' => [ 'type' => 'checkbox', 'label' => __( 'Delay JavaScript Execution safe mode', 'rocket' ), 'container_class' => [ 'wpr-field--children', 'wpr-isParent', ], 'description' => $invalid_license ? __( 'Delay JavaScript Execution safe mode temporarily resolves issues with Delay JavaScript execution but may reduce your PageSpeed Scores and performance. Contact support for help excluding problematic scripts to use this feature fully.', 'rocket' ) : '', 'section' => 'js', 'parent' => 'delay_js', 'page' => 'file_optimization', 'default' => 0, 'sanitize_callback' => 'sanitize_checkbox', 'input_attr' => [ 'disabled' => get_rocket_option( 'delay_js_execution_safe_mode' ) ? 1 : 0, ], 'helper' => '', ], 'delay_js_execution_safe_mode_method' => [ 'type' => 'radio_buttons', 'label' => __( 'Delay JavaScript Execution safe mode', 'rocket' ), 'container_class' => [ 'wpr-field--children', 'wpr-field--js-execution-safe-mode-delivery', 'wpr-field--optimize-css-delivery', ], 'buttons_container_class' => '', 'parent' => 'delay_js_execution_safe_mode', 'section' => 'js', 'page' => 'file_optimization', 'default' => 'dje_safe_mode', 'sanitize_callback' => 'sanitize_checkbox', 'options' => [ 'dje_safe_mode' => [ 'label' => __( 'Delay JavaScript Execution safe mode', 'rocket' ), 'disabled' => $invalid_license ? 'disabled' : false, 'description' => '', // translators: %1$s = opening tag, %2$s = closing tag. 'warning' => $invalid_license ? [] : [ 'title' => __( 'This will decrease the effect of Delay JavaScript Execution', 'rocket' ), 'description' => __( 'This mode temporarily resolves issues with Delay JavaScript execution but may reduce your PageSpeed Scores and performance. Contact support for help excluding problematic scripts to use this feature fully.', 'rocket' ), 'button_label' => __( 'Activate Safe Mode', 'rocket' ), ], 'sub_fields' => $invalid_license ? [] : [ 'delay_js_exclusions' => [ 'type' => 'textarea', 'label' => __( 'Excluded JavaScript Files', 'rocket' ), 'description' => __( 'Specify URLs or keywords that can identify inline or JavaScript files to be excluded from delaying execution (one per line).', 'rocket' ), 'parent' => '', 'section' => 'js', 'page' => 'file_optimization', 'default' => [], 'sanitize_callback' => 'sanitize_textarea', 'input_attr' => [ 'disabled' => get_rocket_option( 'delay_js_execution_safe_mode' ) ? 0 : 1, ], 'helper' => DelayJSSettings::exclusion_list_has_default() ? $delay_js_found_list_helper : $delay_js_list_helper, 'placeholder' => '', ], ], ```

Otherwise, reuse the code from the mentioned PR.

About tracking in mixpanel.

in src/js/global/main.js create a function trackDJESafeMode that will be used to detect if the checkbox enabling safe mode DJE is checked.

/***
* Track DJE Safe Mode Checkbox
***/
function trackDJESafeMode()  {
    var isChecked = $('#delay_js_execution_safe_mode').is(':checked');
    if  (isChecked)  {
        var exclusions = $('#delay_js_exclusions').val();
        if  (typeof mixpanel !==  'undefined')  {
            mixpanel.track('DJE Safe Mode Clicked',  {
                'exclusions': exclusions
            });
        }
    }
}

and add a callback on the button used to save settings OR the form submit. Example of callback on the button click

$('#wpr-options-submit').on('click',  function()  {
    trackDJESafeMode();
});

Finally add the following after this https://github.com/wp-media/wp-rocket/blob/612980f97db6ea6bdadd2c6793892fa0cd59635c/inc/admin/ui/enqueue.php#L70

mixpanel.track(  'DJE Safe Mode Clicked', localStorage.getItem('dje-safe-mode-clicked')  );
remyperona commented 3 weeks ago

Do you need additional code for mixpanel? All the options using checkboxes are tracked automatically already I believe (it's been a long time I checked that)

Miraeld commented 2 weeks ago

We may be tracking all checkboxes already, @wp-media/product could you confirm ?, I've to admit when I was looking at it, I wasn't sure to understand at 100% the mixpanel code and what we are tracking already.

However, from the notion here we need to track the content of the textarea, and for this I guess we'll need to set it up.

DahmaniAdame commented 2 weeks ago

Yes. The feature state and its exclusions entries are sent to mixpanel. But each as a single entry which makes it hard to filter and analyze:

[delay_js_exclusions] => Array
        (
            [0] => \/jquery(-migrate)?-?([0-9.]+)?(.min|.slim|.slim.min)?.js(\?(.*))?( |'|"|>)
            [1] => js-(before|after)
            [2] => (?:/wp-content/|/wp-includes/)(.*)
        )

What is needed is tracking when the above exact consecutive lines are present to know the extent of usage and help decide the next step.

The same should be done for previous iterations of the jquery exclusion pattern (first line). I believe it was changed 2 times or maybe more.

Miraeld commented 2 weeks ago

Ummm, first of all, this grooming is to forget then, as I thought we didn't track everything already.

Then, If I understand well @DahmaniAdame , You want to send something to mixpanel each time someone has

Array
        (
            [0] => \/jquery(-migrate)?-?([0-9.]+)?(.min|.slim|.slim.min)?.js(\?(.*))?( |'|"|>)
            [1] => js-(before|after)
            [2] => (?:/wp-content/|/wp-includes/)(.*)
        )

In their exclusions.

If this is correct, then I don't really understand why would we track this as it won't stay in the textarea as we are sanitizing the content of the textarea and deleting the default exclusion that could be added by the safemode.

As in the future, if these three lines are within the textarea while saving, it should turn on automatically the safe mode (that's something we discussed during the 3.18 presentation, not sure if that's still a plan).

piotrbak commented 2 weeks ago

@DahmaniAdame Wouldn't it make sense to collect everything what's there? We might use this data in the future while looking for the opportunities for enhancements.