Skip to content

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

Merged
merged 7 commits into from
Apr 29, 2025
Merged

chore: refactor faro (CE) #40350

merged 7 commits into from
Apr 29, 2025

Conversation

dvj1988
Copy link
Contributor

@dvj1988 dvj1988 commented Apr 23, 2025

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?

  • Yes
  • No

Summary by CodeRabbit

  • Refactor

    • Unified all error and telemetry reporting to use a centralized telemetry utility for improved consistency.
    • Replaced legacy error reporting imports and methods with a new singleton telemetry interface across the application.
    • Removed obsolete telemetry and error reporting files and classes.
    • Simplified telemetry initialization by removing conditional tracing checks.
    • No changes to user-facing functionality or workflows.
  • Tests

    • Updated test mocks to use the new telemetry interface.

Copy link
Contributor

coderabbitai bot commented Apr 23, 2025

Walkthrough

This change refactors the application's telemetry and error reporting system by centralizing all error capturing and user identification through a new AppsmithTelemetry singleton class. The previous standalone error capturing utility (captureException from instrumentation/sendFaroErrors) and the FaroUtil class are removed. All error reporting calls across the codebase are updated to use the appsmithTelemetry.captureException method. The telemetry initialization, trace context retrieval, and user identification are now encapsulated within the new class. Additionally, the conditional dynamic import for tracing in the entry point is simplified to always attempt the import. The PageLoadInstrumentation and related legacy utilities are deleted.

Changes

Files/Paths (Grouped) Change Summary
app/client/src/instrumentation/index.ts Introduces AppsmithTelemetry singleton class, encapsulating telemetry setup, error capture, tracing, and user identification. Removes global faro and window augmentation.
app/client/src/instrumentation/sendFaroErrors.ts, app/client/src/utils/Analytics/sentry.ts, app/client/src/instrumentation/PageLoadInstrumentation.ts Deletes legacy error capturing utility, Faro integration class, and page load instrumentation.
app/client/src/instrumentation/generateTraces.ts Refactors to use appsmithTelemetry.getTraceAndContext() instead of standalone function.
Most files in app/client/src/ and subdirectories (e.g., sagas/, widgets/, pages/, reducers/, api/, components/, utils/) Replaces all imports and usages of captureException (from instrumentation/sendFaroErrors) with appsmithTelemetry.captureException from instrumentation. No logic changes.
app/client/src/ce/utils/AnalyticsUtil.tsx Replaces FaroUtil.identifyUser with appsmithTelemetry.identifyUser.
app/client/src/index.tsx Removes conditional check for tracing before importing instrumentation; always attempts import.
app/client/src/utils/helpers.test.ts Updates test mocks and imports to use appsmithTelemetry instead of sendFaroErrors.

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)
Loading

Possibly related PRs

  • appsmithorg/appsmith#40220: Refactors error capturing from Sentry to a standalone captureException function; related as both PRs change error reporting mechanisms.
  • appsmithorg/appsmith#40170: Modifies error handling logic in EvalErrorHandler.ts where this PR also updates telemetry calls.
  • appsmithorg/appsmith#40270: Replaces Sentry with Faro in EvalErrorHandler.ts; both PRs update error reporting in the same file.

Suggested reviewers

  • dvj1988
  • vsvamsi1

Poem

In the land of telemetry, a singleton arose,
No more scattered errors, just one place it goes!
The ghosts of Sentry and FaroUtil fade,
As appsmithTelemetry takes the trade.
With every exception, a UUID in tow,
Unified reporting—onward we go!
🎉


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Apr 23, 2025
@dvj1988 dvj1988 changed the title chore: refactor faro chore: refactor faro (CE) Apr 23, 2025
@dvj1988 dvj1988 added the ok-to-test Required label for CI label Apr 23, 2025
@ApekshaBhosale ApekshaBhosale self-assigned this Apr 23, 2025
@ApekshaBhosale ApekshaBhosale marked this pull request as ready for review April 25, 2025 09:57
@ApekshaBhosale ApekshaBhosale requested review from rahulbarwal and removed request for a team April 25, 2025 09:57
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 issue

Critical Bug: Wrong variable used in path reducer
Inside the reduce callback you reference the outer path 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 issue

Fix test failures related to trace context.

There's a pipeline failure related to destructuring the context property from appsmithTelemetry.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 numeric error.code to detect aborted fetches is brittle. Prefer checking error.name === "AbortError" or error.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 missing responseMeta is essential—consider adding HTTP status code and request URL to the contexts object for richer diagnostics.

app/client/src/instrumentation/index.ts (1)

108-114: Annotate return type of getInstance for better type safety

Without 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

📥 Commits

Reviewing files that changed from the base of the PR and between d7d59c6 and 0cf2b56.

📒 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 new appsmithTelemetry singleton. This aligns with the broader refactoring effort to centralize error reporting through the AppsmithTelemetry 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 previous FaroUtil. 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
Importing appsmithTelemetry from the shared instrumentation entrypoint aligns with the new singleton pattern and replaces the old standalone error utility.


50-56: Consistent Exception Capture
Switching to appsmithTelemetry.captureException maintains existing metadata (errorName) and centralizes error reporting.

app/client/src/widgets/MapChartWidget/component/utilities.ts (1)

6-6: Centralized Telemetry Import
Importing appsmithTelemetry 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 standalone getTraceAndContext import with appsmithTelemetry.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 to appsmithTelemetry here aligns Canvas error reporting with the centralized telemetry strategy.

app/client/src/api/helpers/validateJsonResponseMeta.ts (1)

1-1: Standardize Telemetry Import
Importing appsmithTelemetry 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
Importing appsmithTelemetry from "instrumentation" aligns with the centralized telemetry refactor and replaces the old standalone utility.


23-25: Use of appsmithTelemetry.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 to appsmithTelemetry ensures consistency with the new telemetry singleton across the codebase.


42-42: Centralized exception capture
Using appsmithTelemetry.captureException inside componentDidCatch 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 of appsmithTelemetry replaces the removed sendFaroErrors utility, centralizing all capture calls.


293-299: Prepared statement error handling
Capturing a missing-value error via appsmithTelemetry.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 old captureException import aligns error reporting to the new AppsmithTelemetry class.


98-100: Exception report in resend hook
Logging the missing-email scenario with appsmithTelemetry.captureException and errorName "EmailVerificationRetryError" is consistent and effective.

app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/FormRender.tsx (2)

12-12: Telemetry import consolidation
Importing appsmithTelemetry from "instrumentation" replaces the legacy error sender and centralizes error tracking.


106-106: Form render error capture
Switching the catch block to appsmithTelemetry.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 importing captureException 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 system

The 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 system

The 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 system

The 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 system

The 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 system

The 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 system

The 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 system

The 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 system

The 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 good

The 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 refactoring

The change correctly implements the migration from the standalone captureException function to the new centralized appsmithTelemetry.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 consistently

The 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 refactoring

The 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 standalone captureException 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 standalone captureException 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 standalone captureException 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 standalone captureException 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 singleton

The import has been updated to use the appsmithTelemetry singleton from the central instrumentation module instead of directly importing the captureException function.


938-938: Error reporting refactored to use telemetry singleton

Updated 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 singleton

Similarly 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 singleton

Updated the import to use the appsmithTelemetry singleton from the central instrumentation module instead of directly importing the captureException function.


240-242: Updated error reporting to use telemetry singleton

The 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 singleton

Updated the import to use the appsmithTelemetry singleton from the central instrumentation module instead of directly importing the captureException function.


119-121: Updated error reporting to use telemetry singleton

The 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 singleton

Updated the import to use the appsmithTelemetry singleton from the central instrumentation module instead of directly importing the captureException function.


227-229: Updated error reporting to use telemetry singleton

The 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 good

The change from importing a standalone captureException function to using the appsmithTelemetry singleton is part of a broader refactoring effort to centralize error reporting.


192-194: Error reporting implementation updated correctly

The error capturing has been properly migrated to use the singleton instance method while maintaining the same error metadata.


253-255: Error reporting implementation updated correctly

The error capturing has been properly migrated to use the singleton instance method while maintaining the same error metadata.


318-320: Error reporting implementation updated correctly

The 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 good

The change from importing a standalone captureException function to using the appsmithTelemetry singleton is part of a broader refactoring effort to centralize error reporting.


502-504: Error reporting implementation updated correctly

The error capturing has been properly migrated to use the singleton instance method while maintaining the same error metadata.


522-524: Error reporting implementation updated correctly

The error capturing has been properly migrated to use the singleton instance method while maintaining the same error metadata.


582-584: Error reporting implementation updated correctly

The 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 good

The change from importing a standalone captureException function to using the appsmithTelemetry singleton is part of a broader refactoring effort to centralize error reporting.


281-287: Error reporting implementation updated correctly

The 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 correctly

The 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 correctly

The 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 good

The change from importing a standalone captureException function to using the appsmithTelemetry singleton is part of a broader refactoring effort to centralize error reporting.


238-251: Error reporting implementation updated correctly

The 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 correctly

The error capturing for eval tree errors has been properly migrated to use the singleton instance method.


278-281: All error reporting implementations updated correctly

All 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

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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: Add tracerProvider.register() to activate tracing.

The WebTracerProvider is instantiated and processors are added, but never registered. Without tracerProvider.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 expect pushError 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 against hint being undefined to avoid runtime crashes.

Provide a default empty object for the hint parameter to ensure the for (const key in hint) loop on line 142 won't throw TypeError when hint 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)

📥 Commits

Reviewing files that changed from the base of the PR and between 7118c0a and caaee4b.

📒 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

@ApekshaBhosale ApekshaBhosale merged commit 7239cdd into release Apr 29, 2025
83 checks passed
@ApekshaBhosale ApekshaBhosale deleted the chore/refactor-faro branch April 29, 2025 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Required label for CI skip-changelog Adding this label to a PR prevents it from being listed in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants