Browse Source

feat(analytic): allow custom route analytics name and fix bug when routes change (#40911)

This PR adds a new hook called `useRouteAnalyticsEventNames` which
allows using different event names for route analytics events. The
purpose of this feature is so we can replace our existing analytics
events with route analytics while still maintaining the same name.

This PR also fixes a bug where we didn't handle URL changes correctly.
The state would get reset but the `useEffect` inside of other hooks like
`useRouteAnalyticsParams` wouldn't trigger to update the state
correctly. To fix it, I am passing `previousUrl` which means that
`useEffect` will fire after we reset state from the route changing.
Stephen Cefali 2 years ago
parent
commit
c717b86d3c

+ 11 - 1
static/app/utils/routeAnalytics/useDisableRouteAnalytics.spec.tsx

@@ -12,17 +12,27 @@ function TestComponent() {
 describe('useDisableRouteAnalytics', function () {
   it('disables analytics', function () {
     const setDisableRouteAnalytics = jest.fn();
-    render(
+    const getComponent = (extraContext?: Record<string, any>) => (
       <RouteAnalyticsContext.Provider
         value={{
           setDisableRouteAnalytics,
           setRouteAnalyticsParams: jest.fn(),
           setOrganization: jest.fn(),
+          setEventNames: jest.fn(),
+          previousUrl: '',
+          ...extraContext,
         }}
       >
         <TestComponent />
       </RouteAnalyticsContext.Provider>
     );
+    const {rerender} = render(getComponent());
+    expect(setDisableRouteAnalytics).toHaveBeenCalledWith();
+    setDisableRouteAnalytics.mockClear();
+    rerender(getComponent());
+    // should still be called 0 times because previousURL the same
+    expect(setDisableRouteAnalytics).toHaveBeenCalledTimes(0);
+    rerender(getComponent({previousUrl: 'something-else'}));
     expect(setDisableRouteAnalytics).toHaveBeenCalledWith();
   });
 });

+ 3 - 3
static/app/utils/routeAnalytics/useDisableRouteAnalytics.tsx

@@ -4,11 +4,11 @@ import {RouteAnalyticsContext} from 'sentry/views/routeAnalyticsContextProvider'
 
 /**
  * Disables route analytics when called in a component.
- * Must be called within 2s after the organization context is loaded.
+ * Must be called within 4s after the organization context is loaded.
  */
 export default function useDisableRouteAnalytics() {
-  const {setDisableRouteAnalytics} = useContext(RouteAnalyticsContext);
+  const {setDisableRouteAnalytics, previousUrl} = useContext(RouteAnalyticsContext);
   useEffect(() => {
     setDisableRouteAnalytics();
-  }, [setDisableRouteAnalytics]);
+  }, [setDisableRouteAnalytics, previousUrl]);
 }

+ 42 - 0
static/app/utils/routeAnalytics/useRouteAnalyticsEventNames.spec.tsx

@@ -0,0 +1,42 @@
+import {render} from 'sentry-test/reactTestingLibrary';
+
+import {RouteAnalyticsContext} from 'sentry/views/routeAnalyticsContextProvider';
+
+import useRouteAnalyticsEventNames from './useRouteAnalyticsEventNames';
+
+function TestComponent({eventKey, eventName}: {eventKey: string; eventName: string}) {
+  useRouteAnalyticsEventNames(eventKey, eventName);
+  return <div>hi</div>;
+}
+
+describe('useRouteAnalyticsEventNames', function () {
+  it('disables analytics', function () {
+    const setEventNames = jest.fn();
+    const getComponent = (
+      eventKey: string,
+      eventName: string,
+      extraContext?: Record<string, any>
+    ) => (
+      <RouteAnalyticsContext.Provider
+        value={{
+          setDisableRouteAnalytics: jest.fn(),
+          setRouteAnalyticsParams: jest.fn(),
+          setOrganization: jest.fn(),
+          setEventNames,
+          previousUrl: '',
+          ...extraContext,
+        }}
+      >
+        <TestComponent {...{eventKey, eventName}} />
+      </RouteAnalyticsContext.Provider>
+    );
+    const {rerender} = render(getComponent('a', 'b'));
+    expect(setEventNames).toHaveBeenCalledWith('a', 'b');
+    setEventNames.mockClear();
+    rerender(getComponent('a', 'b'));
+    // should still be called 0 times because previousURL the same
+    expect(setEventNames).toHaveBeenCalledTimes(0);
+    rerender(getComponent('a', 'b', {previousUrl: 'something-else'}));
+    expect(setEventNames).toHaveBeenCalledWith('a', 'b');
+  });
+});

+ 15 - 0
static/app/utils/routeAnalytics/useRouteAnalyticsEventNames.tsx

@@ -0,0 +1,15 @@
+import {useContext, useEffect} from 'react';
+
+import {RouteAnalyticsContext} from 'sentry/views/routeAnalyticsContextProvider';
+
+/**
+ * This hook provides custom analytics event names for route based analytics.
+ * @param eventKey The key used to identify the event
+ * @param eventName The English string used as the event name
+ */
+export default function useRouteAnalyticsEventNames(eventKey: string, eventName: string) {
+  const {setEventNames, previousUrl} = useContext(RouteAnalyticsContext);
+  useEffect(() => {
+    setEventNames(eventKey, eventName);
+  }, [setEventNames, eventKey, eventName, previousUrl]);
+}

+ 2 - 0
static/app/utils/routeAnalytics/useRouteAnalyticsHookSetup.spec.tsx

@@ -22,6 +22,8 @@ describe('useRouteAnalyticsHookSetup', function () {
           setOrganization,
           setDisableRouteAnalytics: jest.fn(),
           setRouteAnalyticsParams: jest.fn(),
+          setEventNames: jest.fn(),
+          previousUrl: '',
         }}
       >
         <OrganizationContext.Provider value={organization}>

+ 12 - 1
static/app/utils/routeAnalytics/useRouteAnalyticsParams.spec.tsx

@@ -12,17 +12,28 @@ function TestComponent() {
 describe('useRouteAnalyticsParams', function () {
   it('calls setRouteAnalyticsParams', function () {
     const setRouteAnalyticsParams = jest.fn();
-    render(
+    const getComponent = (extraContext?: Record<string, any>) => (
       <RouteAnalyticsContext.Provider
         value={{
           setRouteAnalyticsParams,
           setOrganization: jest.fn(),
           setDisableRouteAnalytics: jest.fn(),
+          setEventNames: jest.fn(),
+          previousUrl: '',
+          ...extraContext,
         }}
       >
         <TestComponent />
       </RouteAnalyticsContext.Provider>
     );
+
+    const {rerender} = render(getComponent());
+    expect(setRouteAnalyticsParams).toHaveBeenCalledWith({foo: 'bar'});
+    setRouteAnalyticsParams.mockClear();
+    rerender(getComponent());
+    // should still be called 0 times because previousURL the same
+    expect(setRouteAnalyticsParams).toHaveBeenCalledTimes(0);
+    rerender(getComponent({previousUrl: 'something-else'}));
     expect(setRouteAnalyticsParams).toHaveBeenCalledWith({foo: 'bar'});
   });
 });

+ 3 - 4
static/app/utils/routeAnalytics/useRouteAnalyticsParams.tsx

@@ -7,13 +7,12 @@ import {RouteAnalyticsContext} from 'sentry/views/routeAnalyticsContextProvider'
  * Must be called within 2s after the organization context is loaded.
  */
 export default function useRouteAnalyticsParams(params: Record<string, any>) {
-  const {setRouteAnalyticsParams} = useContext(RouteAnalyticsContext);
-  const keys = Object.keys(params);
-  const values = Object.values(params);
+  const {setRouteAnalyticsParams, previousUrl} = useContext(RouteAnalyticsContext);
+  const jsonParams = JSON.stringify(params);
   useEffect(() => {
     setRouteAnalyticsParams(params);
     // use the object values and keys as dependencies to re-trigger rendering
     // if the underlying parameters change
     // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [...values, ...keys]);
+  }, [jsonParams, previousUrl]);
 }

+ 2 - 0
static/app/utils/routeAnalytics/withRouteAnalytics.spec.tsx

@@ -24,6 +24,8 @@ describe('withRouteAnalytics', function () {
           setRouteAnalyticsParams,
           setDisableRouteAnalytics: jest.fn(),
           setOrganization: jest.fn(),
+          setEventNames: jest.fn(),
+          previousUrl: '',
         }}
       >
         <WrappedComponent />

+ 13 - 1
static/app/views/performance/transactionSummary/transactionSpans/spanDetails/content.tsx

@@ -19,6 +19,8 @@ import SuspectSpansQuery, {
 } from 'sentry/utils/performance/suspectSpans/suspectSpansQuery';
 import {SpanSlug} from 'sentry/utils/performance/suspectSpans/types';
 import {decodeScalar} from 'sentry/utils/queryString';
+import useRouteAnalyticsEventNames from 'sentry/utils/routeAnalytics/useRouteAnalyticsEventNames';
+import useRouteAnalyticsParams from 'sentry/utils/routeAnalytics/useRouteAnalyticsParams';
 import Breadcrumb from 'sentry/views/performance/breadcrumb';
 import {getSelectedProjectPlatforms} from 'sentry/views/performance/utils';
 
@@ -46,8 +48,18 @@ export default function SpanDetailsContentWrapper(props: Props) {
   const minExclusiveTime = decodeScalar(location.query[ZoomKeys.MIN]);
   const maxExclusiveTime = decodeScalar(location.query[ZoomKeys.MAX]);
 
+  // customize the route analytics event we send
+  useRouteAnalyticsEventNames(
+    'performance_views.span_summary.view',
+    'Performance Views: Span Summary page viewed'
+  );
+  useRouteAnalyticsParams({
+    project_platforms: project ? getSelectedProjectPlatforms(location, [project]) : '',
+  });
+
   useEffect(() => {
-    if (project) {
+    // TODO: remove once we set this flag to true for everyone
+    if (project && !organization.features.includes('auto-capture-page-load-analytics')) {
       trackAdvancedAnalyticsEvent('performance_views.span_summary.view', {
         organization,
         project_platforms: getSelectedProjectPlatforms(location, [project]),

+ 1 - 0
static/app/views/releases/detail/index.tsx

@@ -274,6 +274,7 @@ class ReleasesDetailContainer extends AsyncComponent<
       prevProps.params.release !== params.release ||
       prevProps.organization.slug !== organization.slug
     ) {
+      this.props.setRouteAnalyticsParams({release: this.props.params.release});
       super.componentDidUpdate(prevProps, prevState);
     }
   }

Some files were not shown because too many files changed in this diff