From 4f80dda772e95d77167ca952e24d49da783a1a57 Mon Sep 17 00:00:00 2001 From: M-ZubairAhmed Date: Tue, 29 Apr 2025 16:51:43 +0530 Subject: [PATCH] [MM-63794] Remove the Redux Selector telemetry (#30794) --- .../src/actions/telemetry_actions.jsx | 25 - webapp/channels/src/components/root/root.tsx | 3 +- .../src/selectors/create_selector/index.js | 96 +--- .../src/utils/post_list.test.ts | 484 ------------------ 4 files changed, 2 insertions(+), 606 deletions(-) diff --git a/webapp/channels/src/actions/telemetry_actions.jsx b/webapp/channels/src/actions/telemetry_actions.jsx index e710cc26ad8..58bf37ffb35 100644 --- a/webapp/channels/src/actions/telemetry_actions.jsx +++ b/webapp/channels/src/actions/telemetry_actions.jsx @@ -3,7 +3,6 @@ import {Client4} from 'mattermost-redux/client'; import {Preferences} from 'mattermost-redux/constants'; -import {getSortedTrackedSelectors} from 'mattermost-redux/selectors/create_selector'; import {getConfig, isPerformanceDebuggingEnabled} from 'mattermost-redux/selectors/entities/general'; import {getBool} from 'mattermost-redux/selectors/entities/preferences'; @@ -172,30 +171,6 @@ export function trackPluginInitialization(plugins) { }); } -export function trackSelectorMetrics() { - setTimeout(() => { - if (!shouldTrackPerformance()) { - return; - } - - const selectors = getSortedTrackedSelectors(); - const filteredSelectors = selectors.filter((selector) => selector.calls > 5); - - trackEvent('performance', 'least_effective_selectors', { - after: 'one_minute', - first: filteredSelectors[0]?.name || '', - first_effectiveness: filteredSelectors[0]?.effectiveness, - first_recomputations: filteredSelectors[0]?.recomputations, - second: filteredSelectors[1]?.name || '', - second_effectiveness: filteredSelectors[1]?.effectiveness, - second_recomputations: filteredSelectors[1]?.recomputations, - third: filteredSelectors[2]?.name || '', - third_effectiveness: filteredSelectors[2]?.effectiveness, - third_recomputations: filteredSelectors[2]?.recomputations, - }); - }, 60000); -} - let requestCount = 0; const requestCountAtMark = {}; let requestObserver; diff --git a/webapp/channels/src/components/root/root.tsx b/webapp/channels/src/components/root/root.tsx index 150b3ee3018..89348bc3767 100644 --- a/webapp/channels/src/components/root/root.tsx +++ b/webapp/channels/src/components/root/root.tsx @@ -14,7 +14,7 @@ import {setUrl} from 'mattermost-redux/actions/general'; import {Client4} from 'mattermost-redux/client'; import {Preferences} from 'mattermost-redux/constants'; -import {measurePageLoadTelemetry, temporarilySetPageLoadContext, trackEvent, trackSelectorMetrics} from 'actions/telemetry_actions.jsx'; +import {measurePageLoadTelemetry, temporarilySetPageLoadContext, trackEvent} from 'actions/telemetry_actions.jsx'; import BrowserStore from 'stores/browser_store'; import {makeAsyncComponent, makeAsyncPluggableComponent} from 'components/async_load'; @@ -360,7 +360,6 @@ export default class Root extends React.PureComponent { this.initiateMeRequests(); measurePageLoadTelemetry(); - trackSelectorMetrics(); // Force logout of all tabs if one tab is logged out window.addEventListener('storage', this.handleLogoutLoginSignal); diff --git a/webapp/channels/src/packages/mattermost-redux/src/selectors/create_selector/index.js b/webapp/channels/src/packages/mattermost-redux/src/selectors/create_selector/index.js index 2b9bad8c005..5b6f32d40e4 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/selectors/create_selector/index.js +++ b/webapp/channels/src/packages/mattermost-redux/src/selectors/create_selector/index.js @@ -3,26 +3,6 @@ /* eslint-disable */ -// Generates a RFC-4122 version 4 compliant globally unique identifier. -export function generateId() { - // implementation taken from http://stackoverflow.com/a/2117523 - let id = 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'; - id = id.replace(/[xy]/g, (c) => { - const r = Math.floor(Math.random() * 16); - let v; - - if (c === 'x') { - v = r; - } else { - // eslint-disable-next-line no-mixed-operators - v = r & 0x3 | 0x8; - } - - return v.toString(16); - }); - return id; -} - function defaultEqualityCheck(a, b) { return a === b; } @@ -43,7 +23,7 @@ function areArgumentsShallowlyEqual(equalityCheck, prev, next) { return true; } -export function defaultMemoize(func, measure, equalityCheck = defaultEqualityCheck) { +export function defaultMemoize(func, equalityCheck = defaultEqualityCheck) { let lastArgs = null let lastResult = null // we reference arguments instead of spreading them for performance reasons @@ -53,9 +33,6 @@ export function defaultMemoize(func, measure, equalityCheck = defaultEqualityChe lastResult = func.apply(null, arguments) } - if (measure) { - measure(); - } lastArgs = arguments return lastResult } @@ -77,25 +54,16 @@ function getDependencies(funcs) { return dependencies; } -const trackedSelectors = {}; - export function createSelectorCreator(memoize, ...memoizeOptions) { return (name, ...funcs) => { - const id = generateId(); - let recomputations = 0; - let calls = 0; const resultFunc = funcs.pop(); const dependencies = getDependencies(funcs); const memoizedResultFunc = memoize( function() { - recomputations++; - trackedSelectors[id].recomputations++; - // apply arguments instead of spreading for performance. return resultFunc?.apply(null, arguments); }, - null, ...memoizeOptions, ); @@ -111,23 +79,10 @@ export function createSelectorCreator(memoize, ...memoizeOptions) { // apply arguments instead of spreading for performance. return memoizedResultFunc.apply(null, params); - }, - () => { - calls++; - trackedSelectors[id].calls++; }); selector.resultFunc = resultFunc; selector.dependencies = dependencies; - selector.recomputations = () => recomputations; - selector.resetRecomputations = () => recomputations = 0; - - trackedSelectors[id] = { - id, - name, - calls: 0, - recomputations: 0, - }; return selector; }; @@ -153,52 +108,3 @@ export function createStructuredSelector(selectors, selectorCreator = createSele }, ); } - -// resetTrackedSelectors resets all the measurements for memoization effectiveness. -function resetTrackedSelectors() { - Object.values(trackedSelectors).forEach((selector) => { - selector.calls = 0; - selector.recomputations = 0; - }); -} - -// getSortedTrackedSelectors returns an array, sorted by effectivness, containing mesaurement data on all tracked selectors. -export function getSortedTrackedSelectors() { - let selectors = Object.values(trackedSelectors); - // Filter out any selector not called - selectors = selectors.filter(selector => selector.calls > 0); - const selectorsData = selectors.map((selector) => ({name: selector.name, effectiveness: effectiveness(selector), recomputations: selector.recomputations, calls: selector.calls})); - selectorsData.sort((a, b) => { - // Sort effectiveness ascending - if (a.effectiveness !== b.effectiveness) { - return a.effectiveness - b.effectiveness; - } - - // And everything else descending - if (a.recomputations !== b.recomputations) { - return b.recomputations - a.recomputations; - } - - if (a.calls !== b.calls) { - return b.calls - a.calls; - } - - return a.name.localeCompare(b.name); - }); - return selectorsData; -} - -function effectiveness(selector) { - return 100 - ((selector.recomputations / selector.calls) * 100); -} - -// dumpTrackedSelectorsStatistics prints to console a table containing the measurement data on all tracked selectors. -function dumpTrackedSelectorsStatistics() { - const selectors = getSortedTrackedSelectors(); - console.table(selectors); //eslint-disable-line no-console -} - -window.dumpTrackedSelectorsStatistics = dumpTrackedSelectorsStatistics; -window.resetTrackedSelectors = resetTrackedSelectors; -window.getSortedTrackedSelectors = getSortedTrackedSelectors - diff --git a/webapp/channels/src/packages/mattermost-redux/src/utils/post_list.test.ts b/webapp/channels/src/packages/mattermost-redux/src/utils/post_list.test.ts index 7d2e1397e0c..0c4fea6e3d6 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/utils/post_list.test.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/utils/post_list.test.ts @@ -239,241 +239,6 @@ describe('makeFilterPostsAndAddSeparators', () => { 'date-' + (today.getTime() + 1000), ]); }); - - it('memoization', () => { - const filterPostsAndAddSeparators = makeFilterPostsAndAddSeparators(); - const time = Date.now(); - const today = new Date(time); - const tomorrow = new Date((24 * 60 * 60 * 1000) + today.getTime()); - - // Posts 7 hours apart so they should appear on multiple days - const initialPosts = { - 1001: {id: '1001', create_at: time, type: ''}, - 1002: {id: '1002', create_at: time + 5, type: ''}, - 1003: {id: '1003', create_at: time + 10, type: ''}, - 1004: {id: '1004', create_at: tomorrow, type: ''}, - 1005: {id: '1005', create_at: tomorrow as any + 5, type: ''}, - 1006: {id: '1006', create_at: tomorrow as any + 10, type: Posts.POST_TYPES.JOIN_CHANNEL}, - }; - let state = { - entities: { - general: { - config: {}, - }, - posts: { - posts: initialPosts, - }, - preferences: { - myPreferences: { - [getPreferenceKey(Preferences.CATEGORY_ADVANCED_SETTINGS, Preferences.ADVANCED_FILTER_JOIN_LEAVE)]: { - category: Preferences.CATEGORY_ADVANCED_SETTINGS, - name: Preferences.ADVANCED_FILTER_JOIN_LEAVE, - value: 'true', - }, - }, - }, - users: { - currentUserId: '1234', - profiles: { - 1234: {id: '1234', username: 'user'}, - }, - }, - }, - } as unknown as GlobalState; - - let postIds = [ - '1006', - '1004', - '1003', - '1001', - ]; - let lastViewedAt = initialPosts['1001'].create_at + 1; - - let now = filterPostsAndAddSeparators(state, {postIds, lastViewedAt, indicateNewMessages: true}); - expect(now).toEqual([ - '1006', - '1004', - 'date-' + tomorrow.getTime(), - '1003', - START_OF_NEW_MESSAGES + lastViewedAt, - '1001', - 'date-' + today.getTime(), - ]); - - // No changes - let prev = now; - now = filterPostsAndAddSeparators(state, {postIds, lastViewedAt, indicateNewMessages: true}); - expect(now).toBe(prev); - expect(now).toEqual([ - '1006', - '1004', - 'date-' + tomorrow.getTime(), - '1003', - START_OF_NEW_MESSAGES + lastViewedAt, - '1001', - 'date-' + today.getTime(), - ]); - - // lastViewedAt changed slightly - lastViewedAt = initialPosts['1001'].create_at + 2; - - prev = now; - now = filterPostsAndAddSeparators(state, {postIds, lastViewedAt, indicateNewMessages: true}); - expect(now).not.toBe(prev); - expect(now).toEqual([ - '1006', - '1004', - 'date-' + tomorrow.getTime(), - '1003', - START_OF_NEW_MESSAGES + lastViewedAt, - '1001', - 'date-' + today.getTime(), - ]); - - // lastViewedAt changed a lot - lastViewedAt = initialPosts['1003'].create_at + 1; - - prev = now; - now = filterPostsAndAddSeparators(state, {postIds, lastViewedAt, indicateNewMessages: true}); - expect(now).not.toBe(prev); - expect(now).toEqual([ - '1006', - '1004', - START_OF_NEW_MESSAGES + lastViewedAt, - 'date-' + tomorrow.getTime(), - '1003', - '1001', - 'date-' + today.getTime(), - ]); - - prev = now; - now = filterPostsAndAddSeparators(state, {postIds, lastViewedAt, indicateNewMessages: true}); - expect(now).toBe(prev); - expect(now).toEqual([ - '1006', - '1004', - START_OF_NEW_MESSAGES + lastViewedAt, - 'date-' + tomorrow.getTime(), - '1003', - '1001', - 'date-' + today.getTime(), - ]); - - // postIds changed, but still shallowly equal - postIds = [...postIds]; - - prev = now; - now = filterPostsAndAddSeparators(state, {postIds, lastViewedAt, indicateNewMessages: true}); - expect(now).toBe(prev); - expect(now).toEqual([ - '1006', - '1004', - START_OF_NEW_MESSAGES + lastViewedAt, - 'date-' + tomorrow.getTime(), - '1003', - '1001', - 'date-' + today.getTime(), - ]); - - // Post changed, not in postIds - state = { - ...state, - entities: { - ...state.entities, - posts: { - ...state.entities.posts, - posts: { - ...state.entities.posts.posts, - 1007: {id: '1007', create_at: 7 * 60 * 60 * 7 * 1000}, - }, - }, - }, - } as unknown as GlobalState; - - prev = now; - now = filterPostsAndAddSeparators(state, {postIds, lastViewedAt, indicateNewMessages: true}); - expect(now).toBe(prev); - expect(now).toEqual([ - '1006', - '1004', - START_OF_NEW_MESSAGES + lastViewedAt, - 'date-' + tomorrow.getTime(), - '1003', - '1001', - 'date-' + today.getTime(), - ]); - - // Post changed, in postIds - state = { - ...state, - entities: { - ...state.entities, - posts: { - ...state.entities.posts, - posts: { - ...state.entities.posts.posts, - 1006: {...state.entities.posts.posts['1006'], message: 'abcd'}, - }, - }, - }, - }; - - prev = now; - now = filterPostsAndAddSeparators(state, {postIds, lastViewedAt, indicateNewMessages: true}); - expect(now).toBe(prev); - expect(now).toEqual([ - '1006', - '1004', - START_OF_NEW_MESSAGES + lastViewedAt, - 'date-' + tomorrow.getTime(), - '1003', - '1001', - 'date-' + today.getTime(), - ]); - - // Filter changed - state = { - ...state, - entities: { - ...state.entities, - preferences: { - ...state.entities.preferences, - myPreferences: { - ...state.entities.preferences.myPreferences, - [getPreferenceKey(Preferences.CATEGORY_ADVANCED_SETTINGS, Preferences.ADVANCED_FILTER_JOIN_LEAVE)]: { - category: Preferences.CATEGORY_ADVANCED_SETTINGS, - name: Preferences.ADVANCED_FILTER_JOIN_LEAVE, - value: 'false', - }, - } as unknown as GlobalState['entities']['preferences']['myPreferences'], - }, - }, - }; - - prev = now; - now = filterPostsAndAddSeparators(state, {postIds, lastViewedAt, indicateNewMessages: true}); - expect(now).not.toBe(prev); - expect(now).toEqual([ - '1004', - START_OF_NEW_MESSAGES + lastViewedAt, - 'date-' + tomorrow.getTime(), - '1003', - '1001', - 'date-' + today.getTime(), - ]); - - prev = now; - now = filterPostsAndAddSeparators(state, {postIds, lastViewedAt, indicateNewMessages: true}); - expect(now).toBe(prev); - expect(now).toEqual([ - '1004', - START_OF_NEW_MESSAGES + lastViewedAt, - 'date-' + tomorrow.getTime(), - '1003', - '1001', - 'date-' + today.getTime(), - ]); - }); }); describe('makeCombineUserActivityPosts', () => { @@ -705,155 +470,6 @@ describe('makeCombineUserActivityPosts', () => { expect(result).toHaveLength(2); }); - - describe('memoization', () => { - const initialPostIds = ['post1', 'post2']; - const initialState = { - entities: { - posts: { - posts: { - post1: {id: 'post1', type: Posts.POST_TYPES.JOIN_CHANNEL}, - post2: {id: 'post2', type: Posts.POST_TYPES.JOIN_CHANNEL}, - }, - }, - }, - } as unknown as GlobalState; - - test('should not recalculate when nothing has changed', () => { - const combineUserActivityPosts = makeCombineUserActivityPosts(); - - expect(combineUserActivityPosts.recomputations()).toBe(0); - - combineUserActivityPosts(initialState, initialPostIds); - - expect(combineUserActivityPosts.recomputations()).toBe(1); - - combineUserActivityPosts(initialState, initialPostIds); - - expect(combineUserActivityPosts.recomputations()).toBe(1); - }); - - test('should recalculate when the post IDs change', () => { - const combineUserActivityPosts = makeCombineUserActivityPosts(); - - let postIds = initialPostIds; - combineUserActivityPosts(initialState, postIds); - - expect(combineUserActivityPosts.recomputations()).toBe(1); - - postIds = ['post1']; - combineUserActivityPosts(initialState, postIds); - - expect(combineUserActivityPosts.recomputations()).toBe(2); - }); - - test('should not recalculate when an unrelated state change occurs', () => { - const combineUserActivityPosts = makeCombineUserActivityPosts(); - - let state = initialState; - combineUserActivityPosts(state, initialPostIds); - - expect(combineUserActivityPosts.recomputations()).toBe(1); - - state = { - ...state, - entities: { - ...state.entities, - posts: { - ...state.entities.posts, - messagesHistory: { - messages: [], - index: { - post: 4, - comment: 3, - }, - }, - }, - }, - }; - combineUserActivityPosts(state, initialPostIds); - - expect(combineUserActivityPosts.recomputations()).toBe(1); - }); - - test('should not recalculate if an unrelated post changes', () => { - const combineUserActivityPosts = makeCombineUserActivityPosts(); - - let state = initialState; - const initialResult = combineUserActivityPosts(state, initialPostIds); - - expect(combineUserActivityPosts.recomputations()).toBe(1); - - // An unrelated post changed - state = { - ...state, - entities: { - ...state.entities, - posts: { - ...state.entities.posts, - posts: { - ...state.entities.posts.posts, - post3: TestHelper.getPostMock({id: 'post3'}), - }, - }, - }, - }; - const result = combineUserActivityPosts(state, initialPostIds); - - // The selector didn't recalculate so the result didn't change - expect(combineUserActivityPosts.recomputations()).toBe(1); - expect(result).toBe(initialResult); - }); - - test('should return the same result when a post changes in a way that doesn\'t affect the result', () => { - const combineUserActivityPosts = makeCombineUserActivityPosts(); - - let state = initialState; - const initialResult = combineUserActivityPosts(state, initialPostIds); - - expect(combineUserActivityPosts.recomputations()).toBe(1); - - // One of the posts was updated, but post type didn't change - state = { - ...state, - entities: { - ...state.entities, - posts: { - ...state.entities.posts, - posts: { - ...state.entities.posts.posts, - post2: {...state.entities.posts.posts.post2, update_at: 1234}, - }, - }, - }, - }; - let result = combineUserActivityPosts(state, initialPostIds); - - // The selector recalculated but is still returning the same array - expect(combineUserActivityPosts.recomputations()).toBe(2); - expect(result).toBe(initialResult); - - // One of the posts changed type - state = { - ...state, - entities: { - ...state.entities, - posts: { - ...state.entities.posts, - posts: { - ...state.entities.posts.posts, - post2: {...state.entities.posts.posts.post2, type: ''}, - }, - }, - }, - }; - result = combineUserActivityPosts(state, initialPostIds); - - // The selector recalculated, and the result changed - expect(combineUserActivityPosts.recomputations()).toBe(3); - expect(result).not.toBe(initialResult); - }); - }); }); describe('isDateLine', () => { @@ -1052,106 +668,6 @@ describe('makeGenerateCombinedPost', () => { metadata: {}, }); }); - - describe('memoization', () => { - const initialState = { - entities: { - posts: { - posts: { - post1: {id: 'post1'}, - post2: {id: 'post2'}, - }, - }, - }, - } as unknown as GlobalState; - const initialCombinedId = 'user-activity-post1_post2'; - - test('should not recalculate when called twice with the same ID', () => { - const generateCombinedPost = makeGenerateCombinedPost(); - - expect((generateCombinedPost as any).recomputations()).toBe(0); - - generateCombinedPost(initialState, initialCombinedId); - - expect((generateCombinedPost as any).recomputations()).toBe(1); - - generateCombinedPost(initialState, initialCombinedId); - - expect((generateCombinedPost as any).recomputations()).toBe(1); - }); - - test('should recalculate when called twice with different IDs', () => { - const generateCombinedPost = makeGenerateCombinedPost(); - - expect((generateCombinedPost as any).recomputations()).toBe(0); - - let combinedId = initialCombinedId; - generateCombinedPost(initialState, combinedId); - - expect((generateCombinedPost as any).recomputations()).toBe(1); - - combinedId = 'user-activity-post2'; - generateCombinedPost(initialState, combinedId); - - expect((generateCombinedPost as any).recomputations()).toBe(2); - }); - - test('should not recalculate when a different post changes', () => { - const generateCombinedPost = makeGenerateCombinedPost(); - - expect((generateCombinedPost as any).recomputations()).toBe(0); - - let state = initialState; - generateCombinedPost(state, initialCombinedId); - - expect((generateCombinedPost as any).recomputations()).toBe(1); - - state = { - ...state, - entities: { - ...state.entities, - posts: { - ...state.entities.posts, - posts: { - ...state.entities.posts.posts, - post3: TestHelper.getPostMock({id: 'post3'}), - }, - }, - }, - }; - generateCombinedPost(state, initialCombinedId); - - expect((generateCombinedPost as any).recomputations()).toBe(2); - }); - - test('should recalculate when one of the included posts change', () => { - const generateCombinedPost = makeGenerateCombinedPost(); - - expect((generateCombinedPost as any).recomputations()).toBe(0); - - let state = initialState; - generateCombinedPost(state, initialCombinedId); - - expect((generateCombinedPost as any).recomputations()).toBe(1); - - state = { - ...state, - entities: { - ...state.entities, - posts: { - ...state.entities.posts, - posts: { - ...state.entities.posts.posts, - post2: TestHelper.getPostMock({id: 'post2', update_at: 1234}), - }, - }, - }, - }; - generateCombinedPost(state, initialCombinedId); - - expect((generateCombinedPost as any).recomputations()).toBe(2); - }); - }); }); const PostTypes = Posts.POST_TYPES; describe('extractUserActivityData', () => {