[ENGG-5345] feat: distributed-tracing [desktop]#302
Open
Aarushsr12 wants to merge 2 commits intomasterfrom
Open
[ENGG-5345] feat: distributed-tracing [desktop]#302Aarushsr12 wants to merge 2 commits intomasterfrom
Aarushsr12 wants to merge 2 commits intomasterfrom
Conversation
WalkthroughThis pull request introduces a distributed tracing system for the Electron application using Sentry. The changes add tracing utilities for both the main process ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Aarushsr12
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/main/actions/tracingHelpers.js (1)
8-13: Capture untraced handler errors consistently.Line 12 executes the handler without the same exception capture path used in traced execution, so failures can be missed in telemetry.
💡 Proposed fix
if (!_traceContext || !_traceContext["sentry-trace"]) { - console.log( - `[Tracing] No trace context found for ${operationName}, running without trace` - ); - return handler(event, actualPayload); + try { + return await handler(event, actualPayload); + } catch (error) { + Sentry.captureException(error, { + tags: { + operation: operationName, + process: "main", + component: "ipc-handler", + traced: false, + }, + }); + throw error; + } }Also applies to: 33-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/tracingHelpers.js` around lines 8 - 13, The untraced branch returns handler(event, actualPayload) directly and skips the exception capture used in the traced branch; update the untraced execution path in tracingHelpers.js (the branch that checks !_traceContext or !_traceContext["sentry-trace"]) to invoke handler(event, actualPayload) inside the same try/catch and telemetry error capture flow used by the traced path (use the same Sentry.captureException or captureException call and rethrow/return behavior), and apply the same fix for the similar logic around lines handling the other untraced branch (the block referenced at 33-45) so both traced and untraced executions consistently capture exceptions for operationName and handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/actions/tracingHelpers.js`:
- Around line 6-7: The destructuring "const { _traceContext, ...actualPayload }
= payload;" assumes payload is a non-null object and will throw for
undefined/null/primitives; change it to first normalize/guard the value (e.g.,
create a safePayload by checking payload !== null && typeof payload === 'object'
? payload : {}) and then destructure from safePayload so _traceContext and
actualPayload are always defined without crashing; update references that use
actualPayload/_traceContext accordingly (look for the variables _traceContext
and actualPayload in this module).
In `@src/main/events.js`:
- Around line 197-199: In the get-api-response IPC handler remove the
intentional thrown error so requests can proceed: delete or comment out the line
that throws new Error("Intentional Tracing Error") and ensure the handler
returns the result of makeApiClientRequest(payload); (look for the
get-api-response handler containing makeApiClientRequest and the throw) so the
IPC API path is no longer forced to fail and the return becomes reachable.
In `@src/renderer/lib/RPCServiceOverIPC.ts`:
- Around line 33-56: Remove any logging or telemetry that includes full RPC
argument payloads (args, lastArg, cleanArgs) in RPCServiceOverIPC.ts; instead
log only safe metadata such as channelName, Array.isArray(args), args.length,
and a boolean for presence of traceContext. Keep the trace extraction logic that
reads lastArg._traceContext into traceContext and sets cleanArgs = args.slice(0,
-1) when present, but never include cleanArgs, lastArg, or the original args in
console logs or Sentry/event contexts; if you must record context for debugging,
redact or hash sensitive values before attaching. Also update any
error/reporting calls in the same scope to avoid passing args or lastArg into
processLogger/Sentry and only include non-sensitive fields (channelName,
argCount, hasTraceContext).
- Around line 44-50: The code assumes args is an array when computing
lastArg/hasTraceContext/cleanArgs in RPCServiceOverIPC.ts; add a guard to
validate Array.isArray(args) (or coerce non-array payloads to an empty array)
before using args.length/slice/indexing, compute lastArg only when args is a
non-empty array, and set traceContext/cleanArgs accordingly so malformed IPC
payloads won't throw and normal response handling still runs.
---
Nitpick comments:
In `@src/main/actions/tracingHelpers.js`:
- Around line 8-13: The untraced branch returns handler(event, actualPayload)
directly and skips the exception capture used in the traced branch; update the
untraced execution path in tracingHelpers.js (the branch that checks
!_traceContext or !_traceContext["sentry-trace"]) to invoke handler(event,
actualPayload) inside the same try/catch and telemetry error capture flow used
by the traced path (use the same Sentry.captureException or captureException
call and rethrow/return behavior), and apply the same fix for the similar logic
around lines handling the other untraced branch (the block referenced at 33-45)
so both traced and untraced executions consistently capture exceptions for
operationName and handler.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/actions/tracingHelpers.jssrc/main/events.jssrc/renderer/lib/RPCServiceOverIPC.ts
src/main/actions/tracingHelpers.js
Outdated
Comment on lines
+6
to
+7
| const { _traceContext, ...actualPayload } = payload; | ||
|
|
There was a problem hiding this comment.
Guard payload destructuring to prevent runtime crash.
Line 6 assumes payload is always a non-null object. If the IPC caller omits payload (or sends a primitive), this throws before tracing/handler logic runs.
💡 Proposed fix
export const withTracing = (operationName, handler) => {
return async (event, payload) => {
// Extract trace
- const { _traceContext, ...actualPayload } = payload;
+ const safePayload =
+ payload && typeof payload === "object" ? payload : {};
+ const { _traceContext, ...actualPayload } = safePayload;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/actions/tracingHelpers.js` around lines 6 - 7, The destructuring
"const { _traceContext, ...actualPayload } = payload;" assumes payload is a
non-null object and will throw for undefined/null/primitives; change it to first
normalize/guard the value (e.g., create a safePayload by checking payload !==
null && typeof payload === 'object' ? payload : {}) and then destructure from
safePayload so _traceContext and actualPayload are always defined without
crashing; update references that use actualPayload/_traceContext accordingly
(look for the variables _traceContext and actualPayload in this module).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/actions/tracingPropogation.js (2)
1-2: Inconsistent module system: mixingrequire()andimport.Line 1 uses CommonJS (
require) while line 2 uses ES modules (import). This inconsistency can cause confusion and potential issues depending on the bundler configuration. Consider using a consistent module system.💡 Proposed fix - use ES module import
-const Sentry = require("@sentry/electron/main"); -import { createTracedHandler } from "../lib/tracingMainUtils"; +import * as Sentry from "@sentry/electron/main"; +import { createTracedHandler } from "../lib/tracingMainUtils";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/tracingPropogation.js` around lines 1 - 2, The file mixes CommonJS and ES modules: Sentry is imported via require(" `@sentry/electron/main`") while createTracedHandler is imported with ES import; make the module system consistent by replacing the CommonJS require with an ES import for Sentry (i.e. import Sentry from "@sentry/electron/main") so both Sentry and createTracedHandler use the same import syntax, and ensure any tooling (babel/tsconfig/packager) supports ES imports in this file.
1-17: Filename typo: "Propogation" should be "Propagation".The filename
tracingPropogation.jscontains a spelling error. This will propagate to all imports referencing this file and may cause confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/tracingPropogation.js` around lines 1 - 17, Rename the file from tracingPropogation.js to tracingPropagation.js to fix the typo and update all imports that reference it; ensure the exported symbol withTracing and its usage of createTracedHandler and Sentry remain unchanged, then run a project-wide search (or adjust module paths) to replace any occurrences of "tracingPropogation" with "tracingPropagation" so imports continue to resolve correctly.src/main/lib/tracingMainUtils.ts (1)
1-11: Consider extracting sharedTraceContextinterface.The
TraceContextinterface is duplicated betweentracingMainUtils.tsandtracingRendererUtils.ts. If these interfaces need to stay in sync, consider extracting to a shared types file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/lib/tracingMainUtils.ts` around lines 1 - 11, TraceContext is duplicated across files; extract it to a shared types module (e.g., create and export interface TraceContext in a new shared file) and update both tracingMainUtils.ts and tracingRendererUtils.ts to import TraceContext instead of declaring it locally; ensure TracedHandlerConfig (and any other shared interfaces like TracedHandlerConfig if needed) either import TraceContext or remain local, and run type checks to confirm no changes to usage in functions/classes that reference TraceContext.src/renderer/lib/tracingRendererUtils.ts (1)
62-68: Spreadingattributesinto Sentry tags may include non-primitive values.The
attributesfield is typed asRecord<string, any>, but Sentry tags must be primitive values (string, number, boolean). Ifattributescontains objects or arrays, they will be silently dropped or converted to[object Object].Consider either validating/filtering attributes before spreading, or using
contextsinstead oftagsfor complex values.💡 Proposed fix
Sentry.captureException(error, { tags: { operation: spanName, traced: true, - ...attributes, }, + contexts: { + tracing: { + ...attributes, + }, + }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/lib/tracingRendererUtils.ts` around lines 62 - 68, The Sentry.captureException call in tracingRendererUtils.ts currently spreads attributes (typed Record<string, any>) into tags which requires only primitive values; update the code to sanitize attributes before sending: iterate over attributes in the capture block, separate primitive entries (string/number/boolean) into a sanitizedTags object and collect non-primitive entries into a separate object (e.g., complexAttributes), then call Sentry.captureException(error, { tags: { operation: spanName, traced: true, ...sanitizedTags }, contexts: { attributes: complexAttributes } }) so complex values are sent under contexts while only primitives become tags; reference the Sentry.captureException invocation and the attributes variable to locate where to implement this filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/lib/tracingMainUtils.ts`:
- Around line 17-22: The destructuring of payload in the wrapper returned by the
function can corrupt primitive payloads; add a type guard before destructuring
so you only extract _traceContext when payload is a non-null object (e.g. check
typeof payload === 'object' && payload !== null), otherwise set actualPayload =
payload directly; update the logic around the existing _traceContext check and
the call to handler(event, actualPayload) so primitives pass through unchanged
and only object payloads are split into _traceContext and actualPayload.
---
Nitpick comments:
In `@src/main/actions/tracingPropogation.js`:
- Around line 1-2: The file mixes CommonJS and ES modules: Sentry is imported
via require(" `@sentry/electron/main`") while createTracedHandler is imported with
ES import; make the module system consistent by replacing the CommonJS require
with an ES import for Sentry (i.e. import Sentry from "@sentry/electron/main")
so both Sentry and createTracedHandler use the same import syntax, and ensure
any tooling (babel/tsconfig/packager) supports ES imports in this file.
- Around line 1-17: Rename the file from tracingPropogation.js to
tracingPropagation.js to fix the typo and update all imports that reference it;
ensure the exported symbol withTracing and its usage of createTracedHandler and
Sentry remain unchanged, then run a project-wide search (or adjust module paths)
to replace any occurrences of "tracingPropogation" with "tracingPropagation" so
imports continue to resolve correctly.
In `@src/main/lib/tracingMainUtils.ts`:
- Around line 1-11: TraceContext is duplicated across files; extract it to a
shared types module (e.g., create and export interface TraceContext in a new
shared file) and update both tracingMainUtils.ts and tracingRendererUtils.ts to
import TraceContext instead of declaring it locally; ensure TracedHandlerConfig
(and any other shared interfaces like TracedHandlerConfig if needed) either
import TraceContext or remain local, and run type checks to confirm no changes
to usage in functions/classes that reference TraceContext.
In `@src/renderer/lib/tracingRendererUtils.ts`:
- Around line 62-68: The Sentry.captureException call in tracingRendererUtils.ts
currently spreads attributes (typed Record<string, any>) into tags which
requires only primitive values; update the code to sanitize attributes before
sending: iterate over attributes in the capture block, separate primitive
entries (string/number/boolean) into a sanitizedTags object and collect
non-primitive entries into a separate object (e.g., complexAttributes), then
call Sentry.captureException(error, { tags: { operation: spanName, traced: true,
...sanitizedTags }, contexts: { attributes: complexAttributes } }) so complex
values are sent under contexts while only primitives become tags; reference the
Sentry.captureException invocation and the attributes variable to locate where
to implement this filtering.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/actions/tracingPropogation.jssrc/main/events.jssrc/main/lib/tracingMainUtils.tssrc/renderer/lib/RPCServiceOverIPC.tssrc/renderer/lib/tracingRendererUtils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/events.js
Comment on lines
+17
to
+22
| return async (event: any, payload: any) => { | ||
| const { _traceContext, ...actualPayload } = payload || {}; | ||
|
|
||
| if (!_traceContext || !_traceContext["sentry-trace"]) { | ||
| return handler(event, actualPayload); | ||
| } |
There was a problem hiding this comment.
Destructuring may corrupt primitive payloads.
If payload is a primitive value (string, number, boolean), the object destructuring at line 18 will produce unexpected results:
- String payload:
actualPayloadbecomes{ 0: 'f', 1: 'o', 2: 'o' }(spreads characters) - Number/boolean:
actualPayloadbecomes{}
Consider adding a type guard if primitive payloads are possible.
💡 Proposed fix
return async (event: any, payload: any) => {
+ // Handle non-object payloads (primitives)
+ if (payload === null || payload === undefined || typeof payload !== "object") {
+ return handler(event, payload);
+ }
+
const { _traceContext, ...actualPayload } = payload || {};
if (!_traceContext || !_traceContext["sentry-trace"]) {
return handler(event, actualPayload);
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return async (event: any, payload: any) => { | |
| const { _traceContext, ...actualPayload } = payload || {}; | |
| if (!_traceContext || !_traceContext["sentry-trace"]) { | |
| return handler(event, actualPayload); | |
| } | |
| return async (event: any, payload: any) => { | |
| // Handle non-object payloads (primitives) | |
| if (payload === null || payload === undefined || typeof payload !== "object") { | |
| return handler(event, payload); | |
| } | |
| const { _traceContext, ...actualPayload } = payload || {}; | |
| if (!_traceContext || !_traceContext["sentry-trace"]) { | |
| return handler(event, actualPayload); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/lib/tracingMainUtils.ts` around lines 17 - 22, The destructuring of
payload in the wrapper returned by the function can corrupt primitive payloads;
add a type guard before destructuring so you only extract _traceContext when
payload is a non-null object (e.g. check typeof payload === 'object' && payload
!== null), otherwise set actualPayload = payload directly; update the logic
around the existing _traceContext check and the call to handler(event,
actualPayload) so primitives pass through unchanged and only object payloads are
split into _traceContext and actualPayload.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Chores