chore: add first load telemetry to evaluations #33989
Conversation
|
Warning Rate limit exceeded@rajatagrawal has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 42 minutes and 24 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. CommitsFiles that changed from the base of the PR and between 8ca496e57a11a5249e71f3dd2938dc748842bc66 and 4cdd01d. WalkthroughThe recent updates primarily focus on refining telemetry data handling in the application. This includes the introduction of new type aliases, adjustments to function signatures, and enhancements to telemetry processing functions. The changes emphasize improved type safety and streamlined data handling, particularly in the context of web worker telemetry. Additionally, new conditional checks and utility functions have been introduced to enhance the robustness and maintainability of the codebase. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
the
skip-changelog
rajatagrawal
force-pushed
the
chore/add-first-load-telemetry-to-evaluations
branch
from
8ca496e to
f6be7a6
Compare
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 326b617 and 8ca496e57a11a5249e71f3dd2938dc748842bc66.
Files selected for processing (8)
- app/client/src/UITelemetry/generateTraces.ts (4 hunks)
- app/client/src/UITelemetry/generateWebWorkerTraces.ts (5 hunks)
- app/client/src/ce/workers/common/types.ts (2 hunks)
- app/client/src/sagas/ActionExecution/PluginActionSaga.ts (1 hunks)
- app/client/src/utils/WorkerUtil.ts (6 hunks)
- app/client/src/workers/Evaluation/handlers/evalTree.ts (3 hunks)
- app/client/src/workers/Evaluation/types.ts (2 hunks)
- app/client/src/workers/common/DataTreeEvaluator/index.ts (3 hunks)
Additional comments not posted (10)
app/client/src/ce/workers/common/types.ts (1)
16-16: The update towebworkerTelemetryenhances flexibility in handling telemetry data.#!/bin/bash # Description: Verify integration of `webworkerTelemetry` with other components. # Test: Search for usage of `WorkerRequest` and ensure proper handling of the new telemetry types. rg --type typescript $'WorkerRequest'app/client/src/UITelemetry/generateWebWorkerTraces.ts (1)
5-5: Refined telemetry handling with updated types and new filtering function.#!/bin/bash # Description: Verify integration of `filterSpanData` with telemetry processing. # Test: Search for usage of `filterSpanData` and ensure it is integrated correctly. rg --type typescript $'filterSpanData'Also applies to: 16-16, 32-33, 45-47, 57-64
app/client/src/UITelemetry/generateTraces.ts (1)
17-18: Standardized telemetry types and updated function signatures for better trace management.#!/bin/bash # Description: Verify integration of `OtlpSpan` and `SpanAttributes` with telemetry functions. # Test: Search for usage of these types in telemetry functions and ensure they are handled correctly. rg --type typescript $'OtlpSpan|SpanAttributes'Also applies to: 39-45, 59-60, 79-80, 83-83
app/client/src/workers/Evaluation/types.ts (1)
19-19: UpdatedwebworkerTelemetryto accept more flexible telemetry data types, enhancing data handling in evaluation processes.#!/bin/bash # Description: Verify integration of `webworkerTelemetry` with evaluation processes. # Test: Search for usage of `EvalTreeResponseData` and ensure proper handling of the new telemetry types. rg --type typescript $'EvalTreeResponseData'Also applies to: 63-63
app/client/src/utils/WorkerUtil.ts (1)
9-13: Enhanced worker utilities with robust telemetry handling and new functions for span management.#!/bin/bash # Description: Verify integration of new worker utilities with worker management processes. # Test: Search for usage of `GracefulWorkerService` and related functions to ensure they are integrated correctly. rg --type typescript $'GracefulWorkerService'Also applies to: 160-183, 219-275
app/client/src/workers/Evaluation/handlers/evalTree.ts (1)
35-35: Refined telemetry handling in the evaluation tree for better data management and debugging.#!/bin/bash # Description: Verify integration of new telemetry handling with evaluation tree processes. # Test: Search for usage of `evalTree` and related telemetry handling to ensure they are integrated correctly. rg --type typescript $'evalTree'Also applies to: 37-37, 89-90, 318-319
app/client/src/sagas/ActionExecution/PluginActionSaga.ts (1)
1665-1667: Proper handling of telemetry span inhandleUpdateActionData.The addition of
endSpanto ensure that the telemetry span is closed correctly is a good practice. This aligns with the PR's objective of enhancing telemetry handling.app/client/src/workers/common/DataTreeEvaluator/index.ts (3)
140-140: Ensure the new importSpanAttributesis utilized correctly throughout the file.
237-237: The default parameter forwebworkerTelemetrynow acceptsSpanAttributesin addition toWebworkerSpanData. Verify that all data passed to this parameter throughout the application is compatible with these types.#!/bin/bash # Description: Verify compatibility of data passed to `webworkerTelemetry`. # Test: Search for all usages of `webworkerTelemetry` and ensure compatibility. rg --type typescript $'webworkerTelemetry'
493-493: Similar to the previous comment, ensure that thewebworkerTelemetryparameter in thesetupUpdateTreemethod is handled correctly wherever it's used.#!/bin/bash # Description: Verify correct handling of `webworkerTelemetry` in `setupUpdateTree`. # Test: Search for all usages of `setupUpdateTree` and ensure correct handling. rg --type typescript $'setupUpdateTree'
rajatagrawal
force-pushed
the
chore/add-first-load-telemetry-to-evaluations
branch
from
f6be7a6 to
4cdd01d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (11)
app/client/src/sagas/ActionExecution/PluginActionSaga.ts (7)
Line range hint
310-314: Consider replacingforEachwithfor...offor better performance.- blobUrlPaths.forEach((blobUrlPath) => { + for (const blobUrlPath of blobUrlPaths) { const blobUrl = value[blobUrlPath] as string; const resolvedBlobValue: unknown = yield call(readBlob, blobUrl); set(value, blobUrlPath, resolvedBlobValue); const blobUrlPathMap = get(value, "blobUrlPaths", {}) as Record<string, string>; set(blobUrlPathMap, blobUrlPath, blobUrl); set(value, "blobUrlPaths", blobUrlPathMap); - }); + }
Line range hint
348-359: Consider replacingforEachwithfor...offor better performance.- Object.entries(blobUrlPaths as Record<string, string>).forEach(([path, blobUrl]) => { + for (const [path, blobUrl] of Object.entries(blobUrlPaths as Record<string, string>)) { if (isArrayBuffer(newVal[path])) { const sanitisedBlobURL = blobUrl.split("?")[0]; blobMap.push(sanitisedBlobURL); set(blobDataMap, sanitisedBlobURL, new Blob([newVal[path]])); set(newVal, path, sanitisedBlobURL); } - }); + }
Line range hint
457-476: Simplify computed expressions by using literal keys.- executeActionRequest.paramProperties[`k${index}`] = { + executeActionRequest.paramProperties.k${index} = { datatype: dataType, };
Line range hint
503-503: Simplify computed expressions by using literal keys.- executeActionRequest.paramProperties[`k${i}`] = { + executeActionRequest.paramProperties.k${i} = { datatype: { array: [arrDatatype[0]] }, };
Line range hint
509-517: Consider replacingforEachwithfor...offor better performance.- bindingBlob?.forEach((item) => formData.append(item.name, item.value)); + if (bindingBlob) { + for (const item of bindingBlob) { + formData.append(item.name, item.value); + } + }
Line range hint
727-727: Simplify computed expressions by using literal keys.- executeActionRequest.paramProperties[`k${i}`].blobIdentifiers = blobMap; + executeActionRequest.paramProperties.k${i}.blobIdentifiers = blobMap;
Line range hint
1047-1262: Avoid redundant double-negation for boolean coercion.- if (!!pluginAction.pluginId) { + if (pluginAction.pluginId) {app/client/src/workers/common/DataTreeEvaluator/index.ts (4)
Line range hint
469-482: Consider replacingforEachwithfor...ofto improve performance and readability.- Object.keys(unEvalJSCollection).forEach((update) => { + for (const update of Object.keys(unEvalJSCollection)) {
Line range hint
471-471: Remove redundant double-negation for cleaner and more readable code.- if (!!unevalTree[update]) { + if (unevalTree[update]) {
Line range hint
1271-1276: Avoid usingreturninsidefinallyblocks as it can lead to unexpected behavior by overridingtryandcatchblocks.
Line range hint
1567-1567: Use optional chaining to make the code more concise and safe.- if (fullPropertyPath && evalErrors.length) { + if (fullPropertyPath?.length && evalErrors.length) {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 8ca496e57a11a5249e71f3dd2938dc748842bc66 and f6be7a61dfd23f4abc4a8fecd0ff319c879c2170.
Files selected for processing (8)
- app/client/src/UITelemetry/generateTraces.ts (4 hunks)
- app/client/src/UITelemetry/generateWebWorkerTraces.ts (5 hunks)
- app/client/src/ce/workers/common/types.ts (2 hunks)
- app/client/src/sagas/ActionExecution/PluginActionSaga.ts (1 hunks)
- app/client/src/utils/WorkerUtil.ts (6 hunks)
- app/client/src/workers/Evaluation/handlers/evalTree.ts (2 hunks)
- app/client/src/workers/Evaluation/types.ts (2 hunks)
- app/client/src/workers/common/DataTreeEvaluator/index.ts (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- app/client/src/UITelemetry/generateTraces.ts
- app/client/src/ce/workers/common/types.ts
- app/client/src/workers/Evaluation/types.ts
Additional context used
Biome
app/client/src/UITelemetry/generateWebWorkerTraces.ts
[error] 49-55: Prefer for...of instead of forEach. (lint/complexity/noForEach)
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
app/client/src/utils/WorkerUtil.ts
[error] 181-181: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
app/client/src/workers/Evaluation/handlers/evalTree.ts
[error] 50-50: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 89-89: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 314-314: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
app/client/src/sagas/ActionExecution/PluginActionSaga.ts
[error] 310-314: Prefer for...of instead of forEach. (lint/complexity/noForEach)
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 348-359: Prefer for...of instead of forEach. (lint/complexity/noForEach)
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 457-457: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 459-459: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 460-460: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 461-461: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 473-473: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 474-474: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 475-475: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 476-476: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 503-503: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 509-509: Prefer for...of instead of forEach. (lint/complexity/noForEach)
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 515-517: Prefer for...of instead of forEach. (lint/complexity/noForEach)
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 727-727: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 1047-1047: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 1136-1136: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 1241-1241: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 1262-1262: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 1451-1451: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negationapp/client/src/workers/common/DataTreeEvaluator/index.ts
[error] 469-482: Prefer for...of instead of forEach. (lint/complexity/noForEach)
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 471-471: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 472-480: Prefer for...of instead of forEach. (lint/complexity/noForEach)
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 785-787: Prefer for...of instead of forEach. (lint/complexity/noForEach)
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 932-938: Prefer for...of instead of forEach. (lint/complexity/noForEach)
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 947-953: Prefer for...of instead of forEach. (lint/complexity/noForEach)
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 971-978: Prefer for...of instead of forEach. (lint/complexity/noForEach)
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 987-995: Prefer for...of instead of forEach. (lint/complexity/noForEach)
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 1271-1276: Unsafe usage of 'return'. (lint/correctness/noUnsafeFinally)
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
[error] 1567-1567: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1668-1681: Prefer for...of instead of forEach. (lint/complexity/noForEach)
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 1672-1673: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Additional comments not posted (2)
app/client/src/workers/common/DataTreeEvaluator/index.ts (2)
140-140: Ensure the new import ofSpanAttributesis utilized effectively in the file.
493-493: The addition ofwebworkerTelemetryandaffectedJSObjectsparameters tosetupUpdateTreemethod enhances telemetry and provides better handling of affected JavaScript objects. Ensure that all calls to this method across the codebase are updated to pass these new parameters.Verification successful
The search results indicate multiple instances of
setupUpdateTreeacross various files. To ensure that all calls to this method are updated to include the new parameterswebworkerTelemetryandaffectedJSObjects, we need to inspect these instances.Let's generate a script to extract the lines where
setupUpdateTreeis called, so we can verify if the new parameters are being passed.
The extracted lines show multiple instances of
setupUpdateTreebeing called. However, to verify if the new parameterswebworkerTelemetryandaffectedJSObjectsare being passed, we need to inspect these calls more closely.Let's generate a script to extract the full lines where
setupUpdateTreeis called, so we can verify if the new parameters are included.
The output shows multiple instances of
setupUpdateTreecalls, but it doesn't provide the full context needed to verify if the new parameters are being passed. To ensure all calls are updated correctly, we need to extract the surrounding lines for each instance.Let's generate a script to extract a few lines before and after each
setupUpdateTreecall to get the full context.
The extracted lines provide sufficient context to verify that the
setupUpdateTreemethod calls have been updated to include the new parameterswebworkerTelemetryandaffectedJSObjects.Here are the key observations:
- The
setupUpdateTreemethod calls inindex.tsandevalTree.tsinclude thewebworkerTelemetryparameter.- The test files (
test.ts,evaluation.test.ts) also show calls tosetupUpdateTreewith the new parameters.Based on this information, it appears that the necessary updates have been made to include the new parameters in the
setupUpdateTreemethod calls.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `setupUpdateTree` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type python $'setupUpdateTree'Length of output: 65
Script:
#!/bin/bash # Description: Verify all function calls to `setupUpdateTree` match the new signature. # Test: Search for the function usage in TypeScript files. Expect: Only occurrences of the new signature. rg --type typescript 'setupUpdateTree'Length of output: 73
Script:
#!/bin/bash # Description: Verify all function calls to `setupUpdateTree` match the new signature. # Test: Search for the function usage in all files. Expect: Only occurrences of the new signature. rg 'setupUpdateTree'Length of output: 4758
Script:
#!/bin/bash # Description: Extract lines where `setupUpdateTree` is called to verify the new parameters. # Extract lines with `setupUpdateTree` calls rg 'setupUpdateTree' -A 1 -B 1Length of output: 11035
Script:
#!/bin/bash # Description: Extract full lines where `setupUpdateTree` is called to verify the new parameters. # Extract full lines with `setupUpdateTree` calls rg 'setupUpdateTree' -o -NLength of output: 3144
Script:
#!/bin/bash # Description: Extract surrounding lines where `setupUpdateTree` is called to verify the new parameters. # Extract surrounding lines with `setupUpdateTree` calls rg 'setupUpdateTree' -A 3 -B 3Length of output: 21968
| return Object.keys(spanData) | ||
| .filter((key) => !key.startsWith("__")) | ||
| .reduce<Record<string, WebworkerSpanData>>((obj, key) => { |
There was a problem hiding this comment.
What are we performing this filter operation?
There was a problem hiding this comment.
Webworker is sending sub span evaluation data and attributes for the root span in the same object. The attributes start with __. This piece of code returns sub span evaluation data to be processed as spans
There was a problem hiding this comment.
Can you please approve the PR if the changes look good ?
|
LGTM |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Description
This PR adds an attribute to evaluations span that tells whether the evaluation is first evaluation on page load or a subsequent one.
This attribute will help us filter evaluations and measure gains in update evaluations and first page load evaluations separately.
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.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9381739425
Commit: 4cdd01d
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
endSpanto handleparentSpanin action execution.Refactor
SpanAttributes.New Features