Feat: Port SqliteSpanExporter from adk-python to core telemetry - #502
Open
AmaadMartin wants to merge 7 commits into
Open
Feat: Port SqliteSpanExporter from adk-python to core telemetry#502AmaadMartin wants to merge 7 commits into
AmaadMartin wants to merge 7 commits into
Conversation
added 5 commits
August 1, 2026 22:59
Ports adk-python's SqliteSpanExporter to adk-js. Spans exported through the existing OTelHooks.spanProcessors surface are persisted to a SQLite file, and whole trace trees can be read back for a session id after a process restart. Reuses the MikroORM stack already shipped for DatabaseSessionService: the @mikro-orm/sqlite driver is loaded through a dynamic import so consumers who install @google/adk without the optional peer are unaffected. @opentelemetry/core becomes a direct dependency because ExportResult and ExportResultCode are named in the export() signature.
Ports all 16 contract tests from adk-python's test_sqlite_span_exporter.py and adds JS-specific cases: persistence across a restart, concurrent first export, a corrupt database file, nanosecond precision beyond Number.MAX_SAFE_INTEGER, a non-string session id attribute, rows written by another writer, and an end-to-end pass through a real BasicTracerProvider + SimpleSpanProcessor.
The upsert already applies rows in order, so the last version of a repeated span id wins without a pre-pass. Removing the map with the test still in place proved the behaviour is unchanged, and the test now interleaves three copies of one span id with a distinct span to pin it.
Move the JSON replacer to a module-level factory so it is not defined inside another function, convert epoch nanoseconds without a string round trip, and drop a default for a NOT NULL column that can never be missing.
MikroORM.init opens the file before it can throw, and it throws without returning a handle, so a failed open leaked the connection: the Windows CI job then failed to unlink the database file with EBUSY. Initialize with connect: false so the exporter owns the instance before any I/O, and close it if connecting or schema setup fails.
added 2 commits
August 1, 2026 23:55
A driver error inlines the failing statement together with its bound values, so logging it verbatim dumped attributes_json - which carries the serialized LLM request and response by default - into the application log, and again through OpenTelemetry's global error handler, which stringifies every property it can reach on ExportResult.error including cause. The failure is now named by its error class and SQLite code only, and the reported Error carries no cause. Also: shutdown awaits an in-flight open so a shutdown issued mid-export cannot orphan the connection, and the connection sets busy_timeout to 30s, matching the reference exporter's sqlite3 connect timeout, so a contended write waits instead of dropping the batch.
The exporter file was mostly free functions: attribute keys, time conversion, attribute (de)serialization and the ReadableSpan <-> StorageSpan mappers left the class itself as a minority of the file. That mapping concern is meaningless outside this feature, so it belongs next to the entity it maps rather than in the shared utils directory, and its tests move with it. Also drops the JSON replacer. It defended against bigint, function and symbol attribute values, none of which can reach it: OpenTelemetry types attributes as primitives and primitive arrays, tracing.ts already substitutes '<not serializable>' for an unstringifiable payload before setting one, and JSON.stringify silently drops functions and symbols anyway - so for those two the replacer fabricated a '<not serializable>' string attribute that then read back as a real value. The plain try/catch fallback remains. Deletes the unixNanosToHrTime wrapper, which had no production caller, shrinks compareByStartTime to a signed subtraction, projects the trace-id lookup with fields: ['traceId'] instead of re-reading whole rows, and widens the SQLite code allowlist to /^SQLITE_[A-Z_]+$/ so extended codes such as SQLITE_CONSTRAINT_NOTNULL keep their diagnostic instead of degrading to a bare Error.
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Closes: N/A
Related: N/A
Problem: adk-python can persist OpenTelemetry spans to a local SQLite database (
src/google/adk/telemetry/sqlite_span_exporter.py), so the local dev UI can reload trace history for older sessions after the process restarts. adk-js has no equivalent:core/src/telemetry/contains onlygoogle_cloud.ts,setup.tsandtracing.ts, and the only span sink in the JS dev server is the process-localInMemoryExporterindev/src/utils/telemetry_utils.ts, whose contents are lost on restart.Solution: Port
SqliteSpanExportertocore/src/telemetry/sqlite_span_exporter.ts, with theReadableSpan<-> row mapping incore/src/telemetry/db/span_mapper.tsnext to theStorageSpanentity it maps. The exporter is a plain@opentelemetry/sdk-trace-baseSpanExporter, so it plugs into the existingOTelHooks.spanProcessorssurface with no changes tosetup.tsand no second OTel init path:getAllSpansForSession(sessionId)returns whole trace trees: it resolves the trace ids belonging to the session first, then returns every span of those traces, so parent and sibling spans that carry no session id of their own come back too.Persistence reuses the MikroORM stack that already ships for
DatabaseSessionServicerather than adding a second SQLite client.@mikro-orm/sqliteis a dev/peer dependency only, so it is loaded through a dynamicimport()exactly ascore/src/sessions/db/operations.tsdoes — installing@google/adkwithout the peer keeps every other import path working. Schema creation reusesensureDatabaseCreated()(ensureDatabase()+updateSchema({safe: true})), which never drops tables or columns, so the exporter can safely point at the same.dbfile asDatabaseSessionService. (Safe mode may still re-create aspanstable another writer declared withINTEGERrather thanbigint; row data survives and both declarations carry INTEGER affinity, so this is inert.)Scope: this PR ships the write path plus its read API, and nothing consumes the read API yet.
getAllSpansForSessionhas no in-repo caller. Its intended consumer isdev/src/server/adk_api_server.ts, which still serves/debug/trace/session/:sessionIdfromInMemoryExporter.getFinishedSpans(dev/src/utils/telemetry_utils.ts) — the same method shape, which is why the return type isReadableSpan[]rather than a bespoke row type. Repointing that endpoint is a deliberate follow-up, queued separately, because it changes thedev/CLI surface (a flag to choose the sink, a migration for the in-memory default) and would make this diff unreviewable. adk-python is in the same state: itsSqliteSpanExporterappears only in its own module and its tests, wired into no entry point, so parity does not require it here either.core/src/telemetry/setup.ts,core/src/index_web.ts(the browser bundle must not pull in SQLite) and everything underdev/are untouched.Collision check.
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000returned 401 open PRs; none touchescore/src/telemetry/sqlite_span_exporter.tsorcore/src/telemetry/db/, and no open PR title or branch mentions a SQLite span exporter. The adjacent telemetry PRs (#400, #399, #395, #394, #387, #357, #348, #308) touchmetrics.ts,token_usage.ts,google_cloud.tsandtracing.tsonly, and the adjacent MikroORM PRs (#500, #291, #386) touch driver peer-dependency declarations and the session service. No overlap, so this branches frommainrather than stacking.Parity wins (observable across the boundary, so adk-python decides):
spansand all nine column names/nullability, verified against the generated DDL:CREATE TABLE `spans` (`span_id` text not null, `trace_id` text not null, `parent_span_id` text null, `name` text not null, `start_time_unix_nano` bigint null, `end_time_unix_nano` bigint null, `session_id` text null, `invocation_id` text null, `attributes_json` text null, primary key (`span_id`))spans_session_id_idx,spans_trace_id_idx.gcp.vertex.agent.session_id->gen_ai.conversation.idsession-id fallback, andgcp.vertex.agent.invocation_id. These are the keys adk-js already emits (core/src/telemetry/tracing.ts), so no key translation is needed. The fallback uses||, not??, so an empty session id falls through exactly as Python'sordoes.busy_timeout = 30000on the connection, matching the reference'ssqlite3.connect(timeout=30.0). Without it a contended write fails immediately at the driver default of 1s — plausible precisely in the shared-file-with-DatabaseSessionServiceconfiguration this class documents.Idiom wins (never leaves the process, so local convention decides):
forceFlush()returnsPromise<void>, notbool— mandated by the OTel JSSpanExporterinterface.export()reports through theresultCallback(ExportResultCode.SUCCESS/FAILED) instead of returning a value, again per the JS interface. It never throws synchronously and never leaves an unhandled rejection.AttributeValue(nested objects, mixed arrays) are dropped on read. adk-python round-trips them because Python span attributes are untyped; OTel JS typesAttributesas primitives and homogeneous primitive arrays, so such a value could never have been a real JS span attribute, and smuggling it through would need a cast.'<not serializable>'sentinel here. Python needs one becausejson.dumpsmeets arbitrary objects; in JS the only valuesJSON.stringifycannot represent arebigintand cycles, neither of which is anAttributeValue— andtracing.ts'ssafeJsonSerializealready substitutes that exact literal before an attribute is set. Reimplementing it in the exporter would have been the second copy of the same sentinel in the same directory, and for functions and symbols it was actively wrong:JSON.stringifydrops those silently, so a replacer fabricated a'<not serializable>'string attribute that then read back as a real value. Atry/catchreturning'{}'is all that remains.shutdown()awaits an in-flight open before closing, so a shutdown issued mid-export cannot orphan the connection that open is still creating (Python, being synchronous, has no equivalent race).threading.Lockis replaced by a memoizedinitpromise: the event loop is single-threaded and MikroORM serialises its own connection, so N concurrentexport()calls perform exactly oneMikroORM.init. A failed open is not memoized, so a later export retries instead of being poisoned forever (Python reconnects the same way).Three implementation notes where the obvious approach is wrong:
Number.MAX_SAFE_INTEGER(9.0e15). Thebigintcolumn stores them exactly, but thesqlite3driver behind@mikro-orm/sqlitehands JavaScript a double for integer columns, which silently rounds1750000000123456789to...800.StorageSpantherefore uses aBigIntType('string')subclass that overridesconvertToJSValueSQLtocast(<col> as text). That cast also applies toORDER BY, which would then sort lexicographically ('1000000000' < '1750000000' < '999'), so ordering is done in JS with a BigInt comparison instead. Both effects are pinned by tests.attributes_json— which by default carries the serializedLlmRequest/LlmResponseand tool arguments, sinceshouldAddRequestResponseToSpans()defaults to true — into the application log. The same string was handed back asExportResult.error, andSimpleSpanProcessorroutes that into OpenTelemetry's global error handler, whoseflattenExceptionstringifies every property it can reach,causeincluded, so it was logged twice. A failure is now named by its error class and SQLite code only (Failed to export spans to SQLite: NotNullConstraintViolationException (SQLITE_CONSTRAINT)), and the reportedErrorcarries nocauseand no driver text on any property. The allowlist is/^SQLITE_[A-Z_]+$/— the underscore matters, or extended codes likeSQLITE_CONSTRAINT_NOTNULLandSQLITE_IOERR_READwould degrade to a bareErrorand lose the diagnostic the allowlist exists to keep.MikroORM.init()opens the SQLite file before it can fail, and it fails without returning a handle, so a bad file leaked an open connection with no way to close it. The first CI run on this PR caught exactly that:run-tests (windows-latest)failed withEBUSY: resource busy or locked, unlink '...corrupt.db'while Linux and macOS passed.open()now usesconnect: falseso the exporter owns the instance before any I/O happens, and closes it if connecting or schema setup throws. Verified locally too: a Node process that runs one failing export and shuts down now exits cleanly instead of hanging on the leaked handle.New direct dependency:
@opentelemetry/core@^2.8.0.ExportResultandExportResultCodelive there and are named in theexport()signature (TypeScript does not contextually type method parameters from animplementsclause), and@opentelemetry/sdk-trace-basere-exports neither. It was already present at exactly 2.8.0 as a transitive dependency of@opentelemetry/sdk-trace-base@2.8.0; the lockfile diff is the single added line. No otherpackage.jsonor version field is touched.No suppressions of any kind are added: the diff against the branch base returns nothing for
@ts-expect-error,@ts-ignore,eslint-disable,v8 ignore,as any,as neveror a bare: any, tests included.Follow-up worth naming:
hrTimeToUnixNanos(exact,string) now coexists withdev/src/utils/telemetry_utils.ts'shrTimeToNanoseconds(lossynumber, and the one behind the live/debug/trace/session/:sessionIdendpoint). Two conversions of the same quantity in two packages, one silently wrong aboveNumber.MAX_SAFE_INTEGER. Consolidating them belongs with the dev-server wiring change, not here.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
40 new tests across two files that mirror the source layout —
core/test/telemetry/sqlite_span_exporter_test.ts(33) andcore/test/telemetry/db/span_mapper_test.ts(7):Coverage of the three new source files is 100% of statements, branches, functions and lines (
sqlite_span_exporter.ts,db/span_mapper.ts,db/schema.ts). No coverage suppression is used and no structure was weakened to reach the number.All 16 contract tests from
adk-python/tests/unittests/telemetry/test_sqlite_span_exporter.pyhave a counterpart, plus JS-specific cases the Python suite does not have: persistence across a restart, two concurrent first exports, a corrupt database file, a retry after the fault is cleared, nanosecond precision beyondNumber.MAX_SAFE_INTEGER, a non-string session-id attribute, a row written by another writer (NULL timestamps + invalid attributes JSON), a failure that must not log its payload, two shutdown-races-open cases, and an end-to-end pass through a realBasicTracerProvider+SimpleSpanProcessor(a local provider, notmaybeSetOtelProviders, so the global OTel provider is not mutated).test_shutdown_closes_connectionasserts on Python's private_conn. The JS counterpart asserts the same behaviour through the public API instead of widening aprivatemember: aftershutdown()the previously written spans are still readable (proving a clean reopen), and a secondshutdown()does not throw.Every test was proved able to fail. Each mutation below was applied to the source, the suite re-run, and the source reverted:
gen_ai.conversation.idsession-id fallbackindexes a span that only carries the conversation id attributeexpected [] to have a length of 1 but got +0compareByStartTimeorders spans numerically by start time(+1)expected '0000000000000300,…' to be '0000000000000100,…'session_idonly instead of trace-wideincludes trace siblings that carry no session id(+1)expected [ 'call_llm' ] to deeply equal [ 'call_llm', 'invocation', …(1) ]Numberinstead ofBigIntconverts a timestamp above Number.MAX_SAFE_INTEGER exactly(+1)expected '1750000000123456800' to be '1750000000123456789'cast(<col> as text)read overridepreserves nanosecond precision beyond Number.MAX_SAFE_INTEGERexpected [ 1750000000, 123456800 ] to deeply equal [ 1750000000, 123456789 ]SUCCESSon a persistence failurereports failure without throwing when the database file is not a database(+1)expected +0 to be 1shutdown()stops clearing the memoized init promisereopens the database after shutdown …(+2)Unable to acquire a connectionstores a span whose session id attribute is not a string without indexing itexpected [ { name: 'test_span', …(16) } ] to deeply equal []drops nested objects and mixed or non-primitive arraysexpected { keep: 'value', …(3) } to deeply equal { keep: 'value', nulls: […] }String(cause)instead ofdescribeError(cause)reports a failure without logging the span payload that caused itinsert into spans … 'SUPER_SECRET_PROMPT' …Error.causereports a failure without logging the span payload that caused itexpected NotNullConstraintViolationException: inse… to be undefinedreopens the database on the next export once the fault is clearedexpected 1 to be +0/^SQLITE_[A-Z]+$/keeps an extended SQLite driver codeexpected 'Error' to be 'Error (SQLITE_CONSTRAINT_NOTNULL)'Mutation 14 — removing an intra-batch de-duplication map by span id — killed no test: MikroORM's
upsertManyapplies the rows in order and the last version wins on its own, so that code was deleted rather than kept, and the test was strengthened to interleave three copies of one span id with a distinct span.Connection-leak regressions are the one class of defect these mutations cannot surface on Linux, because POSIX happily unlinks an open file. They are pinned two other ways: the failure and shutdown-race tests unlink the database file themselves right after
shutdown()(await expect(rm(...)).resolves.toBeUndefined()), which is a real assertion on Windows, and therun-tests (windows-latest)job on this PR is the end-to-end proof. The same honesty applies tobusy_timeout, a per-connection pragma with no observable effect through the public API: it is covered but not asserted, and was verified out of band — a fresh connection reports1000, the exporter's reports30000.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
To inspect the on-disk artefact by hand, export a couple of spans to a temp path, shut the exporter down, construct a second one on the same path and read the session back, then open the file with any SQLite client:
I ran exactly that. The schema is the nine snake*case columns plus both
spans*\*\_idxindexes shown above,typeof(start_time_unix_nano)isinteger, andcast(start_time_unix_nano as text)is1750000000123456789— stored and read back with no rounding. The second exporter returned both spans in start-time order, including the parent span that carries no session id, with the child'sparentSpanContext.spanIdpointing at the parent.Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.