-
Notifications
You must be signed in to change notification settings - Fork 4.1k
chore: refactor faro (CE) #40350
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: refactor faro (CE) #40350
Conversation
WalkthroughThis change refactors the application's telemetry and error reporting system by centralizing all error capturing and user identification through a new Changes
Sequence Diagram(s)sequenceDiagram
participant AppCode as Application Code
participant Telemetry as AppsmithTelemetry
participant Faro as Faro (Telemetry Backend)
AppCode->>Telemetry: captureException(error, context)
Telemetry->>Faro: pushError(error, context)
Faro-->>Telemetry: eventId
Telemetry-->>AppCode: eventId
AppCode->>Telemetry: identifyUser(userId, userData)
Telemetry->>Faro: setUser(userId, userData)
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
app/client/src/utils/getPathAndValueFromActionDiffObject.ts (1)
39-49
:⚠️ Potential issueCritical Bug: Wrong variable used in path reducer
Inside thereduce
callback you reference the outerpath
variable instead of the accumulator (acc
), leading to incorrect or repeated segments.Apply this diff to correctly build the path from the accumulator:
- path = actionObjectDiff[i].path.reduce( - (acc: string, item: number | string) => { - try { - if (typeof item === "string" && acc) { - acc += `${path}.${item}`; - } else if (typeof item === "string" && !acc) { - acc += `${item}`; - } else acc += `${path}[${item}]`; - - return acc; - } catch (error) { - appsmithTelemetry.captureException( - { - message: `Adding key: where failed, cannot create path`, - oldData: actionObjectDiff, - }, - { errorName: "ActionDiffPathError" }, - ); - } - }, - "", - ); + path = actionObjectDiff[i].path.reduce( + (acc: string, item: number | string) => { + try { + if (typeof item === "string" && acc) { + return `${acc}.${item}`; + } else if (typeof item === "string") { + return `${item}`; + } + return `${acc}[${item}]`; + } catch (error) { + appsmithTelemetry.captureException( + { + message: `Adding key: where failed, cannot create path`, + oldData: actionObjectDiff, + }, + { errorName: "ActionDiffPathError" }, + ); + return acc; + } + }, + "", + );app/client/src/utils/helpers.test.ts (1)
548-559
:⚠️ Potential issueFix test failures related to trace context.
There's a pipeline failure related to destructuring the
context
property fromappsmithTelemetry.getTraceAndContext()
. The mock implementation needs to be expanded to handle this method call.(appsmithTelemetry.captureException as jest.Mock).mockImplementation( mockCaptureException, ); +// Add mock implementation for getTraceAndContext +(appsmithTelemetry.getTraceAndContext as jest.Mock).mockReturnValue({ + context: {}, + trace: {} +});
🧹 Nitpick comments (4)
app/client/src/widgets/MapChartWidget/component/utilities.ts (1)
76-81
: Improve Abort Handling in Error Callback
Using numericerror.code
to detect aborted fetches is brittle. Prefer checkingerror.name === "AbortError"
orerror.name === "DOMException"
for clarity and future-proofing.- if (error.code !== 20) { - log.error({ error }); - appsmithTelemetry.captureException(error, { - errorName: "MapChartWidget_utilities", - }); - } + if (error.name !== "AbortError") { + log.error({ error }); + appsmithTelemetry.captureException(error, { + errorName: "MapChartWidget_utilities", + }); + }app/client/src/pages/Editor/Canvas.tsx (1)
123-124
: Enhance Telemetry Context
Capturing exceptions is good; consider enriching the payload (e.g., widget count or canvas dimensions) to simplify post-mortem debugging.app/client/src/api/helpers/validateJsonResponseMeta.ts (1)
10-16
: Enhance API Error Context
Logging the missingresponseMeta
is essential—consider adding HTTP status code and request URL to thecontexts
object for richer diagnostics.app/client/src/instrumentation/index.ts (1)
108-114
: Annotate return type ofgetInstance
for better type safetyWithout an explicit return type, TypeScript infers
AppsmithTelemetry | null
, forcing non-null assertions elsewhere. Declaring it removes that friction:- public static getInstance() { + public static getInstance(): AppsmithTelemetry {Minor, but it improves DX in all call sites.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
app/client/src/AppErrorBoundry.tsx
(2 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/FormRender.tsx
(2 hunks)app/client/src/actions/pageActions.tsx
(2 hunks)app/client/src/api/helpers/validateJsonResponseMeta.ts
(2 hunks)app/client/src/api/interceptors/response/failureHandlers/handleMissingResponseMeta.ts
(1 hunks)app/client/src/api/interceptors/response/failureHandlers/handleNotFoundError.ts
(2 hunks)app/client/src/api/interceptors/response/failureHandlers/handleUnauthorizedError.ts
(2 hunks)app/client/src/ce/sagas/PageSagas.tsx
(2 hunks)app/client/src/ce/utils/AnalyticsUtil.tsx
(2 hunks)app/client/src/components/editorComponents/CodeEditor/EvaluatedValuePopup.tsx
(2 hunks)app/client/src/components/editorComponents/ErrorBoundry.tsx
(2 hunks)app/client/src/components/propertyControls/TabControl.tsx
(2 hunks)app/client/src/ee/sagas/index.tsx
(2 hunks)app/client/src/git/sagas/helpers/handleApiErrors.ts
(2 hunks)app/client/src/index.tsx
(1 hunks)app/client/src/instrumentation/PageLoadInstrumentation.ts
(0 hunks)app/client/src/instrumentation/generateTraces.ts
(1 hunks)app/client/src/instrumentation/index.ts
(4 hunks)app/client/src/instrumentation/sendFaroErrors.ts
(0 hunks)app/client/src/pages/Editor/Canvas.tsx
(2 hunks)app/client/src/pages/UserAuth/Login.tsx
(2 hunks)app/client/src/pages/UserAuth/SignUp.tsx
(2 hunks)app/client/src/pages/UserAuth/VerifyUser.tsx
(2 hunks)app/client/src/pages/UserAuth/helpers.ts
(2 hunks)app/client/src/reducers/evaluationReducers/treeReducer.ts
(2 hunks)app/client/src/sagas/ActionExecution/PluginActionSaga.ts
(2 hunks)app/client/src/sagas/AppThemingSaga.tsx
(2 hunks)app/client/src/sagas/ErrorSagas.tsx
(2 hunks)app/client/src/sagas/EvalErrorHandler.ts
(5 hunks)app/client/src/sagas/EvaluationsSaga.ts
(3 hunks)app/client/src/sagas/FormEvaluationSaga.ts
(2 hunks)app/client/src/sagas/InitSagas.ts
(4 hunks)app/client/src/sagas/ReplaySaga.ts
(4 hunks)app/client/src/sagas/WidgetLoadingSaga.ts
(2 hunks)app/client/src/sagas/layoutConversionSagas.ts
(2 hunks)app/client/src/utils/Analytics/sentry.ts
(0 hunks)app/client/src/utils/getPathAndValueFromActionDiffObject.ts
(2 hunks)app/client/src/utils/helpers.test.ts
(2 hunks)app/client/src/utils/helpers.tsx
(2 hunks)app/client/src/widgets/CurrencyInputWidget/widget/index.tsx
(4 hunks)app/client/src/widgets/JSONFormWidget/fields/CurrencyInputField.tsx
(2 hunks)app/client/src/widgets/JSONFormWidget/fields/useRegisterFieldValidity.ts
(2 hunks)app/client/src/widgets/MapChartWidget/component/utilities.ts
(2 hunks)app/client/src/widgets/PhoneInputWidget/widget/index.tsx
(2 hunks)app/client/src/widgets/TableWidgetV2/component/cellComponents/InlineCellEditor.tsx
(2 hunks)app/client/src/widgets/TableWidgetV2/component/cellComponents/PlainTextCell.tsx
(2 hunks)app/client/src/widgets/TabsMigrator/widget/index.tsx
(2 hunks)app/client/src/widgets/wds/WDSCurrencyInputWidget/widget/index.tsx
(4 hunks)app/client/src/widgets/wds/WDSPhoneInputWidget/widget/index.tsx
(2 hunks)app/client/src/workers/Evaluation/errorModifier.ts
(2 hunks)
💤 Files with no reviewable changes (3)
- app/client/src/instrumentation/sendFaroErrors.ts
- app/client/src/utils/Analytics/sentry.ts
- app/client/src/instrumentation/PageLoadInstrumentation.ts
🧰 Additional context used
🧬 Code Graph Analysis (45)
app/client/src/pages/UserAuth/SignUp.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/pages/UserAuth/VerifyUser.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/instrumentation/generateTraces.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/components/propertyControls/TabControl.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/api/interceptors/response/failureHandlers/handleNotFoundError.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/sagas/FormEvaluationSaga.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/api/helpers/validateJsonResponseMeta.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/FormRender.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/components/editorComponents/CodeEditor/EvaluatedValuePopup.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/pages/Editor/Canvas.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/widgets/MapChartWidget/component/utilities.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/ce/utils/AnalyticsUtil.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/git/sagas/helpers/handleApiErrors.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/components/editorComponents/ErrorBoundry.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/reducers/evaluationReducers/treeReducer.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/sagas/layoutConversionSagas.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/widgets/JSONFormWidget/fields/CurrencyInputField.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/utils/getPathAndValueFromActionDiffObject.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/widgets/PhoneInputWidget/widget/index.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/workers/Evaluation/errorModifier.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/sagas/ActionExecution/PluginActionSaga.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/widgets/TableWidgetV2/component/cellComponents/InlineCellEditor.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/widgets/TableWidgetV2/component/cellComponents/PlainTextCell.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/api/interceptors/response/failureHandlers/handleMissingResponseMeta.ts (2)
app/client/src/api/types.ts (1)
ApiResponse
(14-18)app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/actions/pageActions.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/widgets/wds/WDSCurrencyInputWidget/widget/index.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/widgets/wds/WDSPhoneInputWidget/widget/index.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/pages/UserAuth/helpers.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/sagas/EvaluationsSaga.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/widgets/JSONFormWidget/fields/useRegisterFieldValidity.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/widgets/CurrencyInputWidget/widget/index.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/pages/UserAuth/Login.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/api/interceptors/response/failureHandlers/handleUnauthorizedError.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/sagas/InitSagas.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/AppErrorBoundry.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/utils/helpers.test.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/sagas/AppThemingSaga.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/widgets/TabsMigrator/widget/index.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/sagas/WidgetLoadingSaga.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/ce/sagas/PageSagas.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/ee/sagas/index.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/sagas/ErrorSagas.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/sagas/ReplaySaga.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/utils/helpers.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
app/client/src/sagas/EvalErrorHandler.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry
(162-162)
🪛 GitHub Actions: Quality checks
app/client/src/utils/helpers.test.ts
[error] 12-12: Test suite failed to run due to TypeError: Cannot destructure property 'context' of undefined returned by appsmithTelemetry.getTraceAndContext().
🔇 Additional comments (83)
app/client/src/widgets/JSONFormWidget/fields/useRegisterFieldValidity.ts (1)
10-10
: Telemetry import and usage has been updated to the new telemetry API.The change migrates from using a direct import of
captureException
to utilizing the newappsmithTelemetry
singleton. This aligns with the broader refactoring effort to centralize error reporting through theAppsmithTelemetry
class.Also applies to: 56-56
app/client/src/components/propertyControls/TabControl.tsx (1)
13-13
: Error reporting refactored to use the centralized telemetry API.The code now properly uses the
appsmithTelemetry
singleton for exception capture instead of the previous direct import. The telemetry context remains unchanged, maintaining proper error tracking for tab migration failures.Also applies to: 134-135
app/client/src/ce/utils/AnalyticsUtil.tsx (1)
12-12
: User identification migrated to the centralized telemetry API.The implementation now correctly uses the
appsmithTelemetry
singleton for user identification instead of the previousFaroUtil
. This change maintains the same functionality while centralizing telemetry operations.Also applies to: 97-97
app/client/src/index.tsx (1)
35-41
: Simplified instrumentation import by removing conditional check.The instrumentation module is now always imported asynchronously rather than conditionally checking if tracing is enabled. This change aligns with the centralized telemetry approach where conditions are handled internally by the telemetry class.
app/client/src/utils/getPathAndValueFromActionDiffObject.ts (2)
1-1
: Centralized Telemetry Import
ImportingappsmithTelemetry
from the shared instrumentation entrypoint aligns with the new singleton pattern and replaces the old standalone error utility.
50-56
: Consistent Exception Capture
Switching toappsmithTelemetry.captureException
maintains existing metadata (errorName
) and centralizes error reporting.app/client/src/widgets/MapChartWidget/component/utilities.ts (1)
6-6
: Centralized Telemetry Import
ImportingappsmithTelemetry
from the core instrumentation module standardizes how widgets log exceptions.app/client/src/instrumentation/generateTraces.ts (1)
8-12
: Centralize Trace Context Retrieval
Replacing the standalonegetTraceAndContext
import withappsmithTelemetry.getTraceAndContext()
consolidates context and tracer management in the singleton, ensuring consistent OpenTelemetry usage.app/client/src/pages/Editor/Canvas.tsx (1)
22-22
: Unified Telemetry Import
Switching toappsmithTelemetry
here aligns Canvas error reporting with the centralized telemetry strategy.app/client/src/api/helpers/validateJsonResponseMeta.ts (1)
1-1
: Standardize Telemetry Import
ImportingappsmithTelemetry
here ensures all API validation errors flow through the new telemetry interface.app/client/src/api/interceptors/response/failureHandlers/handleUnauthorizedError.ts (2)
7-7
: Correct telemetry import update
ImportingappsmithTelemetry
from"instrumentation"
aligns with the centralized telemetry refactor and replaces the old standalone utility.
23-25
: Use ofappsmithTelemetry.captureException
The error reporting call now correctly routes through the singleton instance with the same metadata shape.app/client/src/components/editorComponents/ErrorBoundry.tsx (2)
4-4
: Updated error capture import
Switching toappsmithTelemetry
ensures consistency with the new telemetry singleton across the codebase.
42-42
: Centralized exception capture
UsingappsmithTelemetry.captureException
insidecomponentDidCatch
properly records boundary errors with the designated errorName.app/client/src/components/editorComponents/CodeEditor/EvaluatedValuePopup.tsx (2)
31-31
: Importing telemetry singleton
The named import ofappsmithTelemetry
replaces the removedsendFaroErrors
utility, centralizing all capture calls.
293-299
: Prepared statement error handling
Capturing a missing-value error viaappsmithTelemetry.captureException
with contextual props is correct and preserves the metadata.app/client/src/pages/UserAuth/helpers.ts (2)
8-8
: Switched to telemetry singleton import
Replacing the oldcaptureException
import aligns error reporting to the newAppsmithTelemetry
class.
98-100
: Exception report in resend hook
Logging the missing-email scenario withappsmithTelemetry.captureException
and errorName"EmailVerificationRetryError"
is consistent and effective.app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/FormRender.tsx (2)
12-12
: Telemetry import consolidation
ImportingappsmithTelemetry
from"instrumentation"
replaces the legacy error sender and centralizes error tracking.
106-106
: Form render error capture
Switching the catch block toappsmithTelemetry.captureException
with"FormRenderError"
maintains error visibility in the new system.app/client/src/ee/sagas/index.tsx (1)
5-5
: Telemetry import and usage update looks good.The refactoring to use the singleton
appsmithTelemetry
instance instead of directly importingcaptureException
is consistent with modern error handling patterns.Also applies to: 25-27
app/client/src/sagas/ActionExecution/PluginActionSaga.ts (1)
172-172
: Error reporting refactoring looks good.The updated import and method call maintains the same functionality while centralizing telemetry through the singleton pattern. The error context parameters remain unchanged.
Also applies to: 992-1004
app/client/src/pages/UserAuth/SignUp.tsx (1)
67-67
: Properly migrated to the centralized telemetry system.The error reporting for signup errors has been correctly updated to use the singleton instance, maintaining the same metadata structure.
Also applies to: 148-148
app/client/src/api/interceptors/response/failureHandlers/handleNotFoundError.ts (1)
9-9
: Consistent update to error reporting mechanism.The API interceptor now uses the centralized telemetry instance while maintaining the same error capturing behavior with appropriate error naming.
Also applies to: 23-23
app/client/src/reducers/evaluationReducers/treeReducer.ts (2)
6-6
: Import updated for new telemetry systemThe import has been updated to use the new centralized telemetry system through the
appsmithTelemetry
singleton instance.
35-41
: Error capture method updated to use new telemetry systemThe error handling has been updated to use the new telemetry system's method while maintaining the same error metadata structure.
app/client/src/utils/helpers.tsx (2)
47-47
: Import updated for new telemetry systemThe import has been updated to use the new centralized telemetry system through the
appsmithTelemetry
singleton instance.
947-952
: Error capture method updated to use new telemetry systemThe error handling in
captureInvalidDynamicBindingPath
function has been updated to use the new telemetry system's method while maintaining the same error metadata structure.app/client/src/widgets/TableWidgetV2/component/cellComponents/PlainTextCell.tsx (2)
23-23
: Import updated for new telemetry systemThe import has been updated to use the new centralized telemetry system through the
appsmithTelemetry
singleton instance.
230-232
: Error capture method updated to use new telemetry systemThe error handling in the currency formatting logic has been updated to use the new telemetry system's method while maintaining the same error metadata structure.
app/client/src/git/sagas/helpers/handleApiErrors.ts (2)
1-1
: Import updated for new telemetry systemThe import has been updated to use the new centralized telemetry system through the
appsmithTelemetry
singleton instance.
25-25
: Error capture method updated to use new telemetry systemThe error handling in
handleApiErrors
function has been updated to use the new telemetry system's method while maintaining the same error metadata structure.app/client/src/pages/UserAuth/VerifyUser.tsx (1)
9-9
: Approved: Telemetry refactoring implementation looks goodThe change to use the centralized
appsmithTelemetry
singleton follows the new pattern for error reporting across the application.Also applies to: 27-32
app/client/src/widgets/JSONFormWidget/fields/CurrencyInputField.tsx (1)
17-17
: Approved: Good implementation of the telemetry refactoringThe change correctly implements the migration from the standalone
captureException
function to the new centralizedappsmithTelemetry.captureException
method while preserving the same error reporting metadata.Also applies to: 135-137
app/client/src/sagas/FormEvaluationSaga.ts (1)
14-14
: Approved: Telemetry refactoring implemented consistentlyThe change correctly updates the import and usage of the error capturing utility to use the new centralized telemetry system.
Also applies to: 375-377
app/client/src/sagas/WidgetLoadingSaga.ts (1)
19-19
: Approved: Consistent implementation of telemetry refactoringThe change follows the same pattern as other files, correctly transitioning to the new centralized telemetry system while maintaining the same error metadata.
Also applies to: 104-106
app/client/src/sagas/layoutConversionSagas.ts (2)
29-29
: Import updated to use the centralized telemetry singleton.The import has been changed to use
appsmithTelemetry
from the central "instrumentation" module rather than directly importing the standalonecaptureException
function.
217-219
: Updated error reporting to use the centralized telemetry singleton.The error capturing mechanism has been updated to use the method from the
appsmithTelemetry
singleton class while maintaining the same error metadata.app/client/src/widgets/PhoneInputWidget/widget/index.tsx (2)
41-41
: Import updated to use the centralized telemetry singleton.The import has been changed to use
appsmithTelemetry
from the central "instrumentation" module rather than directly importing the standalonecaptureException
function.
351-353
: Updated error reporting to use the centralized telemetry singleton.The error capturing mechanism has been updated to use the method from the
appsmithTelemetry
singleton class while maintaining the same error metadata.app/client/src/widgets/TabsMigrator/widget/index.tsx (3)
15-15
: Import updated to use the centralized telemetry singleton.The import has been changed to use
appsmithTelemetry
from the central "instrumentation" module rather than directly importing the standalonecaptureException
function.
23-27
: Updated error reporting to use the centralized telemetry singleton.The error capturing mechanism has been updated to use the method from the
appsmithTelemetry
singleton class while maintaining the same error metadata.
120-124
: Updated error reporting to use the centralized telemetry singleton.The error capturing mechanism has been updated to use the method from the
appsmithTelemetry
singleton class while maintaining the same error metadata.app/client/src/sagas/ReplaySaga.ts (4)
11-11
: Import updated to use the centralized telemetry singleton.The import has been changed to use
appsmithTelemetry
from the central "instrumentation" module rather than directly importing the standalonecaptureException
function.
136-138
: Updated error reporting to use the centralized telemetry singleton.The error capturing mechanism has been updated to use the method from the
appsmithTelemetry
singleton class while maintaining the same error metadata.
170-170
: Updated error reporting to use the centralized telemetry singleton.The error capturing mechanism has been updated to use the method from the
appsmithTelemetry
singleton class while maintaining the same error metadata.
265-265
: Updated error reporting to use the centralized telemetry singleton.The error capturing mechanism has been updated to use the method from the
appsmithTelemetry
singleton class while maintaining the same error metadata.app/client/src/sagas/EvaluationsSaga.ts (3)
126-126
: Update import to use centralized telemetry singletonThe import has been updated to use the
appsmithTelemetry
singleton from the central instrumentation module instead of directly importing thecaptureException
function.
938-938
: Error reporting refactored to use telemetry singletonUpdated error reporting to use the new centralized telemetry instance. This is aligned with the broader refactoring to standardize error reporting across the codebase.
1009-1009
: Error reporting refactored to use telemetry singletonSimilarly updated error reporting in the infinite loop catch block to use the centralized telemetry instance.
app/client/src/widgets/TableWidgetV2/component/cellComponents/InlineCellEditor.tsx (2)
23-23
: Update import to use centralized telemetry singletonUpdated the import to use the
appsmithTelemetry
singleton from the central instrumentation module instead of directly importing thecaptureException
function.
240-242
: Updated error reporting to use telemetry singletonThe error capturing mechanism has been updated to use the centralized telemetry instance, maintaining the same error name and metadata structure as before.
app/client/src/pages/UserAuth/Login.tsx (2)
55-55
: Update import to use centralized telemetry singletonUpdated the import to use the
appsmithTelemetry
singleton from the central instrumentation module instead of directly importing thecaptureException
function.
119-121
: Updated error reporting to use telemetry singletonThe error capturing mechanism for login errors has been updated to use the centralized telemetry instance, maintaining the same error wrapping and metadata structure as before.
app/client/src/workers/Evaluation/errorModifier.ts (2)
16-16
: Update import to use centralized telemetry singletonUpdated the import to use the
appsmithTelemetry
singleton from the central instrumentation module instead of directly importing thecaptureException
function.
227-229
: Updated error reporting to use telemetry singletonThe error capturing mechanism for JSON stringify errors has been updated to use the centralized telemetry instance, maintaining the same error metadata structure as before.
app/client/src/actions/pageActions.tsx (2)
32-32
: Updated telemetry import to use new singleton pattern.The import is correctly changed to use the centralized
appsmithTelemetry
singleton from "instrumentation" instead of the direct import from "instrumentation/sendFaroErrors".
326-331
: Error reporting updated to use telemetry singleton.The error capture call is correctly updated to use the new telemetry interface while maintaining the same error parameters.
app/client/src/ce/sagas/PageSagas.tsx (2)
154-154
: Updated telemetry import to use new singleton pattern.The import has been correctly updated to use the centralized telemetry singleton approach.
578-583
: Error reporting updated to use telemetry singleton.The error capture implementation is correctly updated to use the new telemetry interface with the same error parameters and metadata.
app/client/src/utils/helpers.test.ts (2)
19-21
: Updated test imports and mocks for new telemetry interface.The imports and mock setup have been correctly updated to work with the new telemetry singleton implementation.
548-549
: Updated mock implementation for telemetry.The mock implementation is correctly updated to work with the new telemetry interface.
app/client/src/api/interceptors/response/failureHandlers/handleMissingResponseMeta.ts (2)
3-3
: Updated telemetry import to use new singleton pattern.The import statement is correctly updated to use the centralized
appsmithTelemetry
singleton.
9-15
: Updated error capture to use telemetry singleton.The error reporting is correctly updated to use the new telemetry interface while maintaining the same error message and context information.
app/client/src/widgets/wds/WDSPhoneInputWidget/widget/index.tsx (1)
23-23
: Telemetry refactor is correctly implemented.The change to use
appsmithTelemetry.captureException
from the centralized telemetry singleton is consistent with the broader refactoring effort across the codebase.Also applies to: 166-168
app/client/src/sagas/AppThemingSaga.tsx (1)
47-47
: Telemetry refactor is correctly implemented.The change to use
appsmithTelemetry.captureException
from the centralized telemetry singleton is consistent with the broader refactoring effort across the codebase.Also applies to: 129-142
app/client/src/AppErrorBoundry.tsx (1)
7-7
: Telemetry refactor is correctly implemented.The change to use
appsmithTelemetry.captureException
from the centralized telemetry singleton is consistent with the broader refactoring effort across the codebase.Also applies to: 35-37
app/client/src/widgets/wds/WDSCurrencyInputWidget/widget/index.tsx (4)
32-32
: Import change looks goodThe change from importing a standalone
captureException
function to using theappsmithTelemetry
singleton is part of a broader refactoring effort to centralize error reporting.
192-194
: Error reporting implementation updated correctlyThe error capturing has been properly migrated to use the singleton instance method while maintaining the same error metadata.
253-255
: Error reporting implementation updated correctlyThe error capturing has been properly migrated to use the singleton instance method while maintaining the same error metadata.
318-320
: Error reporting implementation updated correctlyThe error capturing has been properly migrated to use the singleton instance method while maintaining the same error metadata.
app/client/src/widgets/CurrencyInputWidget/widget/index.tsx (4)
46-46
: Import change looks goodThe change from importing a standalone
captureException
function to using theappsmithTelemetry
singleton is part of a broader refactoring effort to centralize error reporting.
502-504
: Error reporting implementation updated correctlyThe error capturing has been properly migrated to use the singleton instance method while maintaining the same error metadata.
522-524
: Error reporting implementation updated correctlyThe error capturing has been properly migrated to use the singleton instance method while maintaining the same error metadata.
582-584
: Error reporting implementation updated correctlyThe error capturing has been properly migrated to use the singleton instance method while maintaining the same error metadata.
app/client/src/sagas/InitSagas.ts (4)
95-95
: Import change looks goodThe change from importing a standalone
captureException
function to using theappsmithTelemetry
singleton is part of a broader refactoring effort to centralize error reporting.
281-287
: Error reporting implementation updated correctlyThe error capturing in the
getInitResponses
saga has been properly migrated to use the singleton instance method while maintaining the same error metadata.
377-377
: Error reporting implementation updated correctlyThe error capturing in the
startAppEngine
saga has been properly migrated to use the singleton instance method while maintaining the same error metadata.
474-476
: Error reporting implementation updated correctlyThe error capturing in the
eagerPageInitSaga
has been properly migrated to use the singleton instance method while maintaining the same error metadata.app/client/src/sagas/EvalErrorHandler.ts (4)
20-20
: Import change looks goodThe change from importing a standalone
captureException
function to using theappsmithTelemetry
singleton is part of a broader refactoring effort to centralize error reporting.
238-251
: Error reporting implementation updated correctlyThe error capturing for cyclical dependency errors has been properly migrated to use the singleton instance method while maintaining all the same error metadata.
271-274
: Error reporting implementation updated correctlyThe error capturing for eval tree errors has been properly migrated to use the singleton instance method.
278-281
: All error reporting implementations updated correctlyAll remaining error capture calls in this file have been properly migrated to use the singleton instance method while maintaining the same error metadata for each error type.
Also applies to: 284-287, 292-298, 308-312, 315-319, 323-326, 334-335, 339-342
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.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/client/src/instrumentation/index.ts (3)
77-93
: AddtracerProvider.register()
to activate tracing.The
WebTracerProvider
is instantiated and processors are added, but never registered. WithouttracerProvider.register()
, no global provider is set and both auto-instrumentation and manual spans may be dropped.tracerProvider.addSpanProcessor( new FaroSessionSpanProcessor( new BatchSpanProcessor(new FaroTraceExporter({ ...this.faro })), this.faro.metas, ), ); + + // Activate the provider so OpenTelemetry uses it + tracerProvider.register(); this.faro.api.initOTEL(trace, context);
116-127
: Return object shape is inconsistent – may break callers.
getTraceAndContext()
sometimes returns{ trace, context, pushError: () => {} }
and sometimes only{ trace, context }
. This inconsistency can lead to runtime errors for consumers that expectpushError
to always be present.if (!otel || !this.faro) { return { trace, context, pushError: () => {} }; } return { trace: otel.trace, context: otel.context, + pushError: this.faro.api.pushError.bind(this.faro.api), };
129-131
: Guard againsthint
beingundefined
to avoid runtime crashes.Provide a default empty object for the
hint
parameter to ensure thefor (const key in hint)
loop on line 142 won't throw TypeError whenhint
is undefined.// eslint-disable-next-line @typescript-eslint/no-explicit-any - public captureException(exception: any, hint?: Record<string, any>): string { + public captureException(exception: any, hint: Record<string, any> = {}): string {
🧹 Nitpick comments (6)
app/client/src/instrumentation/index.ts (6)
35-96
: Make the constructor private for proper singleton pattern implementation.The constructor should be marked as private to prevent direct instantiation with
new AppsmithTelemetry()
. This enforces usage through the singleton getInstance() method.- constructor() { + private constructor() {
171-172
: Fix typo in comment.- //add name inside cotext + //add name inside context
129-131
: Consider using a more specific type for the exception parameter.Using
any
for the exception parameter reduces type safety. Consider using a more specific type or a union of likely exception types.// eslint-disable-next-line @typescript-eslint/no-explicit-any - public captureException(exception: any, hint?: Record<string, any>): string { + public captureException(exception: Error | unknown, hint?: Record<string, any>): string {
141-149
: Consider using Object.entries for cleaner iteration over hint properties.The current for-in loop with conditional type checking can be refactored using Object.entries for more modern, cleaner code.
if (hint) { - for (const key in hint) { - if (typeof hint[key] === "string") { - context[key] = hint[key]; - } else { - context[key] = JSON.stringify(hint[key]); - } - } + Object.entries(hint).forEach(([key, value]) => { + context[key] = typeof value === "string" ? value : JSON.stringify(value); + }); }
151-158
: Consider using a more specific exception type for error logging.When catching errors from
this.faro.api.pushError()
, consider using a more specific type for better error handling clarity.try { this.faro.api.pushError( exception instanceof Error ? exception : new Error(String(exception)), { type: "error", context: context }, ); - } catch (error) { + } catch (error: unknown) { errorLogger(error); }
178-180
: Use more specific error type in catch block.Similar to the previous comment, use a more specific type for the caught error.
- } catch (e) { + } catch (e: unknown) { errorLogger(e); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/src/instrumentation/index.ts
(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Number
or
Fixes
Issue URL
Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/14664226456
Commit: caaee4b
Cypress dashboard.
Tags:
@tag.All
Spec:
Fri, 25 Apr 2025 13:20:56 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Refactor
Tests