Skip to content

feat(mongo): record collection operations as normalized query events - #239

Draft
evlawler wants to merge 1 commit into
mainfrom
feat/mongo-query-events
Draft

evlawler wants to merge 1 commit into
mainfrom
feat/mongo-query-events

Conversation

@evlawler

@evlawler evlawler commented Sep 17, 2026

Copy link
Copy Markdown

The mongo hook records each Collection method as a function call with its actual arguments, under a synthetic mongodb/<collection> class. That is readable in a single AppMap, but it keeps Mongo operations out of everything that works on sql_query events: the digest, the query section of change reports, and the Database lifeline in sequence diagrams.

What changes

  • Each recorded operation now also gets a sql_query event, nested under the function call the way the Prisma hook nests the SQL it observes. It has database_type: "mongodb" and a statement in shell form with a normalized argument shape, for example db.users.updateOne({"_id": ?}, {"$set": {"name": ?}}, {"upsert": ?}).
  • The function call events are unchanged, so existing consumers and the real argument values are still there.

The shape rules (written down at the top of src/hooks/mongoQuery.ts)

  • Keys and operators are kept, in order, as JSON strings. Every leaf value becomes ?.
  • Arrays collapse to their distinct element shapes in order of first appearance. {"$in": [1, 2, 3]} is {"$in": [?]}, and an insertMany of a thousand documents of one shape is one statement.
  • Aggregation pipelines and update pipelines keep every stage in order, because stage order and repetition are part of the query.
  • Name arguments (a distinct field, an index name, a new collection name) stay verbatim.
  • Plain objects and Maps are documents. Everything else is a leaf: ObjectId, Date, Decimal128, Buffers, RegExp, functions, class instances such as a driver session in options.
  • Collections whose name is not an identifier path render as db.getCollection("my-coll").
  • Cycles, nesting past 32 levels, and array elements past the first 1000 become ? or are skipped. Formatting never throws; a hostile argument renders as db.coll.find(?).

Edge cases handled

  • The query return is emitted when the driver hands back its promise and fixed up with the real elapsed time, or turned into an exception event, when the promise settles. A caught duplicate key error shows as an exception on both the query and the call.
  • Cursors (find, aggregate, listIndexes, watch) are not promises. Their statement is recorded when the cursor is created, so .sort(), .limit() and friends applied afterwards are not part of it.
  • The driver implements some methods on top of others (findOne calls find). The inner call keeps its function call event, but only the outermost operation gets a query event, so one logical operation is one query.
  • Callback style calls get the same treatment.
  • Trailing undefined arguments are omitted, so find() and find(undefined) are the same statement.

Companion changes

Test

  • src/hooks/__tests__/mongoQuery.test.ts: 25 unit tests for the shape rules. The Java agent's unit tests assert the same strings.
  • test/mongo: the fixture now also runs insertMany, a find with an operator and options, an aggregate, createIndex, a caught duplicate key error, and deleteMany. The snapshot was regenerated against MongoDB 8.0.4. The test still requires MONGODB_URI, as before, and CI provides it.
  • yarn lint, yarn typecheck, and the unit suite pass.

Written by Claude in a Claude Code session for Elizabeth Lawler. The commit carries Claude as author.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LntddoBsqjRDRBepBx7oLZ

The mongo hook records each Collection method as a function call with its
actual arguments. That is fine for reading one AppMap, but it keeps Mongo
operations out of everything that works on sql_query events: the digest,
the query diff in change reports, and the Database lifeline in sequence
diagrams.

Each recorded operation now also gets a sql_query event, nested under the
function call the way the Prisma hook nests the SQL it observes. The event
has database_type "mongodb" and a statement in shell form with a normalized
argument shape:

  db.users.updateOne({"_id": ?}, {"$set": {"name": ?}}, {"upsert": ?})

Keys and operators are kept in order, every leaf value becomes `?`, arrays
collapse to their distinct element shapes (so an insertMany of a thousand
documents of one shape is one statement, and `$in` lists do not vary by
length), and aggregation and update pipelines keep every stage in order.
Name arguments (a distinct field, an index name, a new collection name)
stay verbatim. BSON values, class instances, Buffers, functions and cyclic
or very deep structures are leaves. The rules are written down at the top
of src/hooks/mongoQuery.ts; the Java agent implements the same rules.

The query return event is emitted when the driver hands back its promise
and is fixed up with the real elapsed time, or turned into an exception
event, when the promise settles. Cursors (find, aggregate, listIndexes,
watch) are not promises, so their return stays as emitted. Methods the
driver implements on top of other Collection methods (findOne calls find)
produce one query event, for the outer call only.

The fixture now also covers insertMany, find with an operator and options,
aggregate, createIndex, a caught duplicate key error, and deleteMany.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LntddoBsqjRDRBepBx7oLZ
@dividedmind

Copy link
Copy Markdown
Collaborator

I'm not convinced recording mongo db queries as sql_query is the right shape – the documentation, all of our tooling and even the field name makes it clear that SQL is what is expected there. It feels like a hack – I get that it allows leveraging some things that care about sql_query, but most of precisely these things expect SQL there and need to be patched for this to work reliably anyway. Since many of them need to be patched anyway, perhaps it'd be more robust and cleaner to introduce a different well-defined structure for non-SQL database queries like this. @kgilpin wdyt?

@kgilpin

kgilpin commented Sep 17, 2026 via email

Copy link
Copy Markdown
Contributor

@dividedmind

Copy link
Copy Markdown
Collaborator

It will cause a lot of problems downstream to put anything but SQL in that field. I’m not sure what to recommend.

Yes that's exactly my concern. My take is if we really want to have normalized mongo queries somewhere we need to invent a specific, different, place and form to put them in the schema. I'm not sure if it's really worth it, though being able to eg. see the changed queries in the query reports might be a good reason – currently mongo tracing is kind of a special case in appmap-node, and the way it's recorded is mostly to make it clear and readable in the diagrams (no normalization of any kind iirc), so it might be worthwhile to systematize it (maybe normalize it too) so other agents (eg. java) can generate the same shape, then downstream tool support could be enhanced with support for this as needed. But this is something we'd need to discuss, plan and design first.

@evlawler
evlawler marked this pull request as ready for review September 17, 2026 16:16
@evlawler
evlawler requested review from dividedmind and a lite review from Copilot September 17, 2026 16:16

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved issues remain in bulk-write pipeline normalization and large string-array handling, with callback coverage also missing.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds normalized MongoDB sql_query events alongside existing collection call events for query-oriented AppMap analyses.

Changes:

  • Normalizes MongoDB operation arguments and query statements.
  • Records query lifecycle and failure events.
  • Expands unit and integration coverage with updated snapshots.
File summaries
File Reviewed change
test/mongo/index.js Expanded MongoDB integration scenarios.
test/__snapshots__/mongo.test.ts.snap Updated expected AppMap output.
src/hooks/mongoQuery.ts Formats and normalizes MongoDB statements.
src/hooks/mongo.ts Instruments MongoDB query events.
src/hooks/__tests__/mongoQuery.test.ts Tests normalization behavior.
Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/hooks/mongoQuery.ts
Comment on lines +103 to +104
if (Array.isArray(value) && value.every((v) => typeof v === "string"))
return `[${value.map((v) => JSON.stringify(v)).join(", ")}]`;
Comment thread src/hooks/mongoQuery.ts
return shape(value, true);
case "document":
// An update can be a pipeline (an array of stages) instead of a document.
return shape(value, name === "update" && Array.isArray(value));
Comment thread src/hooks/mongo.ts
Comment on lines +132 to 136
const queryEvents = queryCallEvents(recordings, statement);

const startTime = getTime();
args.push((err: unknown, res: unknown) => {
setCustomInspect(res, customInspect);

Copy link
Copy Markdown
Author

Agreed, on both points. sql_query was chosen because it is the only structure that reaches the digest today, but making it work already needed a patch to the appmap-js normalizer, and that is the pattern you describe: every consumer that assumes SQL has to be patched one at a time, and the ones we cannot see (the server) would break silently. That is not a good foundation.

We will step back and design this properly instead: a dedicated place in the schema for non-SQL database queries, one normalized shape that both agents produce, and downstream support added deliberately. The normalization rules and the Java port in these PRs are meant to feed into that design rather than merge as they are.

This PR and getappmap/appmap-java#330 go back to draft as reference for the redesign. getappmap/appmap-js#2401 is closed, since it only existed to support this approach.


Generated by Claude Code

@evlawler
evlawler marked this pull request as draft September 17, 2026 16:26
@dividedmind

Copy link
Copy Markdown
Collaborator

Another idea might be to add a generic marker and field for normalized data. This way the events from mongodb as recorded by appmap-node today could be augmented with something like kind: dbquery (or even maybe just use a label for that?), or maybe just reuse the existing database_type: field, and add a separate field (normalized or normalized_query) for the normalized query. This way existing consumers can understand it and not get confused, augmented clients could look for the label to know that a call is a db query and look at the normalized field if it needs that. And any other weird nonsql db could use the same mechanism in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants