chore: Add deviceType and appMode to new relic telemetry data#33735
chore: Add deviceType and appMode to new relic telemetry data#33735rajatagrawal merged 7 commits intoreleasefrom
Conversation
github-actions
bot
added
the
skip-changelog
dvj1988
added
ok-to-test
WalkthroughThe updates primarily enhance telemetry functionality across various modules. These changes introduce common telemetry attributes, utilize Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant UI
participant Telemetry
participant Worker
participant Evaluator
User->>UI: Interacts with UI
UI->>Telemetry: startRootSpan(spanName, spanAttributes)
Telemetry->>UI: Returns span
UI->>Worker: Calls method with telemetryAttributes
Worker->>Evaluator: Calls evaluateTree with appMode and telemetryAttributes
Evaluator->>Telemetry: Logs telemetry data
Evaluator->>Worker: Returns evaluation result
Worker->>UI: Returns result
UI->>User: Displays result
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (16)
app/client/src/UITelemetry/generateTraces.ts (3)
10-10: Add a comment explaining whydeviceTypeis imported fromreact-device-detect.Adding a brief comment here about the purpose of importing
deviceTypecan help maintain clarity, especially for new developers or those unfamiliar with thereact-device-detectlibrary.
Line range hint
46-67: Similar to the previous comment, add error handling for undefinedtracerinstartNestedSpan.const generatorTrace = trace.getTracer(GENERATOR_TRACE); + if (!generatorTrace) { + throw new Error("Generator Trace not found for GENERATOR_TRACE"); + } if (!spanName || !parentSpan) { // do not generate nested span without parentSpan..we cannot generate context out of it return; }
Line range hint
81-81: Specify a type other thananyfor the function return type inwrapFnWithParentTraceContext.- export function wrapFnWithParentTraceContext(parentSpan: Span, fn: () => any) { + export function wrapFnWithParentTraceContext<T>(parentSpan: Span, fn: () => T): T { const parentContext = trace.setSpan(context.active(), parentSpan); return context.with(parentContext, fn); };This change improves type safety by allowing the function to handle different return types explicitly.
app/client/src/utils/WorkerUtil.ts (3)
15-15: Add a comment explaining the purpose of importingAttributesfrom@opentelemetry/api.A brief comment here about the purpose of importing
Attributescan help maintain clarity, especially for new developers or those unfamiliar with the@opentelemetry/apilibrary.
Line range hint
158-194: RefactoraddChildSpansToRootSpanto handle potential undefinedrootSpanmore gracefully.if (!rootSpan) { return; } const webworkerTelemetryResponse = webworkerTelemetry as Record< string, WebworkerSpanData >;This change ensures that the function exits early if
rootSpanis undefined, preventing further execution and potential errors.
Line range hint
209-258: Ensure proper error handling and type safety in therequestgenerator function.*request( method: string, data: Record<string, unknown> = {}, telemetryAttributes: Attributes = {}, ): Generator<any, any, any> { yield this.ready(true); if (!this._Worker) { + throw new Error("Worker is not initialized"); return; }Adding explicit error handling for an uninitialized worker enhances robustness and prevents silent failures.
app/client/src/workers/Evaluation/handlers/evalTree.ts (3)
Line range hint
146-197: Refactor to simplify the handling ofappModein various profile functions.const setupFirstTreeResponse = profileFn( "setupFirstTree", { description: "non-initialisation", appMode }, webworkerTelemetry, () => dataTreeEvaluator?.setupFirstTree(unevalTree, configTree), );This change ensures that
appModeis consistently passed and handled, reducing the risk of errors due to missing or incorrect parameters.
Line range hint
256-311: Optimize the handling ofappModeindiffAndGenerateSerializeUpdatesto ensure consistency and correctness.const updates = profileFn( "diffAndGenerateSerializeUpdates", { appMode }, webworkerTelemetry, () => { let updates; if (isNewTree) { try { updates = serialiseToBigInt([{ kind: "newTree", rhs: dataTree }]); dataTreeEvaluator?.setPrevState(dataTree); } catch (e) { updates = "[]"; } isNewTree = false; } else { const allUnevalUpdates = unEvalUpdates.map( (update) => update.payload.propertyPath, );This change ensures that
appModeis consistently passed and handled, reducing the risk of errors due to missing or incorrect parameters.
[!TIP]
Codebase VerificationThe search results show multiple instances of
profileFnusage across different files. In theevalTree.tsfile,appModeis consistently passed as an argument toprofileFn. However, in theindex.tsfile within theDataTreeEvaluatordirectory,appModeis not explicitly passed in several instances.
- Files and lines to review:
app/client/src/workers/common/DataTreeEvaluator/index.ts
- Lines where
profileFnis called withoutappMode:
- Line 96:
const parsedCollections = profileFn("SetupFirstTree.parseJSActions", undefined, webworkerTelemetry);- Line 102:
const { dependencies, inverseDependencies } = profileFn("createDependencyMap", undefined, webworkerTelemetry);- Line 108:
>[] = profileFn("SetupUpdateTree.Diff1", telemetryAttributes, webworkerTelemetry);- Line 114:
const parsedCollections = profileFn("SetupUpdateTree.parseJSActions", telemetryAttributes, webworkerTelemetry);- Line 120:
} = profileFn("updateDependencyMap", telemetryAttributes, webworkerTelemetry);- Line 126:
const setupUpdateTreeOutput = profileFn("setupTree", telemetryAttributes, webworkerTelemetry);Please ensure that
appModeis passed correctly in these instances to maintain consistency and safety.Analysis chain
Line range hint
99-114: Ensure thatappModeis always passed correctly and handled safely inprofileFn.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `appMode` is always provided and valid. # Test: Search for all usages of `profileFn` to ensure `appMode` is always passed. rg --type typescript $'profileFn'Length of output: 67
Script:
#!/bin/bash # Description: Verify that `appMode` is always provided and valid. # Test: Search for all usages of `profileFn` to ensure `appMode` is always passed. rg --type ts 'profileFn'Length of output: 1496
Script:
#!/bin/bash # Description: Extract lines around each `profileFn` usage to inspect arguments. # Extract lines around each `profileFn` usage in the relevant files. rg --type ts 'profileFn' -A 3 -B 3Length of output: 9913
app/client/src/workers/common/DataTreeEvaluator/index.ts (7)
136-136: Consider using a more specific import forAttributesto avoid potential conflicts with other libraries.
488-488: Ensure thattelemetryAttributesare properly documented to clarify their usage within the method.
Line range hint
463-476: Consider replacingforEachwithfor...offor better performance and readability.- Object.keys(unEvalJSCollection).forEach((update) => { - const updates = unEvalJSCollection[update]; - if (!!unevalTree[update]) { - Object.keys(updates).forEach((key) => { - const data = get(unevalTree, `${update}.${key}.data`, undefined); - if (isJSObjectFunction(unevalTree, update, key, configTree)) { - set(unevalTree, `${update}.${key}`, new String(updates[key])); - set(unevalTree, `${update}.${key}.data`, data); - } else { - set(unevalTree, `${update}.${key}`, updates[key]); - } - }); - } - }); + for (const update of Object.keys(unEvalJSCollection)) { + const updates = unEvalJSCollection[update]; + if (unevalTree[update]) { + for (const key of Object.keys(updates)) { + const data = get(unevalTree, `${update}.${key}.data`, undefined); + if (isJSObjectFunction(unevalTree, update, key, configTree)) { + set(unevalTree, `${update}.${key}`, new String(updates[key])); + set(unevalTree, `${update}.${key}.data`, data); + } else { + set(unevalTree, `${update}.${key}`, updates[key]); + } + } + } + }
Line range hint
3-3: Avoid shadowing the globalEvalErrorproperty. Consider renaming the localEvalErrortype to prevent potential conflicts.- import type { EvalError } from "utils/DynamicBindingUtils"; + import type { EvaluationError as EvalError } from "utils/DynamicBindingUtils";
Line range hint
175-175: Avoid usinganytype for better type safety. Specify a more explicit type or use generics if applicable.Also applies to: 229-229, 485-485, 725-725, 1039-1039, 1343-1343
Line range hint
992-1000: This default parameter should follow the last required parameter or should be a required parameter to maintain consistency and avoid potential bugs.- setupUpdateTree(unEvalTree: any, configTree: ConfigTree, webworkerTelemetry: Record<string, WebworkerSpanData> = {}, telemetryAttributes: Attributes = {}) + setupUpdateTree(unEvalTree: any, configTree: ConfigTree, telemetryAttributes: Attributes = {}, webworkerTelemetry: Record<string, WebworkerSpanData> = {})
Line range hint
1298-1334: This else clause can be omitted because previous branches break early, simplifying the control flow.- if (condition1) { - // code - } else if (condition2) { - // code - } else { - // code - } + if (condition1) { - // code - } else if (condition2) { - // code - }
|
|
||
| const getCommonTelemetryAttributes = () => { | ||
| return { | ||
| deviceType, |
There was a problem hiding this comment.
Can we please add appMode also as common telemetry attribute ?
There was a problem hiding this comment.
@rajatagrawal The test cases were failing because the worker was trying to access the window.location object which is not available in the worker's scope.
This is happening because we are creating the root span inside the request method of the worker service and this makes it impossible to access the common attributes unless they are passed as an additional argument to the request method. I am reverting the code to the previous state for now.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
app/client/src/UITelemetry/generateTraces.ts (1)
Line range hint
94-94: Specify a more precise type thanany.Using
anycan lead to potential type safety issues. Specify a more precise type for better type checking.- export function wrapFnWithParentTraceContext(parentSpan: Span, fn: () => any) { + export function wrapFnWithParentTraceContext(parentSpan: Span, fn: () => void) {
dvj1988
added
ok-to-test
dvj1988
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
app/client/src/UITelemetry/generateTraces.ts (2)
Line range hint
58-79: OptimizestartNestedSpanby ensuringparentSpanis checked early.The function
startNestedSpanchecks forparentSpaninside the function body. It's more efficient to check this at the beginning to avoid unnecessary operations:export function startNestedSpan( spanName: string, parentSpan?: Span, spanAttributes: Attributes = {}, startTime?: TimeTimeInput, ) { + if (!spanName || !parentSpan) { + return; + } ... }
Line range hint
93-93: Specify a type instead of usingany.The function
wrapFnWithParentTraceContextusesanyfor the return type offn. Usinganycan lead to potential type safety issues. Specify a more appropriate type if possible:- export function wrapFnWithParentTraceContext(parentSpan: Span, fn: () => any) { + export function wrapFnWithParentTraceContext<T>(parentSpan: Span, fn: () => T): T { ... }
There was a problem hiding this comment.
Refactor getCommonTelemetryAttributes for better error handling.
The function getCommonTelemetryAttributes currently assumes that window.location will always be available, which might not be the case in all environments (e.g., server-side rendering or tests). Consider adding a guard clause to handle such scenarios:
const getCommonTelemetryAttributes = () => {
+ if (typeof window === 'undefined' || !window.location) {
+ return { deviceType: 'unknown', appMode: 'unknown' };
+ }
const { pathname } = window.location;
...
};Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const getCommonTelemetryAttributes = () => { | |
| const { pathname } = window.location; | |
| const appMode = isEditorPath(pathname) | |
| ? APP_MODE.EDIT | |
| : isViewerPath(pathname) | |
| ? APP_MODE.PUBLISHED | |
| : ""; | |
| return { | |
| deviceType, | |
| appMode, | |
| }; | |
| }; | |
| const getCommonTelemetryAttributes = () => { | |
| if (typeof window === 'undefined' || !window.location) { | |
| return { deviceType: 'unknown', appMode: 'unknown' }; | |
| } | |
| const { pathname } = window.location; | |
| const appMode = isEditorPath(pathname) | |
| ? APP_MODE.EDIT | |
| : isViewerPath(pathname) | |
| ? APP_MODE.PUBLISHED | |
| : ""; | |
| return { | |
| deviceType, | |
| appMode, | |
| }; | |
| }; |
There was a problem hiding this comment.
Ensure proper error handling in startRootSpan.
The function startRootSpan uses optional chaining (tracer?.startSpan) which could lead to silent failures if tracer is undefined. It's better to handle this explicitly:
const tracer = trace.getTracer(GENERATOR_TRACE);
+ if (!tracer) {
+ throw new Error('Tracer not found for GENERATOR_TRACE');
+ }
if (!spanName) {
return;
}
...Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| spanAttributes: Attributes = {}, | |
| startTime?: TimeInput, | |
| ) { | |
| const tracer = trace.getTracer(GENERATOR_TRACE); | |
| if (!spanName) { | |
| return; | |
| } | |
| const attributes = spanAttributes ?? { attributes: spanAttributes }; | |
| const startTimeAttr = startTime ? { startTime } : {}; | |
| const commonAttributes = getCommonTelemetryAttributes(); | |
| return tracer?.startSpan(spanName, { | |
| kind: SpanKind.CLIENT, | |
| ...attributes, | |
| ...startTimeAttr, | |
| attributes: { | |
| ...commonAttributes, | |
| ...spanAttributes, | |
| }, | |
| startTime, | |
| spanAttributes: Attributes = {}, | |
| startTime?: TimeInput, | |
| ) { | |
| const tracer = trace.getTracer(GENERATOR_TRACE); | |
| if (!tracer) { | |
| throw new Error('Tracer not found for GENERATOR_TRACE'); | |
| } | |
| if (!spanName) { | |
| return; | |
| } | |
| const commonAttributes = getCommonTelemetryAttributes(); | |
| return tracer.startSpan(spanName, { | |
| kind: SpanKind.CLIENT, | |
| attributes: { | |
| ...commonAttributes, | |
| ...spanAttributes, | |
| }, | |
| startTime, |
|
/build-deploy-preview skip-tests=true |
1 similar comment
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9268469276. |
|
Deploy-Preview-URL: https://ce-33735.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
dvj1988
added
ok-to-test
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9289034182. |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
app/client/src/UITelemetry/generateTraces.ts (1)
Line range hint
95-95: Specify a more explicit type thanany.- export function wrapFnWithParentTraceContext(parentSpan: Span, fn: () => any) { + export function wrapFnWithParentTraceContext<T>(parentSpan: Span, fn: () => T): T { const parentContext = trace.setSpan(context.active(), parentSpan); return context.with(parentContext, fn); }
|
Deploy-Preview-URL: https://ce-33735.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9289345647. |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
app/client/src/UITelemetry/generateTraces.ts (1)
Line range hint
95-95: Specify a more appropriate type instead ofany.- export function wrapFnWithParentTraceContext(parentSpan: Span, fn: () => any) { + export function wrapFnWithParentTraceContext(parentSpan: Span, fn: () => void) {
|
Deploy-Preview-URL: https://ce-33735.dp.appsmith.com |
dvj1988
added
ok-to-test
dvj1988
added
ok-to-test
3 participants
Description
Add deviceType and appMode attributes to new relic spans related to evaluation
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/9297145435
Commit: 309d8ae
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?