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

chore: improve breadcrumbs in Sentry with providing more info #864

Merged
merged 2 commits into from
Jan 31, 2025

Conversation

nightnei
Copy link
Contributor

Related issues

Proposed Changes

Sometimes in Sentry breadcrumbs it's not possible to understand which exactly element was clicked, either due to no selectors on element or tailwind classes which are not unique so easy to mix up buttons. Example:

Screenshot 2025-01-30 at 17 12 08 Screenshot 2025-01-30 at 17 13 11

Check out what is the result of the proposed changes. I think, as a first option for the nearest release is a good start and maybe in the future we will improve it, in the next releases.

Testing Instructions

  1. Apply the next diff:
diff --git a/src/index.ts b/src/index.ts
index 4545504..9ca2251 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -41,7 +41,7 @@ if ( ! isCLI() && ! process.env.IS_DEV_BUILD ) {
        Sentry.init( {
                dsn: 'https://97693275b2716fb95048c6d12f4318cf@o248881.ingest.sentry.io/4506612776501248',
                debug: true,
-               enabled: process.env.NODE_ENV !== 'development' && process.env.NODE_ENV !== 'test',
+               enabled: process.env.NODE_ENV !== 'test',
                release: `${ app.getVersion() ? app.getVersion() : COMMIT_HASH }-${ getPlatformName() }`,
        } );
 }
diff --git a/src/lib/shell-open-external-wrapper.ts b/src/lib/shell-open-external-wrapper.ts
index 53916ef..f4cd525 100644
--- a/src/lib/shell-open-external-wrapper.ts
+++ b/src/lib/shell-open-external-wrapper.ts
@@ -12,37 +12,5 @@ const shouldMute = ( error: Error ) => {
 // while only reporting specific errors to Sentry. Some common "application not found"
 // errors are muted to avoid unnecessary error reporting.
 export const shellOpenExternalWrapper = async ( url: string ) => {
-       try {
-               await shell.openExternal( url );
-       } catch ( error ) {
-               if ( error instanceof Error && ! shouldMute( error ) ) {
-                       Sentry.captureException( error );
-               }
-
-               let title = '';
-               let message = '';
-               if ( url.startsWith( 'vscode://file/' ) ) {
-                       title = __( 'Failed to open VS Code' );
-                       message = __(
-                               'Studio is unable to open VS Code. Please ensure it is functioning correctly.'
-                       );
-               } else if ( url.startsWith( 'phpstorm://open?file=' ) ) {
-                       title = __( 'Failed to open PHP Storm' );
-                       message = __(
-                               'Studio is unable to open PHPStorm. Please ensure it is functioning correctly.'
-                       );
-               } else {
-                       title = __( 'Failed to open browser' );
-                       message = __(
-                               'Studio is unable to open your default browser. Please ensure it is functioning correctly.'
-                       );
-               }
-
-               dialog.showMessageBox( {
-                       type: 'error',
-                       message: title,
-                       detail: message,
-                       buttons: [ __( 'OK' ) ],
-               } );
-       }
+       throw new Error( 'test' );
 };
diff --git a/src/renderer.ts b/src/renderer.ts
index 896ec80..1324072 100644
--- a/src/renderer.ts
+++ b/src/renderer.ts
@@ -84,8 +84,9 @@ Sentry.init(
 
                                                return '';
                                        };
-
+                                       console.log( 'before:', breadcrumb.message );
                                        breadcrumb.message = ( breadcrumb.message || '' ) + getExtraInformation();
+                                       console.log( 'after:', breadcrumb.message );
                                }
                        }
  1. Run Studio
  2. Open dev-tools -> Console
  3. Test the next buttons, but as many you test as better, since maybe you will find some cases not handled with proposed changes.
    4.1 Screenshot 2025-01-30 at 17 15 23
    Screenshot 2025-01-30 at 17 19 30
    4.2 Screenshot 2025-01-30 at 17 16 49
    Screenshot 2025-01-30 at 17 18 45
    4.3 Not big benefit but easier to understand at a glance:
    Screenshot 2025-01-30 at 17 21 55
    Screenshot 2025-01-30 at 17 21 22
    4.4 Not perfect, but still very helpful:
    Screenshot 2025-01-30 at 17 25 24
    Screenshot 2025-01-30 at 17 26 15
    4.5 Screenshot 2025-01-30 at 17 28 14
    Screenshot 2025-01-30 at 17 28 40
    4.6 Test as much as possible

Copy link
Contributor

@fredrikekelund fredrikekelund left a comment

Choose a reason for hiding this comment

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

The solution itself looks good to me 👍 As you mentioned, we can probably improve it in the future, but this is a good start. A few thoughts:

  • textContent will depend on which translation is being used. Worth keeping in mind.
  • The parent element traversal can go all the way up to <html>. Probably won't be an issue, though, especially since we clearly label it with parent-aria-label etc.

There are two things with the code I think we should improve:

  1. Refine the comments
  2. "Unwrap" the code to reduce indentation

Here's a suggestion for what it could look like

// Enhances Sentry breadcrumb messages by extracting meaningful information from DOM elements.
const getExtraInformation = ( targetElement: HTMLElement ) => {
	// Check for custom data-sentry attribute first, which is used for elements
	// that need explicit tracking identification
	const sentryData = targetElement.getAttribute( 'data-sentry' );
	if ( sentryData ) {
		return `[data-sentry="${ sentryData }"]`;
	}

	// Skip adding extra context if aria-label is present, as it already provides
	// sufficient identification for both tracking and accessibility
	if ( targetElement.getAttribute( 'aria-label' ) ) {
		return '';
	}

	// Fall back to the element's text content if available
	const textContent = targetElement.textContent?.trim() || targetElement.innerText?.trim();
	if ( textContent ) {
		return `[text-content="${ textContent }"]`;
	}

	// If no identifying information is found on the target element,
	// traverse up the DOM tree looking for identifiable parent elements.
	// This helps with SVG icons or other elements wrapped in interactive parents.
	let element = targetElement.parentElement;
	while ( element ) {
		const ariaLabel = element.getAttribute( 'aria-label' );
		const sentryData = element.getAttribute( 'data-sentry' );
		const textContent = element.textContent?.trim() || element.innerText?.trim();
		if ( ariaLabel ) {
			return `[parent-aria-label="${ ariaLabel }"]`;
		}
		if ( sentryData ) {
			return `[parent-data-sentry="${ sentryData }"]`;
		}
		if ( textContent ) {
			return `[parent-text-content="${ textContent }"]`;
		}
		element = element.parentElement;
	}

	return '';
};

Sentry.init(
	{
		debug: true,
		beforeBreadcrumb( breadcrumb, hint ) {
			if ( breadcrumb.category !== 'ui.click' ) {
				return breadcrumb;
			}

			const targetElement = hint?.event?.target;

			if ( targetElement ) {
				breadcrumb.message = ( breadcrumb.message || '' ) + getExtraInformation( targetElement );
			}

			return breadcrumb;
		},
	},
	reactInit
);

@fredrikekelund
Copy link
Contributor

We could also rename getExtraInformation to something more idiomatic, like getExtraSentryBreadcrumbs.

@nightnei nightnei merged commit 1a79410 into trunk Jan 31, 2025
7 checks passed
@nightnei nightnei deleted the update/breadcrumbsInSentryLogs branch January 31, 2025 10:18
Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

Thanks for improving the Sentry logs.

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