Should `admin_enqueue_scripts()` be opt-in, instead of opt-out?

[lkraav]

We were debugging some admin JS slowness, and I noticed integration loads trackers also in wp-admin.

I think we don't have telemetry so might have to do random guessing, but how many people are using Sentry for wp-admin development, vs frontend?

Feels like defaulting to opt-in could be the right move?

Until then, here's an mu-plugin for those interested in opt-out:

<?php
/**
 * Plugin Name: WP Sentry Integration should avoid wp-admin.
 *
 * @since 2023.05.18
 */

namespace MuPlugins;

use WP_Sentry_Js_Tracker;

/**
 * @see `WP_Sentry_Js_Tracker::__construct()`
 */
const WP_SENTRY_INTEGRATION_ADMIN_ENQUEUE_PRIORITY = 0;

add_action( 'admin_enqueue_scripts', static function(): void {
    remove_action( 'admin_enqueue_scripts', [ WP_Sentry_Js_Tracker::get_instance(), 'on_enqueue_scripts' ], WP_SENTRY_INTEGRATION_ADMIN_ENQUEUE_PRIORITY );
}, WP_SENTRY_INTEGRATION_ADMIN_ENQUEUE_PRIORITY - 1 );

[stayallive]

Tracking JS errors in the front-end is as interesting as in the back-end IMHO 😄

Also at this point opt-in is not an option unless we do a major release, so maybe in the future but at this moment I don't see that that is needed. But happy to add a nicer option than what you have as mu-plugin to disable it in the backend because you also break the tester page with that (although probably not an issue for you).

You mention 6.14.0 specifically in the title, is that because you think it's since that version? Because I think this behaviour has been present for as long as we have the Browser SDK added.

[lkraav]

Tracking JS errors in the front-end is as interesting as in the back-end IMHO 😄

Depends. Could also be considered as pure noise, as there might not be any bandwidth to address anything at wp-admin layer anyway.

Also at this point opt-in is not an option unless we do a major release, so maybe in the future but at this moment I don't see that that is needed. But happy to add a nicer option than what you have as mu-plugin to disable it in the backend because you also break the tester page with that (although probably not an issue for you).

Yeah I didn't QA tester page. Perhaps enqueue method could be better split, because ideally tester page would remain working in all cases, indeed.

You mention 6.14.0 specifically in the title, is that because you think it's since that version? Because I think this behaviour has been present for as long as we have the Browser SDK added.

In this case it's about «latest I tested with».

[stayallive]

So I have been thinking what about:

define( 'WP_SENTRY_BROWSER_DSN', 'JS_DSN' );
define( 'WP_SENTRY_BROWSER_ADMIN_ENABLED', true );    //   is_admin() (default: true)
define( 'WP_SENTRY_BROWSER_FRONTEND_ENABLED', true ); // ! is_admin() (default: true)

Of course we keep the tester page working no matter what you choose here (unless you set no DSN at all).

What do you think?

[lkraav]

Sounds about right.

is_admin() has some worms in the can with it also matching admin-ajax.php which is used across all contexts.

We should also take care to match login screen.

See the fun at WooCommerce::is_request() https://github.com/woocommerce/woocommerce/blob/5d6b3d5ceb6c263bbf29c608a4faacf6ea9a8f7c/plugins/woocommerce/includes/class-woocommerce.php#L383-L400 for reference.

[stayallive]

Well yeah okay poor wording on my part.

I'd actually use admin_enqueue_scripts vs. wp_enqueue_scripts 😄

Should work to allow you to disable it on the admin side by setting WP_SENTRY_BROWSER_ADMIN_ENABLED to false.

If you would be helped by that I will work to implement that!

---

Edit: I will also add an extra define to disable login_enqueue_scripts too.

Full list to add:

define( 'WP_SENTRY_BROWSER_DSN', 'JS_DSN' );          // required for any of the ones below
define( 'WP_SENTRY_BROWSER_ADMIN_ENABLED', true );    // admin_enqueue_scripts (default: true)
define( 'WP_SENTRY_BROWSER_LOGIN_ENABLED', true );    // login_enqueue_scripts (default: true)
define( 'WP_SENTRY_BROWSER_FRONTEND_ENABLED', true ); // wp_enqueue_scripts    (default: true)

Depending on if that works I might allow you to pass a DSN instead of true so you could have different DSNs per admin/login/frontend which might be a cool feature. (I might revisit this in the future if someone needs it)

[stayallive]

v6.15.0 was just released adding these constants, hope that allows you to easily enable the Sentry SDK where you need or want it.