feat(cloudflare): Instrument Agents automatically - #22727
Conversation
JPeer264
changed the title
size-limit report 📦
|
What do you mean we run with v18 locally? Why can't we bump now that we're on v11? |
Because I need to backport that PR. We can change that anytime now ofc |
cc88548 to
62f5ca2
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
62f5ca2 to
8d6a511
Compare
| // its base-class chain tells the two apart. Detection walks the module graph (base classes | ||
| // usually live in their own file), so it is limited to the configured DO classes. |
There was a problem hiding this comment.
q: Does that constraint really hold true? I wouldn't be surprised if someone has several classes in one file. Or am I misunderstanding?
There was a problem hiding this comment.
By definition all the classes are in one file, it is just the question if they are defined in the main entrypoint or in other files.
| const code = stripComments(rawCode); | ||
| const shape = emptyShape(); | ||
|
|
||
| // `class X extends Y {` / `class X<T> extends Y<T> {`, optionally exported and/or abstract. |
There was a problem hiding this comment.
What about cases where generics extend, e.g. class X<T extends S> extends Y<T extends Z>?
There was a problem hiding this comment.
True. I'm sure there are more edge cases which we didn't think of just yet.
Added in facc163
| const CLASS_RE = /\b(?:export\s+(?:default\s+)?)?(?:abstract\s+)?class\s+([A-Za-z_$][\w$]*)([^{]*)\{/g; | ||
| for (const match of code.matchAll(CLASS_RE)) { |
There was a problem hiding this comment.
Bug: The CLASS_RE regex in shapeFromSource() fails to detect anonymous classes like export default class extends Agent {}, causing agent detection to fail for those modules.
Severity: MEDIUM
Suggested Fix
Modify the CLASS_RE regex to make the class name capture group optional. This will allow it to correctly match and parse anonymous default exported classes.
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/cloudflare/src/vite/moduleShape.ts#L213-L214
Potential issue: The regex `CLASS_RE` used in `shapeFromSource()` to parse module shapes
requires a class name to be present after the `class` keyword. This means it will not
match an anonymous default export, such as `export default class extends Agent {}`. When
this pattern is used in a module, `defaultExportIsClass` is not set on the module's
shape. Consequently, when `isAgentBinding()` later checks the module, it incorrectly
determines that the default export is not an agent, leading to a failure in agent
detection. The code seems to anticipate this pattern, as `shapeFromAst()` handles it,
but the regex-based fallback does not.
There was a problem hiding this comment.
Oh this seems relevant too, speaking of edge-cases @JPeer264
There was a problem hiding this comment.
You can't export default a DurableObject.
There was a problem hiding this comment.
At least what I'm aware of*
2 participants
Since
instrumentAgentWithSentryis here, it also needs to be supported within the Vite plugin and itsautoInstrumentflag. The issue here is that we need to parse through the imports in order to understand where theAgentimport is coming from. (The majority of the changes are tests again)It could also be that external libraries are extending from
Agent, such asMcpAgent, and these need to be wrapped too. It is super hacky but it works.For the tests I wished to add them in the
cloudflare-integration-tests, but unfortunatelyagentsneeds Node v20+ and locally we run with v18, so it wouldn't work without either bumping Nodejs or making the tests their own package so they are independent.