-
Notifications
You must be signed in to change notification settings - Fork 4k
[No QA] Fix is_warm labeling on report-open spans #98852
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,24 +1,26 @@ | ||
| import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID'; | ||
| import markOpenReportEnd from '@libs/telemetry/markOpenReportEnd'; | ||
|
|
||
| import type * as OnyxTypes from '@src/types/onyx'; | ||
|
|
||
| import type {OnyxEntry} from 'react-native-onyx'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
|
|
||
| import {useEffect} from 'react'; | ||
|
|
||
| import useOnyx from './useOnyx'; | ||
|
|
||
| /** | ||
| * Closes the open-report performance span as a cold open (`warm: false`) the moment a cold-load skeleton | ||
| * appears. Shared by every report surface that gates its own skeleton (the chat-list guard, the route | ||
| * orchestrator, and the money-request views): the open-report span must still close even though the list | ||
| * body — which would otherwise own this mark once mounted — isn't mounted while the skeleton shows. | ||
| * Closes the open-report span as a cold open (`warm: false`) while a skeleton shows. Call it from a component | ||
| * mounted exactly while the skeleton is on screen — the list body that would otherwise close the span isn't | ||
| * mounted yet. | ||
| */ | ||
| function useMarkOpenReportEndOnSkeleton(report: OnyxEntry<OnyxTypes.Report>, isSkeletonVisible: boolean) { | ||
| function useMarkOpenReportEndOnSkeleton(reportID: string | undefined, isSkeletonVisible = true) { | ||
| const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${getNonEmptyStringOnyxID(reportID)}`); | ||
|
|
||
| useEffect(() => { | ||
| if (!isSkeletonVisible || !report) { | ||
| if (!isSkeletonVisible || !reportID) { | ||
| return; | ||
| } | ||
| markOpenReportEnd(report, {warm: false}); | ||
| }, [report, isSkeletonVisible]); | ||
| markOpenReportEnd(reportID, report, {warm: false}); | ||
| }, [reportID, report, isSkeletonVisible]); | ||
| } | ||
|
|
||
| export default useMarkOpenReportEndOnSkeleton; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import CONST from '@src/CONST'; | |
| import type * as OnyxTypes from '@src/types/onyx'; | ||
|
|
||
| import type {SpanAttributes} from '@sentry/core'; | ||
| import type {OnyxEntry} from 'react-native-onyx'; | ||
|
|
||
| import {endSpanWithAttributes} from './activeSpans'; | ||
|
|
||
|
|
@@ -12,11 +13,10 @@ type MarkOpenReportEndOptions = { | |
| }; | ||
|
|
||
| /** | ||
| * Mark all 'open_report*' telemetry spans as finished. | ||
| * Mark all 'open_report*' telemetry spans as finished. Keyed by `reportID` so it still ends when the report | ||
| * hasn't loaded; the report-shape attributes are then left off. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ CONSISTENCY-16 (docs)Comments should use two separate sentences instead of a semicolon. This JSDoc line joins two independent statements with a semicolon. Split into two sentences: * Mark all 'open_report*' telemetry spans as finished. Keyed by `reportID` so it still ends when the report
* hasn't loaded. The report-shape attributes are then left off.Reviewed at: f281e4a | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency. |
||
| */ | ||
| function markOpenReportEnd(report: OnyxTypes.Report, options: MarkOpenReportEndOptions = {}) { | ||
| const {reportID, type, chatType} = report; | ||
|
|
||
| function markOpenReportEnd(reportID: string, report: OnyxEntry<OnyxTypes.Report>, options: MarkOpenReportEndOptions = {}) { | ||
| const isTransactionThread = isReportTransactionThread(report); | ||
| const isOneTransactionThread = isOneTransactionReport(report); | ||
|
|
||
|
|
@@ -25,8 +25,8 @@ function markOpenReportEnd(report: OnyxTypes.Report, options: MarkOpenReportEndO | |
| const attributes: SpanAttributes = { | ||
| [CONST.TELEMETRY.ATTRIBUTE_IS_TRANSACTION_THREAD]: isTransactionThread, | ||
| [CONST.TELEMETRY.ATTRIBUTE_IS_ONE_TRANSACTION_REPORT]: isOneTransactionThread, | ||
| [CONST.TELEMETRY.ATTRIBUTE_REPORT_TYPE]: type, | ||
| [CONST.TELEMETRY.ATTRIBUTE_CHAT_TYPE]: chatType, | ||
| [CONST.TELEMETRY.ATTRIBUTE_REPORT_TYPE]: report?.type, | ||
| [CONST.TELEMETRY.ATTRIBUTE_CHAT_TYPE]: report?.chatType, | ||
| }; | ||
|
|
||
| if (options.warm !== undefined) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,6 @@ import MoneyRequestReportActionsList from '@components/MoneyRequestReportView/Mo | |
| import NavigationDeferredMount from '@components/NavigationDeferredMount'; | ||
|
|
||
| import {useIsAppLoadPending, useIsReportLoadPending} from '@hooks/useInFlightRequests'; | ||
| import useMarkOpenReportEndOnSkeleton from '@hooks/useMarkOpenReportEndOnSkeleton'; | ||
| import useNetwork from '@hooks/useNetwork'; | ||
| import useOnyx from '@hooks/useOnyx'; | ||
| import usePaginatedReportActions from '@hooks/usePaginatedReportActions'; | ||
|
|
@@ -62,16 +61,13 @@ function ReportActions() { | |
| // The app-load skeleton is hoisted out of the body so the body's data hooks/effects never run | ||
| // during app boot. It only applies on the chat path (after the skeleton and money-request | ||
| // branches below) — matching the previous behavior, where this skeleton lived inside the | ||
| // chat-only ReportActionsView. Because the body won't mount for this branch, it can't close the | ||
| // open-report span itself, so we close it here for the branch we gate. | ||
| // chat-only ReportActionsView. | ||
| // | ||
| // Concierge is excluded so the body still mounts under the app-load skeleton, seeding sessionStartTime | ||
| // before content appeared. | ||
| const isConciergeMainDM = isConciergeChatReport(report, conciergeReportID); | ||
| const shouldShowAppLoadSkeleton = isAppLoadPending && !isOffline && !!report && !shouldWaitForTransactions && !shouldDisplayMoneyRequestActionsList && !isConciergeMainDM; | ||
|
|
||
| useMarkOpenReportEndOnSkeleton(report, shouldShowAppLoadSkeleton); | ||
|
|
||
| if (!report || shouldWaitForTransactions) { | ||
| return ( | ||
| <ReportActionsLoadingSkeleton | ||
|
|
@@ -122,9 +118,11 @@ function ReportActionsWithInboxTabDeferredMount({reportID, shouldDefer}: ReportA | |
| <NavigationDeferredMount | ||
| waitForUpcomingTransition={false} | ||
| placeholder={ | ||
| // Deferral, not a data wait — closing the span here would time the defer and tag a cached report cold. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ CONSISTENCY-16 (docs)Comments should read as plain sentences without em dashes. This comment uses an em dash to join two clauses. Replace the em dash with a period and start a new sentence: // Deferral, not a data wait. Closing the span here would time the defer and tag a cached report cold.Reviewed at: f281e4a | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency. |
||
| <ReportActionsLoadingSkeleton | ||
| reportID={reportID} | ||
| skeletonName={CONST.TELEMETRY.CANCELED_BY_SKELETON.INBOX_TAB_DEFER} | ||
| shouldMarkOpenReportEnd={false} | ||
| /> | ||
| } | ||
| > | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ import ReportActionsSkeletonView from '@components/ReportActionsSkeletonView'; | |
|
|
||
| import useCancelSendMessageSpanOnSkeleton from '@hooks/useCancelSendMessageSpanOnSkeleton'; | ||
| import type {SkeletonName} from '@hooks/useCancelSendMessageSpanOnSkeleton'; | ||
| import useMarkOpenReportEndOnSkeleton from '@hooks/useMarkOpenReportEndOnSkeleton'; | ||
|
|
||
| import React from 'react'; | ||
|
|
||
|
|
@@ -14,14 +15,19 @@ type ReportActionsLoadingSkeletonProps = { | |
|
|
||
| /** Whether the skeleton rows animate */ | ||
| shouldAnimate?: boolean; | ||
|
|
||
| /** Whether this skeleton closes the open-report span as a cold open. Off when it isn't waiting on report data */ | ||
| shouldMarkOpenReportEnd?: boolean; | ||
| }; | ||
|
|
||
| /** | ||
| * Report-actions loading skeleton. Mounted only while the skeleton shows, so it hosts the hook that cancels | ||
| * the otherwise never-ending send-message span (tagged with `skeletonName`). | ||
| * Report-actions loading skeleton. Hosts the skeleton-phase span marks — cancelling the never-ending | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ CONSISTENCY-16 (docs)Comments should read as plain sentences without em dashes. This JSDoc description uses two em dashes to bracket a clause. Rewrite without em dashes, for example: * Report-actions loading skeleton. Hosts the skeleton-phase span marks: cancelling the never-ending
* send-message span, and closing the open-report span as cold. Mounted here means visible, while a
* parent's copy of the skeleton condition can drift from what actually renders.Reviewed at: f281e4a | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency. |
||
| * send-message span, and closing the open-report span as cold — because mounted here means visible, while a | ||
| * parent's copy of the skeleton condition can drift from what actually renders. | ||
| */ | ||
| function ReportActionsLoadingSkeleton({reportID, skeletonName, shouldAnimate = true}: ReportActionsLoadingSkeletonProps) { | ||
| function ReportActionsLoadingSkeleton({reportID, skeletonName, shouldAnimate = true, shouldMarkOpenReportEnd = true}: ReportActionsLoadingSkeletonProps) { | ||
| useCancelSendMessageSpanOnSkeleton(reportID, skeletonName); | ||
| useMarkOpenReportEndOnSkeleton(reportID, shouldMarkOpenReportEnd); | ||
| return <ReportActionsSkeletonView shouldAnimate={shouldAnimate} />; | ||
| } | ||
|
|
||
|
|
||
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.
❌ CONSISTENCY-16 (docs)
Comments should read as plain sentences without em dashes. This JSDoc description line uses an em dash to join two clauses.
Replace the em dash with a period and start a new sentence:
Reviewed at: f281e4a | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.