Uh oh!
There was an error while loading. Please reload this page.
feat(server-utils): Rewrite SentryLangGraphInstrumentation to orchestrion - #22268
Conversation
size-limit report 📦
|
Uh oh!
There was an error while loading. Please reload this page.
| * injected into `@langchain/langgraph`'s `StateGraph.compile` and `createReactAgent`, so it requires | ||
| * the orchestrion runtime hook or bundler plugin. | ||
| */ | ||
| export const langGraphChannelIntegration = defineIntegration(_langGraphChannelIntegration); |
There was a problem hiding this comment.
Missing feat integration tests
Medium Severity
This is a feat PR that adds a new orchestrion diagnostics-channel LangGraph integration, but the diff includes no integration or E2E test covering the new channel path (StateGraph.compile / createReactAgent subscribe + invoke wrapping). Existing OTel suite coverage does not validate this rewrite. Flagged because the review rules require at least one integration or E2E test on feat PRs.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 5b8c0af. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 90d1118. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
| // `createReactAgent` compiles a `StateGraph` internally; suppress the `create_agent` span for that | ||
| // nested compile so a react agent gets a single `invoke_agent` span, matching the OTel path. | ||
| let insideCreateReactAgent = false; |
There was a problem hiding this comment.
Bug: The module-level insideCreateReactAgent flag is not concurrency-safe, leading to race conditions where one request's state can cause incorrect span suppression in another unrelated request.
Severity: HIGH
Suggested Fix
Replace the module-level insideCreateReactAgent flag with a request-scoped state management solution, such as AsyncLocalStorage. This will ensure that the flag's state is isolated to the specific asynchronous context of each request, preventing interference between concurrent operations.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/server-utils/src/integrations/tracing-channel/langgraph.ts#L39
Potential issue: The `insideCreateReactAgent` flag, declared as a module-level
singleton, is modified by event subscribers for `reactAgentChannel`. In a concurrent
environment like a Node.js server, this creates a race condition. If one request calls
`createReactAgent` and sets the flag, a simultaneous, unrelated request to
`StateGraph.compile` will read the globally set flag and incorrectly suppress its
`create_agent` span. This happens because the event callbacks operate in a shared module
context, unlike the OTel path which uses a synchronous `try/finally` block to scope the
flag's state.
Also affects:
packages/server-utils/src/integrations/tracing-channel/langgraph.ts:63~63packages/server-utils/src/integrations/tracing-channel/langgraph.ts:99~99packages/server-utils/src/integrations/tracing-channel/langgraph.ts:102~102packages/server-utils/src/integrations/tracing-channel/langgraph.ts:110~110
Did we get this right? 👍 / 👎 to inform future reviews.
| // `createReactAgent` compiles a `StateGraph` internally; suppress the `create_agent` span for that | ||
| // nested compile so a react agent gets a single `invoke_agent` span, matching the OTel path. | ||
| let insideCreateReactAgent = false; |
There was a problem hiding this comment.
m: afaik create_agent spans are no longer relevant for us, so that this is still emitted in the langgraph instrumentation seems like a leftover. so we could think about simplifying all of this? or if we want to keep parity for now that's also fine I think we should just add a task for the major to get rid of this I think
There was a problem hiding this comment.
So is it safe to drop entirely? If it already gets dropped somewhere along the line then we should simplify sure. Otherwise, I can create an issue and park a PR for v11 for it.
There was a problem hiding this comment.
The OTel path still emits it but we will drop it eventually just didn't have the time yet, but I think it's fine to park this for v11 then we can align this more broadly.
Rewrites the LangGraph integration to a diagnostics-channel listener instead of `InstrumentationBase`, with orchestrion injecting the channels into `@langchain/langgraph`'s `StateGraph.compile` and `createReactAgent`. The subscriber creates the `create_agent` span around `compile`, wraps the returned compiled graph's `invoke` with the shared `invoke_agent` instrumentation, and wraps react-agent tools, reusing the existing core span builders so span output is identical. The OTel path stays as the fallback when orchestrion isn't injected.
- Set the createReactAgent suppression flag only after wrapping tools, so a throw there can't leave it stuck on and permanently suppress create_agent spans. - Route instrumentStateGraphCompile through _INTERNAL_getLangGraphCreateAgentSpanOptions so the create_agent span options have a single source of truth.
07e668b to
1522c6fCompare
Rewrites the LangGraph integration to a
node:diagnostics_channellistener instead ofInstrumentationBase, with orchestrion injecting the channels. The vendored OTel path stays as the fallback when orchestrion isn't injected.closes#20916