chore: Add spans to rts #39848
chore: Add spans to rts #39848
Conversation
WalkthroughThe pull request introduces tracing functionality in the Changes
Sequence Diagram(s)sequenceDiagram
participant C as DSLController
participant T as Tracer
participant M as Migration Logic
participant Client
C->>T: startSpan("dsl-migration")
C->>M: Execute migration process
alt If error occurs
C->>T: endSpan(span, error)
C->>Client: Send error response
else On success
C->>T: endSpan(span)
C->>Client: Send successful response
end
sequenceDiagram
participant C as DSLController
participant T as Tracer
participant V as Version Checker
participant Client
C->>T: startSpan("get-latest-dsl-version")
C->>T: startSpan("version-check", parent=span)
C->>V: Execute version check
alt If error occurs
C->>T: endSpan(childSpan, error)
C->>T: endSpan(parentSpan, error)
C->>Client: Send error response
else On success
C->>T: endSpan(childSpan)
C->>T: endSpan(parentSpan)
C->>Client: Send successful response
end
Suggested labels
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/client/packages/rts/src/controllers/Dsl/DslController.ts (2)
5-5: Update import path to use alias patternThe import for tracing utilities uses a relative path while other imports in this file follow an alias pattern (e.g.,
@controllers). Consider updating to use a consistent import pattern for better maintainability.-import { startSpan, endSpan } from "../../utils/tracing"; +import { startSpan, endSpan } from "@utils/tracing";
8-10: Remove unnecessary constructorThis constructor only calls the parent constructor with no additional logic, making it redundant.
- constructor() { - super(); - }🧰 Tools
🪛 Biome (1.9.4)
[error] 8-10: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
app/client/packages/rts/src/utils/tracing.ts (2)
30-43: Consider adding support for recording additional error detailsThe error handling looks good, but consider enhancing it to record more error context when available.
export function endSpan(span: Span, error?: Error): void { if (error) { span.setStatus({ code: SpanStatusCode.ERROR, message: error.message, }); + // Record additional error details if available + span.setAttribute('error.type', error.name || 'Error'); + if (error.stack) { + span.setAttribute('error.stack', error.stack); + } } span.end(); }
1-43: Consider adding a configurable tracer initialization functionThe current implementation initializes the tracer directly, which works but offers limited configurability. Consider adding a function to initialize the tracer with configuration options.
+import { trace, context, SpanStatusCode, TracerOptions } from '@opentelemetry/api'; import type { Span } from '@opentelemetry/api'; -const tracer = trace.getTracer('rts-tracer'); +let tracer = trace.getTracer('rts-tracer'); + +/** + * Initialize the OpenTelemetry tracer with custom options + * @param name Name of the tracer + * @param options Optional tracer options + */ +export function initTracer(name: string = 'rts-tracer', options?: TracerOptions): void { + tracer = trace.getTracer(name, options); +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/packages/rts/src/controllers/Dsl/DslController.ts(1 hunks)app/client/packages/rts/src/utils/tracing.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/rts/src/controllers/Dsl/DslController.ts
[error] 8-10: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (2)
app/client/packages/rts/src/utils/tracing.ts (2)
1-5: LGTM: Good import and tracer initializationThe OpenTelemetry imports and tracer initialization look appropriate.
6-28: Well-documented and implemented startSpan functionThe implementation is clean and the JSDoc comments are thorough. The function properly handles parent spans and attributes.
| getLatestDSLVersion(req: Request, res: Response) { | ||
| const span = startSpan("get-latest-dsl-version"); | ||
| const childSpan = startSpan("version-check", {}, span); | ||
|
|
||
| try { | ||
| super.sendResponse(res, { version: latestDSLVersion }); | ||
| } catch (err) { | ||
| endSpan(childSpan, err); | ||
| endSpan(span, err); | ||
| return super.sendError( | ||
| res, | ||
| this.serverErrorMessage, | ||
| [err.message], | ||
| StatusCodes.INTERNAL_SERVER_ERROR, | ||
| ); | ||
| } | ||
| endSpan(childSpan); | ||
| endSpan(span); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply try-finally pattern to ensure spans are closed
Similar to the previous method, use a try-finally pattern to guarantee spans are always closed even if super.sendResponse throws an exception.
getLatestDSLVersion(req: Request, res: Response) {
const span = startSpan("get-latest-dsl-version");
const childSpan = startSpan("version-check", {}, span);
try {
super.sendResponse(res, { version: latestDSLVersion });
} catch (err) {
endSpan(childSpan, err);
endSpan(span, err);
return super.sendError(
res,
this.serverErrorMessage,
[err.message],
StatusCodes.INTERNAL_SERVER_ERROR,
);
+ } finally {
+ if (childSpan.isRecording()) endSpan(childSpan);
+ if (span.isRecording()) endSpan(span);
}
- endSpan(childSpan);
- endSpan(span);
}📝 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.
| getLatestDSLVersion(req: Request, res: Response) { | |
| const span = startSpan("get-latest-dsl-version"); | |
| const childSpan = startSpan("version-check", {}, span); | |
| try { | |
| super.sendResponse(res, { version: latestDSLVersion }); | |
| } catch (err) { | |
| endSpan(childSpan, err); | |
| endSpan(span, err); | |
| return super.sendError( | |
| res, | |
| this.serverErrorMessage, | |
| [err.message], | |
| StatusCodes.INTERNAL_SERVER_ERROR, | |
| ); | |
| } | |
| endSpan(childSpan); | |
| endSpan(span); | |
| } | |
| getLatestDSLVersion(req: Request, res: Response) { | |
| const span = startSpan("get-latest-dsl-version"); | |
| const childSpan = startSpan("version-check", {}, span); | |
| try { | |
| super.sendResponse(res, { version: latestDSLVersion }); | |
| } catch (err) { | |
| endSpan(childSpan, err); | |
| endSpan(span, err); | |
| return super.sendError( | |
| res, | |
| this.serverErrorMessage, | |
| [err.message], | |
| StatusCodes.INTERNAL_SERVER_ERROR, | |
| ); | |
| } finally { | |
| if (childSpan.isRecording()) endSpan(childSpan); | |
| if (span.isRecording()) endSpan(span); | |
| } | |
| } |
rajatagrawal
force-pushed
the
chore/add-span-to-rts
branch
from
87b123a to
470973c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/client/packages/rts/src/controllers/Dsl/DslController.ts (2)
12-28: 🛠️ Refactor suggestionUse try-finally pattern to ensure spans are properly ended
The current implementation may leave the span open if
super.sendResponsethrows an exception. Use a try-finally pattern to ensure the span is always ended.async migrateDSL(req: Request, res: Response) { const span = startSpan("dsl-migration"); try { const latestDSL = await migrateDSLToLatest(req.body); super.sendResponse(res, latestDSL); } catch (err) { endSpan(span, err); return super.sendError( res, this.serverErrorMessage, [err.message], StatusCodes.INTERNAL_SERVER_ERROR, ); + } finally { + if (!span.isRecording()) return; + endSpan(span); } - endSpan(span); }
30-48: 🛠️ Refactor suggestionApply try-finally pattern to ensure spans are closed
Similar to the previous method, use a try-finally pattern to guarantee spans are always closed even if
super.sendResponsethrows an exception.getLatestDSLVersion(req: Request, res: Response) { const span = startSpan("get-latest-dsl-version"); const childSpan = startSpan("version-check", {}, span); try { super.sendResponse(res, { version: latestDSLVersion }); } catch (err) { endSpan(childSpan, err); endSpan(span, err); return super.sendError( res, this.serverErrorMessage, [err.message], StatusCodes.INTERNAL_SERVER_ERROR, ); + } finally { + if (childSpan.isRecording()) endSpan(childSpan); + if (span.isRecording()) endSpan(span); } - endSpan(childSpan); - endSpan(span); }
🧹 Nitpick comments (3)
app/client/packages/rts/src/controllers/Dsl/DslController.ts (3)
5-5: Verify the relative import path for the tracing utilityThe import statement uses a relative path (
../../utils/tracing) which could be simplified to use the aliased import pattern like the other imports.-import { startSpan, endSpan } from "../../utils/tracing"; +import { startSpan, endSpan } from "@utils/tracing";
8-10: Remove unnecessary constructorThis constructor doesn't perform any initialization beyond calling the parent constructor and can be safely removed.
- constructor() { - super(); - }🧰 Tools
🪛 Biome (1.9.4)
[error] 8-10: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
32-32: Consider adding descriptive attributes to the version-check spanThe second parameter in
startSpanis for attributes, but it's passed as an empty object. Add relevant attributes to provide more context to the span.- const childSpan = startSpan("version-check", {}, span); + const childSpan = startSpan("version-check", { dslVersion: latestDSLVersion }, span);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
app/client/packages/rts/src/controllers/Dsl/DslController.ts(1 hunks)app/client/packages/rts/src/utils/tracing.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/rts/src/utils/tracing.ts
🧰 Additional context used
🧬 Code Definitions (1)
app/client/packages/rts/src/controllers/Dsl/DslController.ts (1)
app/client/packages/rts/src/utils/tracing.ts (2) (2)
startSpan(13-30)endSpan(37-45)
🪛 Biome (1.9.4)
app/client/packages/rts/src/controllers/Dsl/DslController.ts
[error] 8-10: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/client/packages/rts/src/controllers/Dsl/DslController.ts (2)
13-14: 🛠️ Refactor suggestionUse try-finally pattern to ensure spans are properly closed
The current implementation may leave the span open if
super.sendResponsethrows an exception. Use a try-finally pattern to ensure the span is always ended.async migrateDSL(req: Request, res: Response) { const span = startSpan("dsl-migration"); try { const latestDSL = await migrateDSLToLatest(req.body); super.sendResponse(res, latestDSL); } catch (err) { endSpan(span, err); return super.sendError( res, this.serverErrorMessage, [err.message], StatusCodes.INTERNAL_SERVER_ERROR, ); + } finally { + if (span.isRecording()) endSpan(span); } - endSpan(span); }Also applies to: 20-21, 29-29
33-35: 🛠️ Refactor suggestionApply try-finally pattern to ensure spans are closed
Similar to the
migrateDSLmethod, the current implementation may leave spans open ifsuper.sendResponsethrows an exception. Use a try-finally pattern to guarantee spans are always closed.getLatestDSLVersion(req: Request, res: Response) { const span = startSpan("get-latest-dsl-version"); const childSpan = startSpan("version-check", {}, span); try { super.sendResponse(res, { version: latestDSLVersion }); } catch (err) { endSpan(childSpan, err); endSpan(span, err); return super.sendError( res, this.serverErrorMessage, [err.message], StatusCodes.INTERNAL_SERVER_ERROR, ); + } finally { + if (childSpan.isRecording()) endSpan(childSpan); + if (span.isRecording()) endSpan(span); } - endSpan(childSpan); - endSpan(span); }Also applies to: 39-41, 49-50
🧹 Nitpick comments (1)
app/client/packages/rts/src/controllers/Dsl/DslController.ts (1)
8-10: Remove unnecessary constructorThe constructor doesn't add any functionality beyond what the parent class constructor provides.
- constructor() { - super(); - }🧰 Tools
🪛 Biome (1.9.4)
[error] 8-10: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
app/client/packages/rts/src/controllers/Dsl/DslController.ts(1 hunks)app/client/packages/rts/src/utils/tracing.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/rts/src/utils/tracing.ts
🧰 Additional context used
🧬 Code Definitions (1)
app/client/packages/rts/src/controllers/Dsl/DslController.ts (1)
app/client/packages/rts/src/utils/tracing.ts (2) (2)
startSpan(13-30)endSpan(37-46)
🪛 Biome (1.9.4)
app/client/packages/rts/src/controllers/Dsl/DslController.ts
[error] 8-10: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (1)
app/client/packages/rts/src/controllers/Dsl/DslController.ts (1)
5-5: Proper import for tracing utilities addedThe import statement correctly brings in the necessary tracing utilities from the defined path.
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 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=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD 9c7cd35 yet
Fri, 21 Mar 2025 11:06:06 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit