fix: instrument both builds of a dual-package provider SDK - #26
Merged
Merged
Conversation
Raises the OpenInference instrumentation floor to the releases carrying the per-class patch guard: instrumentation-anthropic >= 0.2.1, -openai >= 4.2.1. The anthropic bump is the one that matters. npm reads a caret on a 0.x release as patch-only, so the previous "^0.1.20" meant "<0.2.0" and would never have resolved 0.2.1 on its own. Those releases scope OpenInference's double-patch guard to the patched class instead of a module-global boolean (Arize-ai/openinference#3557). openai and @anthropic-ai/sdk ship separate CJS and ESM builds with separate class objects, and the old flag meant whichever build was patched first silently blocked the other. So a CJS require arriving after init() went uninstrumented even though the require hook was there to catch it, and patchActiveBuild had to choose which build to spend the single patch on. It no longer has to. The require hook can now patch a later CJS require on top of the ESM build we patch here, which closes that gap with no change to this function. The test that documented the gap asserted the CJS build stayed unwrapped; it now asserts the opposite, and is what proves the floor in package.json is doing its job — against an older instrumentation it fails. What is still open is narrower and unchanged by this: a hybrid app that has loaded both builds before init() gets the cached CJS copy patched and leaves the ESM one alone. Patching both is now permitted and would close it, at the cost of importing a build the app may never use. Consumers using the anthropic instrumentation must upgrade it alongside this release. The peer is optional, so anyone not using it is unaffected. The 0.2.0 that precedes the fix is additive despite the version jump: beta messages.create coverage, request/response model names, fallback-boundary tracing and finish_reason.
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
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.
Closes step 1 of RIUS-475. Raises the OpenInference instrumentation floor to the releases carrying our upstream fix (Arize-ai/openinference#3558).
@arizeai/openinference-instrumentation-anthropic^0.1.20^0.2.1@arizeai/openinference-instrumentation-openai^4.1.8^4.2.1The anthropic one is the point. npm reads a caret on a
0.xrelease as patch-only, so^0.1.20meant>=0.1.20 <0.2.0and would never have resolved0.2.1on its own. Without this, the fix simply never reaches us.What the upstream release changes
OpenInference guarded
patch()with a module-global_isOpenInferencePatchedboolean.openaiand@anthropic-ai/sdkship separate CJS and ESM builds with separate class objects, so whichever build was patched first silently blocked the other (#3557). The guard is now scoped to the patched class.Two consequences here:
init()is now instrumented. The require hook was always there to catch it; the global flag was what stopped it. It can now patch the CJS build on top of the ESM build we patch inpatchActiveBuild. No logic change was needed for this — the upgrade alone closes it.patchActiveBuildno longer has to spend its single patch carefully, so the trade-off its docstring documented is gone.The test that caught it
tests/instrumentationEsmPatch.test.tscarried a tripwire from when the workaround was written:It failed on the first
npm installafter the bump —expected true to be false. It now asserts the opposite, and it stays as the thing that proves the floor is doing its job: run it against an older instrumentation and it fails.Still open, deliberately
A hybrid app that has loaded both builds before
init()gets the cached CJS copy patched and leaves the ESM one alone. Patching both is now permitted and would close it, at the cost of importing a build the app may never use. That trade-off is worth its own decision, so it stays on RIUS-475 rather than riding along here.Unrelated and unaffected: the residual gap at
src/instrumentation.tsfor the MCP SDK is our owninstrumentMcpClient, not OpenInference's guard.Consumer impact
Anyone using the anthropic instrumentation must upgrade it alongside this release. The peer is
optional, so anyone not using it is unaffected.The
0.2.0that precedes the fix is additive despite the version jump (changesets'0.xconvention makes aminorlook like a major):beta.messages.createcoverage,llm.request.model_name/llm.response.model_name, fallback-boundary tracing, andllm.finish_reason.openai@4.2.0widens support toopenai@^7, which our devDependency already assumed.Expect new attributes to start appearing on Anthropic spans.
Verification
224 passed(24 files), biome clean,tsc --noEmitclean,tsupbuild clean. Resolved versions confirmed:Note on the release type
Committed as
fix:. It raises an optional peer floor, which is arguably breaking for consumers pinned below it — retitle tofeat!:before merge if you would rather release-please surface that as a version bump.