-
Notifications
You must be signed in to change notification settings - Fork 39
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
Conversation
@@ -0,0 +1,8 @@ | |||
export default { |
There was a problem hiding this comment.
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)
@@ -58,6 +59,7 @@ export const initGuiReport = () => { | |||
|
|||
export const initStaticReport = () => { | |||
return async dispatch => { | |||
performance?.mark?.(performanceMarks.JS_EXEC); |
There was a problem hiding this comment.
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
case actionNames.BROWSERS_SELECTED: { | ||
execOnMetrikaEnabled(() => { | ||
sendCounterId(action.type); | ||
}); | ||
|
||
const result = next(action); | ||
|
||
if (!reportFullyLoaded) { | ||
reportFullyLoaded = true; | ||
|
||
performance?.mark?.(performanceMarks.FULLY_LOADED); | ||
|
||
const marks = extractPerformanceMarks(); | ||
|
||
if (metrika && !isEmpty(marks)) { | ||
metrika.sendVisitParams(marks); | ||
} | ||
} | ||
|
||
return result; | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPD: добавил проверку, чтоб собирать данные только при state.gui === false
be9a58e
to
c038aad
Compare
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В остальном /ok
|
||
performance?.mark?.(performanceMarks.FULLY_LOADED); | ||
|
||
const marks = extractPerformanceMarks(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А почему эту логики выполняешь на browsers_selected
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Потому что пока первый browsers_selected
не выполнится, ты все еще будешь смотреть на загрузку, и по сути отчет еще не загружен
@@ -66,17 +68,23 @@ export const initStaticReport = () => { | |||
const mainDatabaseUrls = new URL('databaseUrls.json', window.location.href); | |||
const fetchDbResponses = await fetchDataFromDatabases([mainDatabaseUrls.href]); | |||
|
|||
performance?.mark?.(performanceMarks.DBS_LOADED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Кмк в таких метриках не хватает инфы сколько мы в итоге базок загружали, сколько мерджили, сколько строк из базок выгружали и т.д.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Есть testCount
} | ||
|
||
return result; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А тесты не хочешь на этот код набросать? У нас модуль метрики покрыт тестами.
There was a problem hiding this comment.
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 нода такое не поддерживает.
В общем, код тестов получится намного более сложным этого кода
541aa1f
to
42d71fc
Compare
a78a8b9
to
70f93a9
Compare
Что сделано
Добавил метрики открытия отчета