From b500fac551de070cb215b0366e7a93d7ef862e61 Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Sun, 26 Sep 2021 18:31:22 +0200 Subject: [PATCH] Review feedback. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Camila Belo Co-authored-by: Fredrik Adelöw Signed-off-by: Eric Peterson --- .../core-app-api/src/routing/RouteTracker.tsx | 10 +-- .../src/components/Link/Link.test.tsx | 27 ++++---- .../src/components/Link/Link.tsx | 4 +- packages/core-plugin-api/api-report.md | 15 ++-- .../src/analytics/AnalyticsContext.tsx | 15 ++-- .../core-plugin-api/src/analytics/Tracker.ts | 14 +++- .../core-plugin-api/src/analytics/index.ts | 6 +- .../core-plugin-api/src/analytics/types.ts | 13 ++-- .../src/analytics/useAnalytics.tsx | 16 ++++- .../src/extensions/extensions.test.tsx | 2 +- .../src/extensions/extensions.tsx | 12 ++-- .../AnalyticsApi/MockAnalyticsApi.test.ts | 2 + plugins/analytics-module-ga/README.md | 22 ++++-- plugins/analytics-module-ga/package.json | 1 + .../AnalyticsApi/GoogleAnalytics.test.ts | 1 + .../AnalyticsApi/GoogleAnalytics.ts | 68 +++++++++---------- 16 files changed, 129 insertions(+), 99 deletions(-) diff --git a/packages/core-app-api/src/routing/RouteTracker.tsx b/packages/core-app-api/src/routing/RouteTracker.tsx index f65ef49dec..464f451c9c 100644 --- a/packages/core-app-api/src/routing/RouteTracker.tsx +++ b/packages/core-app-api/src/routing/RouteTracker.tsx @@ -40,15 +40,15 @@ const getExtensionContext = ( ): CommonAnalyticsContext | {} => { try { // Find matching routes for the given path name. - const matches = matchRoutes(routes, { pathname }); + const matches = matchRoutes(routes, { pathname }) as + | { route: BackstageRouteObject }[] + | null; // Of the matching routes, get the last (e.g. most specific) instance of // the BackstageRouteObject. const routeObject = matches - ?.filter( - match => (match?.route as BackstageRouteObject).routeRefs?.size > 0, - ) - .pop()?.route as BackstageRouteObject; + ?.filter(match => match?.route.routeRefs?.size > 0) + .pop()?.route; // If there is no route object, then allow inheritance of default context. if (!routeObject) { diff --git a/packages/core-components/src/components/Link/Link.test.tsx b/packages/core-components/src/components/Link/Link.test.tsx index 9690bdfcba..071b6fcffe 100644 --- a/packages/core-components/src/components/Link/Link.test.tsx +++ b/packages/core-components/src/components/Link/Link.test.tsx @@ -15,13 +15,12 @@ */ import React from 'react'; -import { render, fireEvent } from '@testing-library/react'; +import { render, fireEvent, waitFor } from '@testing-library/react'; import { MockAnalyticsApi, wrapInTestApp } from '@backstage/test-utils'; import { ApiProvider, ApiRegistry } from '@backstage/core-app-api'; import { analyticsApiRef } from '@backstage/core-plugin-api'; import { isExternalUri, Link } from './Link'; import { Route, Routes } from 'react-router'; -import { act } from 'react-dom/test-utils'; describe('', () => { it('navigates using react-router', async () => { @@ -36,10 +35,10 @@ describe('', () => { ), ); expect(() => getByText(testString)).toThrow(); - await act(async () => { - fireEvent.click(getByText(linkText)); + fireEvent.click(getByText(linkText)); + await waitFor(() => { + expect(getByText(testString)).toBeInTheDocument(); }); - expect(getByText(testString)).toBeInTheDocument(); }); it('captures click using analytics api', async () => { @@ -57,18 +56,18 @@ describe('', () => { ), ); - await act(async () => { - fireEvent.click(getByText(linkText)); - }); + fireEvent.click(getByText(linkText)); // Analytics event should have been fired. - expect(analyticsApi.getEvents()[0]).toMatchObject({ - action: 'click', - subject: '/test', - }); + await waitFor(() => { + expect(analyticsApi.getEvents()[0]).toMatchObject({ + action: 'click', + subject: '/test', + }); - // Custom onClick handler should have still been fired too. - expect(customOnClick).toHaveBeenCalled(); + // Custom onClick handler should have still been fired too. + expect(customOnClick).toHaveBeenCalled(); + }); }); describe('isExternalUri', () => { diff --git a/packages/core-components/src/components/Link/Link.tsx b/packages/core-components/src/components/Link/Link.tsx index 69da5cd613..fcfd2483de 100644 --- a/packages/core-components/src/components/Link/Link.tsx +++ b/packages/core-components/src/components/Link/Link.tsx @@ -47,9 +47,7 @@ const ActualLink = React.forwardRef( const newWindow = external && !!/^https?:/.exec(to); const handleClick = (event: React.MouseEvent) => { - if (onClick !== undefined) { - onClick(event); - } + onClick?.(event); analytics.captureEvent('click', to); }; diff --git a/packages/core-plugin-api/api-report.md b/packages/core-plugin-api/api-report.md index 76411f3f02..48ac4f7156 100644 --- a/packages/core-plugin-api/api-report.md +++ b/packages/core-plugin-api/api-report.md @@ -53,17 +53,15 @@ export const AnalyticsContext: ({ attributes, children, }: { - attributes: AnalyticsContextValue; + attributes: Partial; children: ReactNode; }) => JSX.Element; -// Warning: (ae-forgotten-export) The symbol "AnyAnalyticsContext" needs to be exported by the entry point index.d.ts // Warning: (ae-missing-release-tag) "AnalyticsContextValue" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public -export type AnalyticsContextValue = Partial< - CommonAnalyticsContext & AnyAnalyticsContext ->; +export type AnalyticsContextValue = CommonAnalyticsContext & + AnyAnalyticsContext; // Warning: (ae-missing-release-tag) "AnalyticsEvent" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // @@ -97,6 +95,13 @@ export type AnalyticsTracker = { ) => void; }; +// Warning: (ae-missing-release-tag) "AnyAnalyticsContext" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public +export type AnyAnalyticsContext = { + [param in string]: string | boolean | number | undefined; +}; + // Warning: (ae-missing-release-tag) "AnyApiFactory" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) diff --git a/packages/core-plugin-api/src/analytics/AnalyticsContext.tsx b/packages/core-plugin-api/src/analytics/AnalyticsContext.tsx index 5852dab51c..284b4186b2 100644 --- a/packages/core-plugin-api/src/analytics/AnalyticsContext.tsx +++ b/packages/core-plugin-api/src/analytics/AnalyticsContext.tsx @@ -14,7 +14,7 @@ * limitations under the License. */ -import React, { createContext, ReactNode, useContext } from 'react'; +import React, { createContext, ReactNode, useContext, useMemo } from 'react'; import { AnalyticsContextValue } from './types'; const AnalyticsReactContext = createContext({ @@ -43,14 +43,17 @@ export const AnalyticsContext = ({ attributes, children, }: { - attributes: AnalyticsContextValue; + attributes: Partial; children: ReactNode; }) => { const parentValues = useAnalyticsContext(); - const combinedValue = { - ...parentValues, - ...attributes, - }; + const combinedValue = useMemo( + () => ({ + ...parentValues, + ...attributes, + }), + [parentValues, attributes], + ); return ( diff --git a/packages/core-plugin-api/src/analytics/Tracker.ts b/packages/core-plugin-api/src/analytics/Tracker.ts index d1ddf5d342..51e99bff14 100644 --- a/packages/core-plugin-api/src/analytics/Tracker.ts +++ b/packages/core-plugin-api/src/analytics/Tracker.ts @@ -14,13 +14,21 @@ * limitations under the License. */ -import { AnalyticsApi, AnalyticsEventAttributes } from '../apis'; +import { + AnalyticsApi, + AnalyticsEventAttributes, + AnalyticsTracker, +} from '../apis'; import { AnalyticsContextValue } from './'; -export class Tracker { +export class Tracker implements AnalyticsTracker { constructor( private readonly analyticsApi: AnalyticsApi, - private context: AnalyticsContextValue = {}, + private context: AnalyticsContextValue = { + routeRef: 'unknown', + pluginId: 'root', + extension: 'App', + }, ) {} setContext(context: AnalyticsContextValue) { diff --git a/packages/core-plugin-api/src/analytics/index.ts b/packages/core-plugin-api/src/analytics/index.ts index 651e8e105e..942df56ef8 100644 --- a/packages/core-plugin-api/src/analytics/index.ts +++ b/packages/core-plugin-api/src/analytics/index.ts @@ -15,5 +15,9 @@ */ export { AnalyticsContext } from './AnalyticsContext'; -export type { AnalyticsContextValue, CommonAnalyticsContext } from './types'; +export type { + AnalyticsContextValue, + AnyAnalyticsContext, + CommonAnalyticsContext, +} from './types'; export { useAnalytics } from './useAnalytics'; diff --git a/packages/core-plugin-api/src/analytics/types.ts b/packages/core-plugin-api/src/analytics/types.ts index 045328f613..ea6c3b030b 100644 --- a/packages/core-plugin-api/src/analytics/types.ts +++ b/packages/core-plugin-api/src/analytics/types.ts @@ -19,17 +19,17 @@ */ export type CommonAnalyticsContext = { /** - * The associated plugin. + * The nearest known parent plugin where the event was captured. */ pluginId: string; /** - * The ID of the associated route ref. + * The ID of the routeRef that was active when the event was captured. */ routeRef: string; /** - * The name of the associated extension. + * The nearest known parent extension where the event was captured. */ extension: string; }; @@ -37,13 +37,12 @@ export type CommonAnalyticsContext = { /** * Allow arbitrary scalar values as context attributes too. */ -type AnyAnalyticsContext = { +export type AnyAnalyticsContext = { [param in string]: string | boolean | number | undefined; }; /** * Analytics context envelope. */ -export type AnalyticsContextValue = Partial< - CommonAnalyticsContext & AnyAnalyticsContext ->; +export type AnalyticsContextValue = CommonAnalyticsContext & + AnyAnalyticsContext; diff --git a/packages/core-plugin-api/src/analytics/useAnalytics.tsx b/packages/core-plugin-api/src/analytics/useAnalytics.tsx index dd1176fff0..2dcb284879 100644 --- a/packages/core-plugin-api/src/analytics/useAnalytics.tsx +++ b/packages/core-plugin-api/src/analytics/useAnalytics.tsx @@ -20,11 +20,21 @@ import { useRef } from 'react'; import { Tracker } from './Tracker'; function useTracker(): AnalyticsTracker { + const trackerRef = useRef(null); const analyticsApi = useApi(analyticsApiRef); - const tracker = useRef(new Tracker(analyticsApi)); const context = useAnalyticsContext(); - tracker.current.setContext(context); - return tracker.current; + + function getTracker(): Tracker { + if (trackerRef.current === null) { + trackerRef.current = new Tracker(analyticsApi); + } + return trackerRef.current; + } + + const tracker = getTracker(); + tracker.setContext(context); + + return tracker; } /** diff --git a/packages/core-plugin-api/src/extensions/extensions.test.tsx b/packages/core-plugin-api/src/extensions/extensions.test.tsx index eff8db5029..06b77c248c 100644 --- a/packages/core-plugin-api/src/extensions/extensions.test.tsx +++ b/packages/core-plugin-api/src/extensions/extensions.test.tsx @@ -126,7 +126,7 @@ describe('extensions', () => { ); }, }, - data: { 'core.mountpoint': { id: 'some-ref' } }, + data: { 'core.mountPoint': { id: 'some-ref' } }, }), ); diff --git a/packages/core-plugin-api/src/extensions/extensions.tsx b/packages/core-plugin-api/src/extensions/extensions.tsx index 31bb08a13f..5427b5f61c 100644 --- a/packages/core-plugin-api/src/extensions/extensions.tsx +++ b/packages/core-plugin-api/src/extensions/extensions.tsx @@ -133,6 +133,9 @@ export function createReactExtension< const Result: any = (props: any) => { const app = useApp(); const { Progress } = app.getComponents(); + const mountPoint = data?.['core.mountPoint'] as + | { id?: string } + | undefined; return ( }> @@ -140,13 +143,8 @@ export function createReactExtension< diff --git a/packages/test-utils/src/testUtils/apis/AnalyticsApi/MockAnalyticsApi.test.ts b/packages/test-utils/src/testUtils/apis/AnalyticsApi/MockAnalyticsApi.test.ts index dad11f985f..3593774a01 100644 --- a/packages/test-utils/src/testUtils/apis/AnalyticsApi/MockAnalyticsApi.test.ts +++ b/packages/test-utils/src/testUtils/apis/AnalyticsApi/MockAnalyticsApi.test.ts @@ -19,6 +19,8 @@ import { MockAnalyticsApi } from './MockAnalyticsApi'; describe('MockAnalyticsApi', () => { const context = { pluginId: 'some-plugin', + routeRef: 'some-route-ref', + extension: 'some-extension', }; it('should collect events', () => { diff --git a/plugins/analytics-module-ga/README.md b/plugins/analytics-module-ga/README.md index a7e681bbe5..15ad4f1948 100644 --- a/plugins/analytics-module-ga/README.md +++ b/plugins/analytics-module-ga/README.md @@ -8,9 +8,9 @@ This plugin contains no other functionality. ## Installation -1. Install this plugin in your Backstage app: +1. Install the plugin package in your Backstage app: `cd packages/app && yarn add @backstage/plugin-analytics-module-ga` -2. Add the API implementation to your App and configure the plugin (see below). +2. Wire up the API implementation to your App: ```tsx // packages/app/src/apis.ts @@ -27,7 +27,7 @@ export const apis: AnyApiFactory[] = [ ]; ``` -## Configuration +3. Configure the plugin in your `app-config.yaml`: The following is the minimum configuration required to start sending analytics events to GA. All that's needed is your Universal Analytics tracking ID: @@ -41,11 +41,11 @@ app: trackingId: UA-0000000-0 ``` -### Plugin Analytics +## Configuration In order to be able to analyze usage of your Backstage instance _by plugin_, we -strongly recommend configuring at least one custom dimension to capture Plugin -IDs associated with events, including page views. +strongly recommend configuring at least one [custom dimension][what-is-a-custom-dimension] +to capture Plugin IDs associated with events, including page views. 1. First, [configure the custom dimension in GA][configure-custom-dimension]. Be sure to set the Scope to `hit`, and name it something like `Plugin`. Note @@ -84,6 +84,10 @@ app: index: 2 source: context key: routeRef + - type: dimension + index: 3 + source: context + key: extension - type: metric index: 1 source: attributes @@ -104,6 +108,9 @@ app: debug: true # Logs analytics event to the web console ``` +You might commonly set the above in an `app-config.local.yaml` file, which is +normally `gitignore`'d but loaded and merged in when Backstage is bootstrapped. + ## Development If you would like to contribute improvements to this plugin, the easiest way to @@ -141,4 +148,5 @@ app: key: pluginId ``` -[configure-custom-dimension]: https://support.google.com/analytics/answer/2709828?hl=en#configuration +[what-is-a-custom-dimension]: https://support.google.com/analytics/answer/2709828 +[configure-custom-dimension]: https://support.google.com/analytics/answer/2709828#configuration diff --git a/plugins/analytics-module-ga/package.json b/plugins/analytics-module-ga/package.json index d50306d0a4..71168f819c 100644 --- a/plugins/analytics-module-ga/package.json +++ b/plugins/analytics-module-ga/package.json @@ -4,6 +4,7 @@ "main": "src/index.ts", "types": "src/index.ts", "license": "Apache-2.0", + "private": false, "publishConfig": { "access": "public", "main": "dist/index.esm.js", diff --git a/plugins/analytics-module-ga/src/apis/implementations/AnalyticsApi/GoogleAnalytics.test.ts b/plugins/analytics-module-ga/src/apis/implementations/AnalyticsApi/GoogleAnalytics.test.ts index c262849695..cbcf95cb55 100644 --- a/plugins/analytics-module-ga/src/apis/implementations/AnalyticsApi/GoogleAnalytics.test.ts +++ b/plugins/analytics-module-ga/src/apis/implementations/AnalyticsApi/GoogleAnalytics.test.ts @@ -53,6 +53,7 @@ describe('GoogleAnalytics', () => { const context = { extension: 'App', pluginId: 'some-plugin', + routeRef: 'unknown', releaseNum: 1337, }; const advancedConfig = new ConfigReader({ diff --git a/plugins/analytics-module-ga/src/apis/implementations/AnalyticsApi/GoogleAnalytics.ts b/plugins/analytics-module-ga/src/apis/implementations/AnalyticsApi/GoogleAnalytics.ts index a05b4fe127..1e6216936c 100644 --- a/plugins/analytics-module-ga/src/apis/implementations/AnalyticsApi/GoogleAnalytics.ts +++ b/plugins/analytics-module-ga/src/apis/implementations/AnalyticsApi/GoogleAnalytics.ts @@ -23,25 +23,18 @@ import { } from '@backstage/core-plugin-api'; import { Config } from '@backstage/config'; -type CDM = { +type CustomDimensionOrMetricConfig = { type: 'dimension' | 'metric'; index: number; source: 'context' | 'attributes'; key: string; }; -/** - * Type guard for emptiness check in array filter. - */ -function notEmpty(value: T | undefined): value is T { - return value !== undefined; -} - /** * Google Analytics API provider for the Backstage Analytics API. */ export class GoogleAnalytics implements AnalyticsApi { - private readonly cdmConfig: CDM[]; + private readonly cdmConfig: CustomDimensionOrMetricConfig[]; /** * Instantiate the implementation and initialize ReactGA. @@ -52,7 +45,7 @@ export class GoogleAnalytics implements AnalyticsApi { testMode, debug, }: { - cdmConfig: CDM[]; + cdmConfig: CustomDimensionOrMetricConfig[]; trackingId: string; testMode: boolean; debug: boolean; @@ -69,19 +62,22 @@ export class GoogleAnalytics implements AnalyticsApi { static fromConfig(config: Config) { // Get all necessary configuration. const trackingId = config.getString('app.analytics.ga.trackingId'); - const debug = !!config.getOptionalBoolean('app.analytics.ga.debug'); - const testMode = !!config.getOptionalBoolean('app.analytics.ga.testMode'); + const debug = config.getOptionalBoolean('app.analytics.ga.debug') ?? false; + const testMode = + config.getOptionalBoolean('app.analytics.ga.testMode') ?? false; const cdmConfig = config .getOptionalConfigArray('app.analytics.ga.customDimensionsMetrics') ?.map(c => { return { - type: c.getString('type') as CDM['type'], + type: c.getString('type') as CustomDimensionOrMetricConfig['type'], index: c.getNumber('index'), - source: c.getString('source') as CDM['source'], + source: c.getString( + 'source', + ) as CustomDimensionOrMetricConfig['source'], key: c.getString('key'), }; - }) || []; + }) ?? []; // Return an implementation instance. return new GoogleAnalytics({ @@ -106,7 +102,7 @@ export class GoogleAnalytics implements AnalyticsApi { }: AnalyticsEvent) { const customMetadata = this.getCustomDimensionMetrics(context, attributes); - if (action === 'navigate' && context?.extension === 'App') { + if (action === 'navigate' && context.extension === 'App') { // Set any/all custom dimensions. if (Object.keys(customMetadata).length) { ReactGA.set(customMetadata); @@ -127,34 +123,32 @@ export class GoogleAnalytics implements AnalyticsApi { /** * Returns an object of dimensions/metrics given an Analytics Context and an - * Event Context, e.g. { dimension1: "some value", metric8: 42 } + * Event Attributes, e.g. { dimension1: "some value", metric8: 42 } */ private getCustomDimensionMetrics( context: AnalyticsContextValue, attributes: AnalyticsEventAttributes = {}, ) { - const dataArray = this.cdmConfig - .map(cdm => { - const value = - cdm.source === 'context' ? context[cdm.key] : attributes[cdm.key]; + const customDimensionsMetrics: { [x: string]: string | number | boolean } = + {}; - // Never pass a non-numeric value on a metric. - if (cdm.type === 'metric' && typeof value !== 'number') { - return undefined; - } + this.cdmConfig.forEach(config => { + const value = + config.source === 'context' + ? context[config.key] + : attributes[config.key]; - return value !== undefined - ? { - [`${cdm.type}${cdm.index}`]: value, - } - : undefined; - }) - .filter(notEmpty); + // Never pass a non-numeric value on a metric. + if (config.type === 'metric' && typeof value !== 'number') { + return; + } - return dataArray.length - ? dataArray.reduce((result, cd) => { - return Object.assign(result, cd); - }) - : {}; + // Only set defined values. + if (value !== undefined) { + customDimensionsMetrics[`${config.type}${config.index}`] = value; + } + }); + + return customDimensionsMetrics; } }