-
Notifications
You must be signed in to change notification settings - Fork 4.6k
chore: Add newrelic tracking spans to AppViewer engine and AppEditor engine #34716
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
Changes from 2 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 |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ import URLGeneratorFactory from "entities/URLRedirect/factory"; | |
| import { updateBranchLocally } from "actions/gitSyncActions"; | ||
| import { getCurrentGitBranch } from "selectors/gitSyncSelectors"; | ||
| import { restoreIDEEditorViewMode } from "actions/ideActions"; | ||
| import type { Span } from "@opentelemetry/api"; | ||
| import { endSpan, startNestedSpan } from "UITelemetry/generateTraces"; | ||
|
|
||
| export interface AppEnginePayload { | ||
| applicationId?: string; | ||
|
|
@@ -29,10 +31,14 @@ export interface AppEnginePayload { | |
| } | ||
|
|
||
| export interface IAppEngine { | ||
| setupEngine(payload: AppEnginePayload): any; | ||
| loadAppData(payload: AppEnginePayload): any; | ||
| setupEngine(payload: AppEnginePayload, rootSpan: Span): any; | ||
| loadAppData(payload: AppEnginePayload, rootSpan: Span): any; | ||
| loadAppURL(pageId: string, pageIdInUrl?: string): any; | ||
| loadAppEntities(toLoadPageId: string, applicationId: string): any; | ||
| loadAppEntities( | ||
| toLoadPageId: string, | ||
| applicationId: string, | ||
| rootSpan: Span, | ||
| ): any; | ||
| loadGit(applicationId: string): any; | ||
| completeChore(): any; | ||
| } | ||
|
|
@@ -55,13 +61,19 @@ export default abstract class AppEngine { | |
| toLoadPageId: string, | ||
| applicationId: string, | ||
| allResponses: InitConsolidatedApi, | ||
| rootSpan: Span, | ||
| ): any; | ||
| abstract loadGit(applicationId: string): any; | ||
| abstract loadGit(applicationId: string, rootSpan: Span): any; | ||
| abstract startPerformanceTracking(): any; | ||
| abstract stopPerformanceTracking(): any; | ||
| abstract completeChore(): any; | ||
| abstract completeChore(rootSpan: Span): any; | ||
|
|
||
| *loadAppData(payload: AppEnginePayload, allResponses: InitConsolidatedApi) { | ||
| *loadAppData( | ||
| payload: AppEnginePayload, | ||
| allResponses: InitConsolidatedApi, | ||
| rootSpan: Span, | ||
| ) { | ||
| const loadAppDataSpan = startNestedSpan("AppEngine.loadAppData", rootSpan); | ||
| const { applicationId, branch, pageId } = payload; | ||
| const { pages } = allResponses; | ||
| const apiCalls: boolean = yield failFastApiCalls( | ||
|
|
@@ -82,8 +94,11 @@ export default abstract class AppEngine { | |
| ReduxActionErrorTypes.FETCH_PAGE_LIST_ERROR, | ||
| ], | ||
| ); | ||
| if (!apiCalls) | ||
|
|
||
| if (!apiCalls) { | ||
| throw new PageNotFoundError(`Cannot find page with id: ${pageId}`); | ||
| } | ||
|
|
||
| const application: ApplicationPayload = yield select(getCurrentApplication); | ||
| const currentGitBranch: ReturnType<typeof getCurrentGitBranch> = | ||
| yield select(getCurrentGitBranch); | ||
|
|
@@ -97,25 +112,36 @@ export default abstract class AppEngine { | |
| application.applicationVersion, | ||
| this._mode, | ||
| ); | ||
|
|
||
| endSpan(loadAppDataSpan); | ||
| return { toLoadPageId, applicationId: application.id }; | ||
| } | ||
|
|
||
| *setupEngine(payload: AppEnginePayload): any { | ||
| *setupEngine(payload: AppEnginePayload, rootSpan: Span): any { | ||
| const setupEngineSpan = startNestedSpan("AppEngine.setupEngine", rootSpan); | ||
|
|
||
| const { branch } = payload; | ||
| yield put(updateBranchLocally(branch || "")); | ||
| yield put(setAppMode(this._mode)); | ||
| yield put(restoreIDEEditorViewMode()); | ||
| yield put({ type: ReduxActionTypes.START_EVALUATION }); | ||
|
|
||
| endSpan(setupEngineSpan); | ||
| } | ||
|
|
||
| *loadAppURL(pageId: string, pageIdInUrl?: string) { | ||
| *loadAppURL(pageId: string, pageIdInUrl: string = "", rootSpan: Span) { | ||
| try { | ||
| const loadAppUrlSpan = startNestedSpan("AppEngine.loadAppURL", rootSpan); | ||
|
|
||
| if (!this._urlRedirect) return; | ||
| const newURL: string = yield call( | ||
| this._urlRedirect.generateRedirectURL.bind(this), | ||
| pageId, | ||
| pageIdInUrl, | ||
| ); | ||
|
|
||
| endSpan(loadAppUrlSpan); | ||
|
|
||
|
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. Is this necessary, no blocking or expensive code here?
Author
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. yeah that's true, I just added them to all components.
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. Also i think the start span should be after the return statement in line 144, so that we don't have an incomplete span.
Author
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. loadAppData set the value of But yeah I agree, we should avoid any case where there are spans that don't end. |
||
| if (!newURL) return; | ||
| history.replace(newURL); | ||
| } catch (e) { | ||
|
|
||
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.
Address the default parameter issue.
The default parameter should follow the last required parameter.
Committable suggestion
Tools
Biome