Repository navigation
[HOLD for payment 2024-11-29] [HOLD for payment 2024-11-21] Align Firebase performance metrics with core in-app workflows #51742
Description
Coming from this Slack thread: https://expensify.slack.com/archives/C05LX9D6E07/p1730214849510819
Problem
Our performance metrics do not represent exactly what we care about when tracking App's performance across the user base. They either have unclear naming, or code markers that are put in the wrong places, making measurements incorrect and giving us falsy results.
Solution
Let's align the definitions and fix code markers' placement to make sure we collect whole user interactions when people eg. open chats from different scenarios, send message or open up search. Detailed changes in the first comment.
Activity
adhorodyski commentedon Oct 30, 2024
homepage_initial_renderdoes not serve as a meaningful measure of performance with a misleading event name and not appropriate code markers (should be in different components + AuthScreens is not fully laying out). I advocate for removing this event.switch_reportcan be deprecated in favour of specificopen_report*events that take into account different scenarios and have a more correct naming.chat_renderdoes not effectively measure what it's name describes (component fully laying out). I advocate for removing this event and focus on events that better mark full user interactions.sidebar_loadeddoes not measure anything meaningful in the performance context. I advocate for removing it (at least from tracking) as it's based on the falsy end event (LHN is not interactive when we hide the splash screen). Additionally, thesidebarnaming is misleading.search_renderis no longer being used. Either docs need to be updated to account for the newsearch_router_render, or naming can be improved toopen_searchto be more descriptive.report_initial_renderdoes not measure anything meaningful, we haveopen_report*events for this use case. The start marker it put in the wrong place causing false results. I advocate for removing this event.message_sentis currently not being sent to Firebase. I advocate for adding a Firebase call for it so we can start tracking how long it takes for people to send messages.open_reportis currently not being sent to Firebase. Let's add a Firebase call for it so we can start tracking how long it takes for people to fully navigate to chats from LHN.SEARCH_FILTER_OPTIONSneeds to be updated in docs (Finder is now deprecatedSEARCH_BUILD_TREE,SEARCH_MAKE_TREEandSEARCH_CONVERT_SEARCH_VALUESall need to be documentedadhorodyski commentedon Oct 30, 2024
Adam from Callstack, I'd like to work on this issue.
adhorodyski commentedon Oct 31, 2024
@kirillzyusko I noticed we're relying on the
chat_renderevent here but it's not being trigger anywhere. Can this be also removed from our E2E suite? I think we can realign this afterwards based on the newly set "correct" events.adhorodyski commentedon Oct 31, 2024
E2E tests depend on the events being trigger - for now I'll intentionally remove deprecated events in the PR, breaking E2E in the process so that we have a clear diff for what I have to fix before we merge anything.
adhorodyski commentedon Oct 31, 2024
Created a draft PR as it's in progress: #51815
adhorodyski commentedon Nov 4, 2024
Updated the PR, now I need to work on bringing e2e tests back to life.
adhorodyski commentedon Nov 5, 2024
asked @kirillzyusko for a review as e2e's are ~close to being back alive 😅
kirillzyusko commentedon Nov 7, 2024
e2e tests are fully back now and works pretty stable 😎
adhorodyski commentedon Nov 8, 2024
@kirillzyusko oh you mean on this branch? 🤞
adhorodyski commentedon Nov 8, 2024
This is opened for a review now.
13 remaining items
melvin-bot commentedon Nov 21, 2024
Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.
adhorodyski commentedon Nov 22, 2024
I'm currently reworking the StartupTimer native module to work with multiple traces based on a map (the same way we do it with JS). This will enable a true TTI measurement the same way we do track the
js_loadedevent.It's only for Firebase, I have an idea on how we can enable generic tracing for multiple reporters (Grafana as well) for all metrics. I'll followup with a separate proposal for that.
[-][HOLD for payment 2024-11-21] Align Firebase performance metrics with core in-app workflows[/-][+][HOLD for payment 2024-11-29] [HOLD for payment 2024-11-21] Align Firebase performance metrics with core in-app workflows[/+]melvin-bot commentedon Nov 22, 2024
Reviewinglabel has been removed, please complete the "BugZero Checklist".melvin-bot commentedon Nov 22, 2024
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.65-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-11-29. 🎊
For reference, here are some details about the assignees on this issue:
melvin-bot commentedon Nov 29, 2024
Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.
mountiny commentedon Nov 29, 2024
@adhorodyski anything else for this PR?
adhorodyski commentedon Nov 29, 2024
I don't think so, let's close this issue - core workflows are covered.
I'm still wrapping my head around the best way of reporting the TTI for all systems. We don't want to hardcode this for Firebase only, I'll propose something else to be more generic and also enable pushing to Grafana at the same time (collecting between threads is problematic).
mountiny commentedon Nov 30, 2024
Thanks, yes I think that sounds like a good path