From 591693b0d3a4d2cc0394b36e2902b3dd200dfc99 Mon Sep 17 00:00:00 2001 From: Jacob Lucky Date: Sun, 14 Dec 2025 14:06:59 -0500 Subject: [PATCH] OCPBUGS-67319: Fix duplicate telemetry events on page load Stabilize the useTelemetry hook callback using the "latest ref" pattern to prevent re-renders from causing duplicate identify and page events. Previously, the callback had four dependencies that changed frequently, triggering multiple effect re-runs. Key changes: - Use refs to store current values, allowing a stable callback - Fix CaptureTelemetry to track fired events and prevent duplicates - Remove duplicate analytics.page() call from Segment initialization - Add test isolation for module-level telemetry event queue Fixes https://issues.redhat.com/browse/OCPBUGS-67319 --- .../src/api/segment-analytics.ts | 1 - .../src/hooks/__tests__/useTelemetry.spec.ts | 2 + .../console-shared/src/hooks/useTelemetry.ts | 48 ++++++++++++++----- frontend/public/components/app.tsx | 31 ++++++++---- 4 files changed, 62 insertions(+), 20 deletions(-) diff --git a/frontend/packages/console-dynamic-plugin-sdk/src/api/segment-analytics.ts b/frontend/packages/console-dynamic-plugin-sdk/src/api/segment-analytics.ts index 49298112c2d..50759a30b60 100644 --- a/frontend/packages/console-dynamic-plugin-sdk/src/api/segment-analytics.ts +++ b/frontend/packages/console-dynamic-plugin-sdk/src/api/segment-analytics.ts @@ -106,7 +106,6 @@ const initSegmentAnalytics = () => { options.integrations = { 'Segment.io': { apiHost: TELEMETRY_API_HOST } }; } analytics.load(TELEMETRY_API_KEY, options); - analytics.page(); // Make the first page call to load the integrations }; if (!SAMPLE_SESSION) { diff --git a/frontend/packages/console-shared/src/hooks/__tests__/useTelemetry.spec.ts b/frontend/packages/console-shared/src/hooks/__tests__/useTelemetry.spec.ts index 646f87e9ca9..d4c2deb743c 100644 --- a/frontend/packages/console-shared/src/hooks/__tests__/useTelemetry.spec.ts +++ b/frontend/packages/console-shared/src/hooks/__tests__/useTelemetry.spec.ts @@ -12,6 +12,7 @@ import { testHook } from '@console/shared/src/test-utils/hooks-utils'; import { getClusterProperties, updateClusterPropertiesFromTests, + clearTelemetryEventsForTests, useTelemetry, } from '../useTelemetry'; @@ -135,6 +136,7 @@ describe('useTelemetry', () => { beforeEach(() => { listener.mockReset(); + clearTelemetryEventsForTests(); const extensions: ResolvedExtension[] = [ { type: 'console.telemetry/listener', diff --git a/frontend/packages/console-shared/src/hooks/useTelemetry.ts b/frontend/packages/console-shared/src/hooks/useTelemetry.ts index dcd70902066..bb6f815858f 100644 --- a/frontend/packages/console-shared/src/hooks/useTelemetry.ts +++ b/frontend/packages/console-shared/src/hooks/useTelemetry.ts @@ -1,4 +1,4 @@ -import { useEffect, useCallback } from 'react'; +import { useEffect, useCallback, useRef } from 'react'; import { useResolvedExtensions, isTelemetryListener, @@ -70,6 +70,10 @@ let clusterProperties = getClusterProperties(); export const updateClusterPropertiesFromTests = () => (clusterProperties = getClusterProperties()); +export const clearTelemetryEventsForTests = () => { + telemetryEvents = []; +}; + export const useTelemetry = () => { // TODO use usePluginInfo() hook to tell whether all dynamic plugins have been processed // to avoid firing telemetry events multiple times whenever a dynamic plugin loads asynchronously @@ -85,24 +89,44 @@ export const useTelemetry = () => { const [extensions] = useResolvedExtensions(isTelemetryListener); + // Store current values in refs so the callback can access them without being recreated + const extensionsRef = useRef(extensions); + extensionsRef.current = extensions; + const userPreferenceRef = useRef(currentUserPreferenceTelemetryValue); + userPreferenceRef.current = currentUserPreferenceTelemetryValue; + const userResourceRef = useRef(userResource); + userResourceRef.current = userResource; + const userResourceIsLoadedRef = useRef(userResourceIsLoaded); + userResourceIsLoadedRef.current = userResourceIsLoaded; + + // Replay queued events when user explicitly opts in (OPT-IN cluster mode only) useEffect(() => { if ( userIsOptedInToTelemetry(currentUserPreferenceTelemetryValue) && clusterIsOptedInToTelemetry() && - telemetryEvents.length > 0 && - userResourceIsLoaded + extensions.length > 0 && + userResourceIsLoaded && + telemetryEvents.length > 0 ) { telemetryEvents.forEach(({ eventType, event }) => { extensions.forEach((e) => e.properties.listener(eventType, { ...event, userResource })); }); telemetryEvents = []; } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [currentUserPreferenceTelemetryValue, userResourceIsLoaded]); + }, [currentUserPreferenceTelemetryValue, userResourceIsLoaded, extensions, userResource]); + // Return a stable callback that reads current values from refs + // This prevents consumers from re-running effects when telemetry state changes return useCallback( (eventType, properties: Record) => { - if (isOptedOutFromTelemetry(currentUserPreferenceTelemetryValue)) return; + const currentUserPreference = userPreferenceRef.current; + const currentExtensions = extensionsRef.current; + const currentUserResource = userResourceRef.current; + const currentUserResourceIsLoaded = userResourceIsLoadedRef.current; + + if (isOptedOutFromTelemetry(currentUserPreference)) { + return; + } const event = { ...clusterProperties, @@ -112,20 +136,22 @@ export const useTelemetry = () => { }; if ( - (clusterIsOptedInToTelemetry() && !currentUserPreferenceTelemetryValue) || - !userResourceIsLoaded + (clusterIsOptedInToTelemetry() && !currentUserPreference) || + !currentUserResourceIsLoaded ) { telemetryEvents.push({ eventType, event }); if (telemetryEvents.length > 10) { - telemetryEvents.shift(); // Remove the first element + telemetryEvents.shift(); } return; } - extensions.forEach((e) => e.properties.listener(eventType, { ...event, userResource })); + currentExtensions.forEach((e) => + e.properties.listener(eventType, { ...event, userResource: currentUserResource }), + ); }, - [extensions, currentUserPreferenceTelemetryValue, userResource, userResourceIsLoaded], + [], // Empty deps - callback is stable, reads current values from refs ); }; diff --git a/frontend/public/components/app.tsx b/frontend/public/components/app.tsx index 86a826d10dd..3de22bddd34 100644 --- a/frontend/public/components/app.tsx +++ b/frontend/public/components/app.tsx @@ -360,16 +360,18 @@ const AppRouter: React.FC = () => { const CaptureTelemetry = memo(function CaptureTelemetry() { const [perspective] = useActivePerspective(); const fireTelemetryEvent = useTelemetry(); - const [debounceTime, setDebounceTime] = useState(5000); const [titleOnLoad, setTitleOnLoad] = useState(''); // notify of identity change const user = useSelector(getUser); const telemetryTitle = getTelemetryTitle(); + // Track if we've already fired the initial page event + const initialPageFiredRef = useRef(false); + + // Wait 5 seconds before firing the first page event to allow initial redirects to settle useEffect(() => { setTimeout(() => { setTitleOnLoad(telemetryTitle); - setDebounceTime(500); }, 5000); }, [telemetryTitle]); @@ -377,9 +379,11 @@ const CaptureTelemetry = memo(function CaptureTelemetry() { if (user?.uid || user?.username) { fireTelemetryEvent('identify', { perspective, user }); } - // Only trigger identify event when the user identifier changes + // Only trigger identify event when the user identifier changes. + // fireTelemetryEvent is stable (doesn't change identity) so it's safe to omit. + // We intentionally exclude perspective/user object - identify is for user identity, not navigation. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [user?.uid || user?.username, fireTelemetryEvent]); + }, [user?.uid, user?.username]); // notify url change events // Debouncing the url change events so that redirects don't fire multiple events. @@ -390,25 +394,36 @@ const CaptureTelemetry = memo(function CaptureTelemetry() { title: getTelemetryTitle(), ...withoutSensitiveInformations(location), }); - }, debounceTime); + }, 500); + + // Use a ref to always have the latest fireUrlChangeEvent without triggering effect re-runs + const fireUrlChangeEventRef = useRef(fireUrlChangeEvent); + fireUrlChangeEventRef.current = fireUrlChangeEvent; + useEffect(() => { if (!titleOnLoad) { return; } - fireUrlChangeEvent(history.location); + // Only fire initial page event once + if (!initialPageFiredRef.current) { + initialPageFiredRef.current = true; + fireUrlChangeEventRef.current(history.location); + } let { pathname, search } = history.location; const unlisten = history.listen((location) => { const { pathname: nextPathname, search: nextSearch } = history.location; if (pathname !== nextPathname || search !== nextSearch) { pathname = nextPathname; search = nextSearch; - fireUrlChangeEvent(location); + fireUrlChangeEventRef.current(location); } }); return () => { unlisten(); }; - }, [perspective, fireUrlChangeEvent, titleOnLoad]); + // fireUrlChangeEventRef is stable, so we don't need it in deps + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [perspective, titleOnLoad]); return null; });