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: add performance metrics #495

Merged
merged 2 commits into from
Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lib/constants/performance-marks.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export default {
Copy link
Member Author

Choose a reason for hiding this comment

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

Нужны вот эти метки, потому что они лучше отражают, на что тратится время

  • время от открытия страницы до исполнения JS (network)
  • время до загрузки баз с тестами (network)
  • время до слияния баз в одну (cpu)
  • время до загрузки плагинов (network)
  • время до получения строк из единой базы (cpu)
  • время до полной загрузки отчета: построение дерева, инициализация redux store… (cpu)

JS_EXEC: 'js exec',
DBS_LOADED: 'dbs loaded',
DBS_MERGED: 'dbs merged',
PLUGINS_LOADED: 'plugins loaded',
DB_EXTRACTED_ROWS: 'db extracted rows',
FULLY_LOADED: 'fully loaded'
} as const;
16 changes: 16 additions & 0 deletions lib/static/modules/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {getHttpErrorMessage} from './utils';
import {fetchDataFromDatabases, mergeDatabases, connectToDatabase, getMainDatabaseUrl, getSuitesTableRows} from '../../db-utils/client';
import {setFilteredBrowsers} from './query-params';
import plugins from './plugins';
import performanceMarks from '../../constants/performance-marks';

export const createNotification = (id, status, message, props = {}) => {
const notificationProps = {
Expand All @@ -31,14 +32,19 @@ export const dismissNotification = dismissNotify;

export const initGuiReport = () => {
return async (dispatch) => {
performance?.mark?.(performanceMarks.JS_EXEC);
try {
const appState = await axios.get('/init');

const mainDatabaseUrl = getMainDatabaseUrl();
const db = await connectToDatabase(mainDatabaseUrl.href);

performance?.mark?.(performanceMarks.DBS_LOADED);

await plugins.loadAll(appState.data.config);

performance?.mark?.(performanceMarks.PLUGINS_LOADED);

dispatch({
type: actionNames.INIT_GUI_REPORT,
payload: {...appState.data, db}
Expand All @@ -58,6 +64,7 @@ export const initGuiReport = () => {

export const initStaticReport = () => {
KuznetsovRoman marked this conversation as resolved.
Show resolved Hide resolved
return async dispatch => {
performance?.mark?.(performanceMarks.JS_EXEC);
Copy link
Member Author

Choose a reason for hiding this comment

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

Здесь и далее использую performance?.mark?. вместо performance.mark на случай, если performance API не поддерживается

Использую именно Performance API вместо скрипта, записывающего Date.now() выполнения, чтобы также получить и время до загрузки html

const dataFromStaticFile = window.data || {};
let fetchDbDetails = [];
let db = null;
Expand All @@ -66,17 +73,23 @@ export const initStaticReport = () => {
const mainDatabaseUrls = new URL('databaseUrls.json', window.location.href);
const fetchDbResponses = await fetchDataFromDatabases([mainDatabaseUrls.href]);

performance?.mark?.(performanceMarks.DBS_LOADED);
Copy link
Member

Choose a reason for hiding this comment

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

Кмк в таких метриках не хватает инфы сколько мы в итоге базок загружали, сколько мерджили, сколько строк из базок выгружали и т.д.

Copy link
Member Author

Choose a reason for hiding this comment

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

Есть testCount


fetchDbDetails = fetchDbResponses.map(({url, status, data}) => ({url, status, success: !!data}));

const dataForDbs = fetchDbResponses.map(({data}) => data).filter(data => data);

db = await mergeDatabases(dataForDbs);

performance?.mark?.(performanceMarks.DBS_MERGED);
} catch (e) {
dispatch(createNotificationError('initStaticReport', e));
}

await plugins.loadAll(dataFromStaticFile.config);

performance?.mark?.(performanceMarks.PLUGINS_LOADED);

if (!db || isEmpty(fetchDbDetails)) {
return dispatch({
type: actionNames.INIT_STATIC_REPORT,
Expand All @@ -86,6 +99,9 @@ export const initStaticReport = () => {

const testsTreeBuilder = StaticTestsTreeBuilder.create();
const suitesRows = getSuitesTableRows(db);

performance?.mark?.(performanceMarks.DB_EXTRACTED_ROWS);

const {tree, stats, skips, browsers} = testsTreeBuilder.build(suitesRows);

dispatch({
Expand Down
37 changes: 35 additions & 2 deletions lib/static/modules/middlewares/metrika.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import {get} from 'lodash';
import {get, isEmpty} from 'lodash';
import actionNames from '../action-names';

import {measurePerformance} from '../web-vitals';
import performanceMarks from '../../../constants/performance-marks';

let metrika;
let reportFullyLoaded = false;

export default metrikaClass => store => next => action => {
switch (action.type) {
Expand Down Expand Up @@ -58,6 +60,28 @@ export default metrikaClass => store => next => action => {
return next(action);
}

case actionNames.BROWSERS_SELECTED: {
execOnMetrikaEnabled(() => {
sendCounterId(action.type);
});

const result = next(action);

if (!reportFullyLoaded) {
reportFullyLoaded = true;

performance?.mark?.(performanceMarks.FULLY_LOADED);

const marks = extractPerformanceMarks();

Copy link
Member

Choose a reason for hiding this comment

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

А почему эту логики выполняешь на browsers_selected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Потому что пока первый browsers_selected не выполнится, ты все еще будешь смотреть на загрузку, и по сути отчет еще не загружен

if (metrika && !isEmpty(marks)) {
metrika.sendVisitParams(marks);
}
}

return result;
}

Comment on lines +63 to +83
Copy link
Member Author

Choose a reason for hiding this comment

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

После INIT_STATIC_REPORT страница еще не интерактивна. На самом деле, отчет полностью прогружается после первого BROWSERS_SELECT

Copy link
Member Author

Choose a reason for hiding this comment

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

UPD: добавил проверку, чтоб собирать данные только при state.gui === false

Copy link
Member

Choose a reason for hiding this comment

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

А тесты не хочешь на этот код набросать? У нас модуль метрики покрыт тестами.

Copy link
Member Author

Choose a reason for hiding this comment

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

Тут reportFullyLoaded будет вне зоны видимости тестов, да и к тому же с performance api из-за 14 ноды придется перезаписывать global.performance: 14 нода такое не поддерживает.

В общем, код тестов получится намного более сложным этого кода

case actionNames.RUN_ALL_TESTS:
case actionNames.RUN_FAILED_TESTS:
case actionNames.RETRY_SUITE:
Expand All @@ -75,7 +99,6 @@ export default metrikaClass => store => next => action => {
case actionNames.VIEW_SWITCH_DIFF:
case actionNames.VIEW_SWIPE_DIFF:
case actionNames.VIEW_ONION_SKIN_DIFF:
case actionNames.BROWSERS_SELECTED:
case actionNames.VIEW_UPDATE_FILTER_BY_NAME:
case actionNames.VIEW_SET_STRICT_MATCH_FILTER:
case actionNames.RUN_CUSTOM_GUI_ACTION:
Expand Down Expand Up @@ -119,3 +142,13 @@ function execOnMetrikaEnabled(cb) {
function sendCounterId(counterId) {
metrika.sendVisitParams({counterId});
}

function extractPerformanceMarks() {
const marks = performance?.getEntriesByType?.('mark') || [];

return marks.reduce((acc, {name, startTime}) => {
acc[name] = Math.round(startTime);

return acc;
}, {});
}
3 changes: 3 additions & 0 deletions test/unit/lib/static/modules/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ import {StaticTestsTreeBuilder} from 'lib/tests-tree-builder/static';
import {LOCAL_DATABASE_NAME} from 'lib/constants/database';
import {DiffModes} from 'lib/constants/diff-modes';

// eslint-disable-next-line
globalThis.performance = globalThis.performance; // node v14 stub
Copy link
Member Author

Choose a reason for hiding this comment

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

node v14 не умеет в performance и кидает ошибку: performance is undefined из-за strict mode


describe('lib/static/modules/actions', () => {
const sandbox = sinon.sandbox.create();
let dispatch, actions, notify, getSuitesTableRows, getMainDatabaseUrl, connectToDatabaseStub, pluginsStub;
Expand Down
3 changes: 3 additions & 0 deletions test/unit/lib/static/modules/middlewares/metrika.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ import YandexMetrika from 'lib/static/modules/yandex-metrika';
import actionNames from 'lib/static/modules/action-names';
import webVitals from 'lib/static/modules/web-vitals';

// eslint-disable-next-line
globalThis.performance = globalThis.performance; // node v14 stub

describe('lib/static/modules/middlewares/metrika', () => {
const sandbox = sinon.sandbox.create();
let next, metrikaMiddleware;
Expand Down
Loading