chore: fix issues in eval error logs sent to faro #40285
Conversation
WalkthroughThis change enhances error handling and reporting throughout the evaluation and instrumentation subsystems. It introduces stack trace support to error objects by updating the Changes
Sequence Diagram(s)sequenceDiagram
participant EvalWorker as Evaluation Worker
participant Helper as Helper Utilities
participant ErrorLogger as Error Logger
participant Faro as Faro Instrumentation
EvalWorker->>Helper: reconstructErrorFromEvalError(serializedError)
Helper-->>EvalWorker: Error object with message, stack, context
EvalWorker->>ErrorLogger: errorLogger.error(error)
EvalWorker->>Faro: captureException(error)
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
|
github-actions
bot
added
the
skip-changelog
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/src/sagas/helper.ts (2)
40-40: Consider renaming the imported type to avoid shadowing.The imported type
EvalErrorshadows the globalEvalErrorproperty, which could lead to confusion. Consider renaming it to something likeEvaluationErrorto avoid potential issues.-import type { EvalError } from "utils/DynamicBindingUtils"; +import type { EvalError as EvaluationError } from "utils/DynamicBindingUtils";🧰 Tools
🪛 Biome (1.9.4)
[error] 40-40: Do not shadow the global "EvalError" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
64-78: Well-structured utility for reconstructing error objects.This function properly reconstructs standard Error objects from serialized evaluation errors, preserving important properties like stack traces and context. This will ensure consistent error reporting to monitoring systems like Faro.
A few suggestions for improvement:
- Consider adding JSDoc type annotations for the return value
- Consider handling potential undefined message values
/** * Reconstructs an EvalError from a serialized error object. This attaches the correct stack trace and error type to the error. * this is used to send the error to faro. * * @param serializedError - The serialized error object to reconstruct. - * @returns A reconstructed Error object. + * @returns {Error} A reconstructed Error object with original stack and context. */ -export function reconstructErrorFromEvalError(serializedError: EvalError) { - const error = new Error(serializedError.message); +export function reconstructErrorFromEvalError(serializedError: EvaluationError) { + const error = new Error(serializedError.message || 'Unknown evaluation error'); if (serializedError.stack) { error.stack = serializedError.stack; } if (serializedError.context) { Object.assign(error, { context: serializedError.context, }); } return error; }app/client/src/workers/Evaluation/helpers.ts (1)
9-9: Warning: Potential naming conflict with global EvalError.While importing the EvalError type is necessary for typing, this name conflicts with the JavaScript global
EvalErrorobject.Consider using a more specific name like
AppEvalErrororCustomEvalErrorto avoid confusion with the JavaScript global.🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Do not shadow the global "EvalError" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/client/src/instrumentation/index.ts(2 hunks)app/client/src/instrumentation/sendFaroErrors.ts(2 hunks)app/client/src/sagas/EvalErrorHandler.ts(6 hunks)app/client/src/sagas/helper.ts(2 hunks)app/client/src/utils/DynamicBindingUtils.ts(1 hunks)app/client/src/workers/Evaluation/JSObject/index.ts(1 hunks)app/client/src/workers/Evaluation/handlers/evalTree.ts(1 hunks)app/client/src/workers/Evaluation/helpers.ts(4 hunks)app/client/src/workers/common/DataTreeEvaluator/index.ts(2 hunks)app/client/src/workers/common/DependencyMap/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/client/src/sagas/helper.ts (1)
app/client/src/utils/DynamicBindingUtils.ts (1)
EvalError(166-173)
app/client/src/sagas/EvalErrorHandler.ts (3)
app/client/src/sagas/helper.ts (1)
reconstructErrorFromEvalError(64-78)app/client/src/instrumentation/sendFaroErrors.ts (1)
captureException(5-19)app/client/src/utils/Analytics/sentry.ts (1)
captureException(19-21)
app/client/src/workers/Evaluation/helpers.ts (1)
app/client/src/utils/DynamicBindingUtils.ts (1)
EvalError(166-173)
🪛 Biome (1.9.4)
app/client/src/sagas/helper.ts
[error] 40-40: Do not shadow the global "EvalError" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
app/client/src/workers/Evaluation/helpers.ts
[error] 9-9: Do not shadow the global "EvalError" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
🔇 Additional comments (24)
app/client/src/utils/DynamicBindingUtils.ts (1)
430-430: Good addition of stack trace to EvaluationError interface.Adding the optional stack property to the EvaluationError interface enables proper stack trace propagation in error objects. This will help with debugging by providing the complete error context in monitoring systems.
app/client/src/instrumentation/sendFaroErrors.ts (2)
2-2: Appropriate use of structured logging.Importing the error logger from loglevel instead of using console directly is a good practice for consistent error handling.
15-15: Proper replacement of direct console usage.Replacing console.error with errorLogger ensures consistent error logging behavior throughout the application.
app/client/src/workers/common/DependencyMap/utils.ts (1)
112-112: Excellent preservation of error stack traces.Adding the stack property when constructing the EvalError object ensures the original stack trace is preserved when propagating errors through the application, which will significantly improve debugging capabilities.
app/client/src/instrumentation/index.ts (2)
18-18: Good import of LogLevel enum.Adding the LogLevel import enables more granular control over console logging instrumentation.
61-66: Great implementation of granular log level filtering.Replacing the simple boolean flag with a specific list of disabled log levels provides much better control over which console messages are captured by Faro. This configuration ensures that ERROR and WARN level logs are still captured while filtering out less critical log levels.
app/client/src/workers/Evaluation/JSObject/index.ts (1)
213-221: Good addition of detailed error reporting.Adding stack trace capture and structured error context will significantly improve debugging of JS parsing issues. The error object now includes type, message, stack trace, and contextual information which will help track down the root cause of failures in the evaluation pipeline.
app/client/src/workers/common/DataTreeEvaluator/index.ts (2)
1208-1209: Stack trace inclusion enhances error reporting.Good addition of stack trace to property evaluation errors. This will help developers trace the exact execution path that led to failures.
1412-1413: Consistent stack trace capture for tree-level errors.Adding stack trace capture here ensures consistency with the property-level error handling above. This pattern makes error reporting more uniform across different error types.
app/client/src/workers/Evaluation/handlers/evalTree.ts (1)
311-315: Enhanced error handling for serialization failures.Good improvement. Previously errors during serialization would only reset updates without capturing detailed diagnostic information. Now we'll have proper stack traces and error context for debugging serialization issues.
app/client/src/sagas/EvalErrorHandler.ts (11)
33-33: Added essential import for error reconstruction.This import brings in a new helper function that ensures errors maintain their stack traces when sent to monitoring systems.
221-222: Good enhancement to preserve stack traces in errors.The reconstructed error object preserves the original stack trace and context, making debugging far more effective. This change addresses the issue of "[object Object]" errors appearing in Faro.
238-250: Updated error capture for cyclical dependency errors.Using the reconstructed error ensures the stack trace is properly captured for cyclical dependency errors.
271-272: Updated error capture for eval tree errors.Correctly using the reconstructed error for eval tree errors.
276-279: Updated error capture for bad uneval tree errors.Using the reconstructed error with proper formatting.
282-285: Updated error capture for eval property errors.Using the reconstructed error for property evaluation errors.
290-296: Updated error capture for clone errors.Using the reconstructed error while preserving all context information.
306-310: Updated error capture for parse JS errors.Using the reconstructed error for JavaScript parsing errors.
313-317: Updated error capture for extract dependency errors.Using the reconstructed error for dependency extraction errors.
321-324: Updated error capture for update data tree errors.Using the reconstructed error for data tree update errors.
332-333: Updated error capture for unknown errors.Using the reconstructed error for unknown evaluation errors.
app/client/src/workers/Evaluation/helpers.ts (3)
468-468: Updated return type to use proper EvalError typing.Using the imported EvalError type improves type safety and maintains consistency across the codebase.
494-495: Added stack trace to serialization errors.Including the stack trace in the error object is crucial for debugging serialization issues.
551-552: Added stack trace to data tree update errors.Including the stack trace in errors during data tree updates will significantly improve debugging capabilities.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/sagas/EvalWorkerActionSagas.ts (1)
29-39: Consider addressing TODOs in a future PR.The file contains multiple TODOs related to disabling ESLint rules (
@typescript-eslint/no-explicit-any). While not directly related to the current changes, addressing these in a future PR would improve type safety and code maintainability.Also applies to: 41-55, 57-64, 66-97, 99-117, 119-185
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
app/client/src/sagas/EvalWorkerActionSagas.ts(1 hunks)app/client/src/sagas/types.ts(1 hunks)app/client/src/workers/Evaluation/evalTreeWithChanges.test.ts(1 hunks)app/client/src/workers/Evaluation/evalTreeWithChanges.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- app/client/src/sagas/types.ts
- app/client/src/workers/Evaluation/evalTreeWithChanges.test.ts
- app/client/src/workers/Evaluation/evalTreeWithChanges.ts
🔇 Additional comments (3)
app/client/src/sagas/EvalWorkerActionSagas.ts (3)
27-27: Appropriate refactoring of type definition.Moving the
UpdateDataTreeMessageDatainterface to a separate types file improves code organization and promotes reusability across modules. This aligns with best practices for TypeScript projects.
166-166: LGTM: Type usage looks correct.The imported type is being properly used here for the destructured data object.
184-184:✅ Verification successful
Verify error handling across the application.
This line is crucial for the PR objective of fixing evaluation error logs. While no changes were made here, ensure that the
evalErrorHandlerfunction properly captures and propagates error stack traces to improve debugging.
🏁 Script executed:
#!/bin/bash # Check if evalErrorHandler has been modified elsewhere in the PR git diff -- app/client/src/sagas/EvalErrorHandler.ts # Look at current implementation cat app/client/src/sagas/EvalErrorHandler.tsLength of output: 10846
🏁 Script executed:
#!/bin/bash # Locate the implementation of reconstructErrorFromEvalError rg "reconstructErrorFromEvalError" -A5 -B5 --glob "*.ts"Length of output: 2439
🏁 Script executed:
#!/bin/bash # Display full reconstructErrorFromEvalError implementation with surrounding lines rg -n -A10 "export function reconstructErrorFromEvalError" app/client/src/sagas/helper.tsLength of output: 438
No action needed:
evalErrorHandleralready preserves error stacks
ThereconstructErrorFromEvalErrorhelper assigns the originalstack(andcontext) onto the newErrorbefore it’s sent tocaptureException, so full traces are retained for debugging.
dvj1988
added
ok-to-test
## Description Errors sent from evaluation worker shows up as following in faro. This entry is illegible and doesn't have the correct stack trace. ``` console.error: [object Object] [object Object] at ? (https://app.appsmith.com/src/instrumentations/console/instrumentation.ts:28:33) at log (https://app.appsmith.com/static/js/sagas/ErrorSagas.tsx:322:22) ``` This pr sends a reconstructed error object for eval errors and retains the original stack trace of the error ``` error: Fetch computation cache failed!! at AppComputationCache.getCachedComputationResult (https://dev.appsmith.com/static/js/src_workers_Evaluation_handlers_evalTree_ts-node_modules_blueprintjs_core_lib_esm_components_-d37317.chunk.js:48831:11) at createDependencyMap (https://dev.appsmith.com/static/js/src_workers_Evaluation_handlers_evalTree_ts-node_modules_blueprintjs_core_lib_esm_components_-d37317.chunk.js:50756:98) at ? (https://dev.appsmith.com/static/js/src_workers_Evaluation_handlers_evalTree_ts-node_modules_blueprintjs_core_lib_esm_components_-d37317.chunk.js:49278:221) at profileAsyncFn (https://dev.appsmith.com/static/js/src_workers_Evaluation_handlers_evalTree_ts-node_modules_blueprintjs_core_lib_esm_components_-d37317.chunk.js:37451:21) at DataTreeEvaluator.setupFirstTree (https://dev.appsmith.com/static/js/src_workers_Evaluation_handlers_evalTree_ts-node_modules_blueprintjs_core_lib_esm_components_-d37317.chunk.js:49278:103) at async profileAsyncFn (https://dev.appsmith.com/static/js/src_workers_Evaluation_handlers_evalTree_ts-node_modules_blueprintjs_core_lib_esm_components_-d37317.chunk.js:37451:15) at async evalTree (https://dev.appsmith.com/static/js/src_workers_Evaluation_handlers_evalTree_ts-node_modules_blueprintjs_core_lib_esm_components_-d37317.chunk.js:46384:38) at async asyncRequestMessageListener (https://dev.appsmith.com/static/js/evalWorker.chunk.js:236:16) ``` 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 <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/14512303434> > Commit: 3bbb8c1 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=14512303434&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Thu, 17 Apr 2025 10:31:54 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced error reporting across the application by including stack traces with error messages, providing more detailed information for debugging. - **Bug Fixes** - Improved error handling in various areas to ensure errors are consistently captured and reported with additional context. - **Chores** - Updated internal logging mechanisms for better error tracking and maintainability. - Refined error reconstruction and logging processes for more accurate error capture and reporting. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Errors sent from evaluation worker shows up as following in faro. This entry is illegible and doesn't have the correct stack trace.
This pr sends a reconstructed error object for eval errors and retains the original stack trace of the error
Fixes #
Issue Numberor
Fixes
Issue URLWarning
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/14512303434
Commit: 3bbb8c1
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 17 Apr 2025 10:31:54 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Chores