[otel] - Adding sentry operation attribute #1120
Conversation
…n the otel providers
|
@TheYoxy is attempting to deploy a commit to the unnoq-team Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis PR adds Sentry context detection and OpenTelemetry span annotation throughout the oRPC stack. A new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Span as OTEL Span
participant Sentry as Sentry Context
participant Op as Operation
App->>Span: runWithSpan(callback)
Span->>Sentry: isInSentryContext()?
alt In Sentry Context
Sentry-->>Span: true
Span->>Span: span.setAttribute("sentry.op", "orpc")
else Not in Sentry Context
Sentry-->>Span: false
note over Span: No sentry.op set
end
Span->>Op: Execute callback with span
Op-->>Span: Complete
Span-->>App: Return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes follow a consistent repetitive pattern across multiple files (import function, add conditional check, set span attribute), reducing cognitive load. However, the distributed nature across 9+ files and multiple layers (adapters, handlers, procedures, transport) requires systematic review to ensure the guard is correctly applied in each context. Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Summary of ChangesHello @TheYoxy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request aims to improve the observability of Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
dosubot
bot
added
the
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly adds the sentry.op attribute to OpenTelemetry spans when Sentry is detected. The implementation of isInSentryContext and its tests are well done. However, there is significant code duplication across multiple files where the attribute is set. I've suggested extracting this logic into a new helper function setSentryOpAttribute in packages/shared/src/otel.ts and using it throughout the codebase. This will improve maintainability and centralize the Sentry-specific logic.
| import type { StandardLinkPlugin } from './plugin' | ||
| import type { StandardLinkClient, StandardLinkCodec } from './types' | ||
| import { asyncIteratorWithSpan, getGlobalOtelConfig, intercept, isAsyncIteratorObject, ORPC_NAME, runWithSpan, toArray } from '@orpc/shared' | ||
| import { asyncIteratorWithSpan, getGlobalOtelConfig, intercept, isAsyncIteratorObject, isInSentryContext, ORPC_NAME, runWithSpan, toArray } from '@orpc/shared' |
There was a problem hiding this comment.
To avoid code duplication, I've suggested adding a setSentryOpAttribute helper. Please import it here instead of isInSentryContext.
| import { asyncIteratorWithSpan, getGlobalOtelConfig, intercept, isAsyncIteratorObject, isInSentryContext, ORPC_NAME, runWithSpan, toArray } from '@orpc/shared' | |
| import { asyncIteratorWithSpan, getGlobalOtelConfig, intercept, isAsyncIteratorObject, setSentryOpAttribute, ORPC_NAME, runWithSpan, toArray } from '@orpc/shared' |
| if (isInSentryContext()) { | ||
| span?.setAttribute('sentry.op', 'orpc') | ||
| } |
There was a problem hiding this comment.
Please use the setSentryOpAttribute helper function to reduce code duplication.
setSentryOpAttribute(span)| if (isInSentryContext()) { | ||
| span?.setAttribute('sentry.op', 'orpc') | ||
| } |
There was a problem hiding this comment.
Please use the setSentryOpAttribute helper function to reduce code duplication.
setSentryOpAttribute(span)| if (isInSentryContext()) { | ||
| span?.setAttribute('sentry.op', 'orpc') | ||
| } |
There was a problem hiding this comment.
Please use the setSentryOpAttribute helper function to reduce code duplication.
setSentryOpAttribute(span)| if (isInSentryContext()) { | ||
| span?.setAttribute('sentry.op', 'orpc') | ||
| } |
There was a problem hiding this comment.
Please use the setSentryOpAttribute helper function to reduce code duplication.
setSentryOpAttribute(span)| if (isInSentryContext()) { | ||
| span?.setAttribute('sentry.op', 'orpc') | ||
| } |
There was a problem hiding this comment.
Please use the setSentryOpAttribute helper function to reduce code duplication.
setSentryOpAttribute(span)| import type { EventIteratorPayload } from './codec' | ||
| import type { EncodedMessage, EncodedMessageSendFn } from './types' | ||
| import { AbortError, AsyncIdQueue, getGlobalOtelConfig, isAsyncIteratorObject, runWithSpan } from '@orpc/shared' | ||
| import { AbortError, AsyncIdQueue, getGlobalOtelConfig, isAsyncIteratorObject, isInSentryContext, runWithSpan } from '@orpc/shared' |
There was a problem hiding this comment.
To avoid code duplication, I've suggested adding a setSentryOpAttribute helper. Please import it here instead of isInSentryContext.
| import { AbortError, AsyncIdQueue, getGlobalOtelConfig, isAsyncIteratorObject, isInSentryContext, runWithSpan } from '@orpc/shared' | |
| import { AbortError, AsyncIdQueue, getGlobalOtelConfig, isAsyncIteratorObject, setSentryOpAttribute, runWithSpan } from '@orpc/shared' |
| if (isInSentryContext()) { | ||
| span?.setAttribute('sentry.op', 'orpc') | ||
| } |
There was a problem hiding this comment.
Please use the setSentryOpAttribute helper function to reduce code duplication.
setSentryOpAttribute(span)| if (isInSentryContext()) { | ||
| span?.setAttribute('sentry.op', 'orpc') | ||
| } |
There was a problem hiding this comment.
Please use the setSentryOpAttribute helper function to reduce code duplication.
setSentryOpAttribute(span)| if (isInSentryContext()) { | ||
| span?.setAttribute('sentry.op', 'orpc') | ||
| } |
There was a problem hiding this comment.
Please use the setSentryOpAttribute helper function to reduce code duplication.
setSentryOpAttribute(span)![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 (2)
packages/standard-server-peer/src/event-iterator.ts (1)
7-7: Minor formatting adjustment.The space inside the empty braces is a stylistic change with no functional impact. Typically handled by code formatters.
packages/server/src/adapters/standard/handler.ts (1)
86-88: Consider extracting the Sentry span annotation pattern.The same conditional block appears three times in this file:
if (isInSentryContext()) { span?.setAttribute('sentry.op', 'orpc') }You could extract this into a helper function to reduce duplication:
function setSentryOpIfInContext(span: Span | undefined): void { if (isInSentryContext()) { span?.setAttribute('sentry.op', 'orpc') } }Then use:
setSentryOpIfInContext(span)However, since this pattern is applied consistently across multiple modules according to the PR summary, the current approach prioritizes clarity and local reasoning over abstraction.
Also applies to: 105-110, 127-132
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/client/src/adapters/standard/link.ts(5 hunks)packages/server/src/adapters/standard/handler.ts(5 hunks)packages/server/src/plugins/batch.ts(2 hunks)packages/server/src/procedure-client.ts(6 hunks)packages/shared/src/otel.test.ts(2 hunks)packages/shared/src/otel.ts(2 hunks)packages/standard-server-fetch/src/body.ts(2 hunks)packages/standard-server-peer/src/client.ts(2 hunks)packages/standard-server-peer/src/event-iterator.ts(2 hunks)packages/standard-server-peer/src/server.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
packages/server/src/plugins/batch.ts (1)
packages/shared/src/otel.ts (1)
isInSentryContext(206-209)
packages/server/src/procedure-client.ts (1)
packages/shared/src/otel.ts (1)
isInSentryContext(206-209)
packages/standard-server-peer/src/event-iterator.ts (1)
packages/shared/src/otel.ts (2)
SetSpanErrorOptions(56-61)isInSentryContext(206-209)
packages/client/src/adapters/standard/link.ts (1)
packages/shared/src/otel.ts (2)
isInSentryContext(206-209)runWithSpan(155-184)
packages/server/src/adapters/standard/handler.ts (2)
packages/shared/src/otel.ts (1)
isInSentryContext(206-209)packages/standard-server-peer/src/client.ts (1)
request(73-179)
packages/standard-server-peer/src/client.ts (1)
packages/shared/src/otel.ts (1)
isInSentryContext(206-209)
packages/standard-server-peer/src/server.ts (1)
packages/shared/src/otel.ts (2)
isInSentryContext(206-209)runWithSpan(155-184)
packages/standard-server-fetch/src/body.ts (2)
packages/standard-server-node/src/body.ts (2)
ToStandardBodyOptions(10-10)toStandardBody(12-51)packages/shared/src/otel.ts (2)
runWithSpan(155-184)isInSentryContext(206-209)
packages/shared/src/otel.test.ts (1)
packages/shared/src/otel.ts (2)
setGlobalOtelConfig(34-36)isInSentryContext(206-209)
🔇 Additional comments (16)
packages/shared/src/otel.ts (2)
10-15: LGTM: Symbol definition aligns with Sentry conventions.The use of
Symbol.for('sentry_scopes')correctly creates a global symbol that matches Sentry's internal implementation, enabling proper detection of Sentry context across module boundaries.
203-209: LGTM: Clean implementation with safe optional chaining.The function correctly detects Sentry context by checking for the presence of the Sentry scope symbol in the active OpenTelemetry context. The optional chaining ensures graceful handling when OTEL is not configured.
packages/server/src/plugins/batch.ts (1)
82-85: LGTM: Sentry context detection properly integrated.The conditional span annotation is correctly placed at the start of the batch request handling span, ensuring Sentry operation metadata is available when operating within a Sentry context.
packages/server/src/procedure-client.ts (1)
114-116: LGTM: Comprehensive span annotation across procedure lifecycle.The Sentry context check is systematically applied to all critical spans in the procedure execution flow (call_procedure, validate_input, validate_output, middleware, and handler), ensuring consistent observability when integrated with Sentry.
packages/standard-server-peer/src/client.ts (1)
78-81: LGTM: Consistent Sentry context handling in peer client.The span annotation follows the established pattern. The callback is correctly marked as
asyncto accommodate the attribute setting logic.packages/standard-server-peer/src/server.ts (1)
110-113: LGTM: Proper span annotation in peer server flow.The Sentry context detection is correctly applied across all peer server spans (receive, handle, and send), maintaining consistency with the broader instrumentation pattern.
packages/standard-server-fetch/src/body.ts (1)
15-18: LGTM: Span parameter correctly utilized for Sentry context.The callback signature properly updated to accept the span parameter, enabling conditional annotation based on Sentry context detection.
packages/shared/src/otel.test.ts (1)
365-394: LGTM: Comprehensive test coverage for Sentry context detection.The test suite properly validates all critical scenarios: symbol presence (true case), missing global config (false case), and symbol absence in active context (false case). The test implementation correctly uses
Symbol.for('sentry_scopes')to match the production code.packages/client/src/adapters/standard/link.ts (2)
53-55: LGTM: Root span and nested spans properly annotated.The Sentry context check is correctly integrated at the root RPC span level and propagated through encode, send, and decode operations. The implementation aligns with OpenTelemetry semantic conventions for RPC spans.
65-68: Good documentation of context management strategy.The updated comment clearly explains the need for manual context management in browser environments where the OpenTelemetry context manager may not work reliably with async functions.
packages/standard-server-peer/src/event-iterator.ts (2)
4-4: LGTM - Import addition is correct.The
isInSentryContextimport is properly added and used in the implementation below.
98-100: LGTM - Sentry context detection and span annotation.The implementation correctly checks for Sentry context and sets the
sentry.opattribute when appropriate. This follows the established pattern used across other modules in the PR.packages/server/src/adapters/standard/handler.ts (4)
11-11: LGTM - Import addition is correct.The
isInSentryContextimport is properly added and used throughout the file.
86-88: LGTM - Sentry span annotation in top-level handler.The Sentry context check and span attribute setting is correctly implemented for the top-level request span.
105-110: LGTM - Correct refactoring to callback pattern.The change from a direct call to a callback pattern is correct for
runWithSpan. The Sentry context check is properly integrated.
127-132: LGTM - Correct refactoring to callback pattern.The refactoring matches the pattern used for the
find_procedurespan and correctly integrates the Sentry context check.
|
OpenTelemetry already distinguishes spans using the package name: https://github.com/unnoq/orpc/blob/main/packages/otel/src/instrumentation.ts#L20 |
More templates
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/experimental-durable-iterator
@orpc/hey-api
@orpc/interop
@orpc/json-schema
@orpc/nest
@orpc/openapi
@orpc/openapi-client
@orpc/otel
@orpc/experimental-publisher
@orpc/react
@orpc/react-query
@orpc/experimental-react-swr
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-aws-lambda
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
2 participants
This adds an attribute to every span created by orpc, which describes the operation when sentry is detected in the opentelemetry context.
It search in the opentelemetry active context, for the symbol
sentry_scopeswhich is added by default when callingSentry.init().Reference of this symbol
The operation is using an attribute,
sentry.opwhich is defined here. This attribute describes the operation type and will be displayed directly on the sentry dashboard asorpc.Here is a preview of what it should look likes when sentry is detected:
Note
I haven't added any specific playground to play with it since it needs to setup a sentry instance with accounts that must be shared with anyone who wants to try it.
Summary by CodeRabbit