Add mastra auto-instrumentation#1901
Conversation
Stephen Belanger (Qard)
requested review from
Abhijeet Prasad (AbhiPrasad) and
Luca Forstner (lforst)
Luca Forstner (lforst)
left a comment
Luca Forstner (lforst)
left a comment
There was a problem hiding this comment.
Can we also add the mastra e2e test scenario to the ci summary?
Stephen Belanger (Qard)
force-pushed
the
mastra-auto-instrumentation
branch
from
37f8aaa to
43c826c
Compare
Stephen Belanger (Qard)
force-pushed
the
mastra-auto-instrumentation
branch
from
443f70c to
8d58469
Compare
Stephen Belanger (Qard)
force-pushed
the
mastra-auto-instrumentation
branch
from
c01e4fd to
ffcc4ea
Compare
Stephen Belanger (Qard)
force-pushed
the
mastra-auto-instrumentation
branch
3 times, most recently
from
b59c93e to
282b413
Compare
…iterals (#2036) ## Summary - `areInterfaceSignaturesCompatible` was flagging adding an optional property to an inline object literal type (e.g. `mastra?: boolean` on `InstrumentationConfig.integrations`) as a breaking change. - Root cause: it compared each property's *type* as a string, then only re-checked via `isUnionTypeWidening` when the strings differed. Inline object literals aren't unions, so any new field tipped it into "modified" territory. - Fix: delegate to the existing `areObjectTypeDefinitionsCompatible` helper when both sides are inline object literals, mirroring the pattern already in `areTypeAliasSignaturesCompatible`. Also apply the same delegation inside `areObjectTypeDefinitionsCompatible` itself so nested object literals are handled the same way. - Lifted the inline `isObjectType` helper to a module-scope `isObjectLiteralType` so the interface, object-literal, and intersection comparators share one definition. After this lands, the `js-api-compatibility (20)` check on PRs that add optional integration keys (#1891, #1897, #1901, future SDK integrations) will pass instead of failing informationally. ## Test plan - [x] Added 4 regression tests in `describe("areInterfaceSignaturesCompatible", ...)`: - optional property added to inline object literal (the `InstrumentationConfig` case) → passes - required property added → still rejected - property removed → still rejected - optional property added to deeply nested inline object literal → passes - [x] `pnpm exec vitest run tests/api-compatibility/api-compatibility.test.ts` — all 14 interface-compat tests pass plus the rest of the file - [x] `pnpm run fix:formatting` and `pnpm run lint` clean #skip-changeset — this is a test-only change with no impact on the published package. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Stephen Belanger (Qard)
force-pushed
the
mastra-auto-instrumentation
branch
2 times, most recently
from
f7f5fbe to
ea0197d
Compare
|
Luca Forstner (@lforst) Getting some provider limit failures, but otherwise should be good for another review. |
| @@ -0,0 +1,268 @@ | |||
| /** | |||
There was a problem hiding this comment.
This is the most important feedback.
This goes more into the direction I find good. However, I find this approach still quite sketchy. I would find this acceptable if we were in a hurry to get this out asap, which is not the case.
I honestly think we should invest the time to use require-in-the-middle and import-in-the-middle whenever we are targeting top-level-exports (ie. stable API) which seems to be the case here. That way we don't need sketchy custom one-off instrumentation code for all packages.
There was a problem hiding this comment.
IITM would be substantially worse. It's very unreliable and has been known-broken with half the major AI SDKs for a while now because there are a bunch of design issues which are simply not addressable via basic ESM loaders wrapping. I went with orchestrion in the first place because most of the modules we instrument would crash IITM. As for RITM, it's not particularly useful at this point either given many of the AI SDKs are ESM-only now.
There's talk of rewriting the two from scratch on the newer sync loader hooks API, but no one has really been putting any work into that. Unless we want to take on rewriting RITM + IITM ourselves, I don't think there's a viable path there yet.
There was a problem hiding this comment.
IITM would be substantially worse.
Can why it would be substantially worse for this particular case?
It's very unreliable
From experience, IITM only has problems when there are actual problems. I have not seen it be unreliable. Can you elaborate what you mean by it being unreliable?
Its only downside that I know of you have already mentioned, which is that it does not work for very specific packages if they export things in a certain dodgy way.
Right now looking at the PR, probabilistically extracting a chunk seems more unreliable (ie. less future-proof) than using IITM to patch public API (which tends to be stable). To me, unless IITM/RITM are broken for Mastra, it seems like the best tool for the job.
Stephen Belanger (Qard)
force-pushed
the
mastra-auto-instrumentation
branch
2 times, most recently
from
b4ac2fd to
4a5d3fb
Compare
Replaces the chunk-AST instrumentation with a Braintrust
ObservabilityExporter that the loader auto-installs into every
`new Mastra(...)`. Survives Mastra's content-hashed chunk renames
release-to-release because the loader only patches the stable
`dist/index.{js,cjs}` entrypoints — chunk paths are read out of the
original source at load time, not pinned in a version table.
Two integration paths:
- Auto: under `node --import braintrust/hook.mjs`, the loader rewrites
Mastra's entry into a Proxy-wrapped class that auto-constructs an
`Observability` instance if none was provided, and appends a Proxy
wrap to `@mastra/observability`'s entry so the constructor injects
our exporter into every config.
- Manual: `import { BraintrustObservabilityExporter } from "braintrust";`
and pass it to `new Mastra({ observability: new Observability({...}) })`.
Requires `@mastra/core >= 1.20.0` for the auto path; manual integration
works on any Mastra version that accepts an `ObservabilityExporter`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Stephen Belanger (Qard)
force-pushed
the
mastra-auto-instrumentation
branch
from
4a5d3fb to
7068c6a
Compare
2 participants
Fixes #1890
Summary
Testing
pnpm --dir js exec vitest run src/auto-instrumentations/configs/mastra.test.ts src/instrumentation/plugins/mastra-plugin.test.ts src/instrumentation/registry.test.tspnpm run test:e2e -- mastra-instrumentationNotes
pnpm --dir js test -- mastraandpnpm --dir js test -- auto-instrumentationsstill hit unrelated existing network-dependent failures insrc/framework.test.tsandsrc/logger-misc.test.tspnpm run formattingstill reports pre-existing unformatted fixture files underjs/tests/auto-instrumentations/fixtures/test-files/