Repository navigation
Conversation
Store link contexts and attributes so they survive exporter restart. Upgrade existing span tables without losing rows, and serialize schema initialization across exporter instances. Related: google#7484
BlueRaddish
left a comment
There was a problem hiding this comment.
[P2] Avoid taking the writer lock when the schema is already current
At src/google/adk/telemetry/sqlite_span_exporter.py:110, the unconditional BEGIN IMMEDIATE also runs when links_json and both indexes already exist. This changes an otherwise read-only use of the public exporter: opening a saved trace for get_all_spans_for_session() now waits for an unrelated writer, then fails after the default 30-second timeout. SQLite WAL normally allows that reader to proceed.
I reproduced this against parent cec52fc005a2c76963feb0a424647ca9576ebb2d and head 4069887b1fd5cc582a0b95302bfb7a07c6b9968b using the same already-migrated database, created by the head exporter through a real TracerProvider / SimpleSpanProcessor with one linked span. After closing the seed exporter, the independent writer was:
with sqlite3.connect(db_path) as connection:
assert connection.execute("PRAGMA journal_mode=WAL").fetchone()[0] == "wal"
writer = sqlite3.connect(db_path)
writer.execute("BEGIN IMMEDIATE")
writer.execute("UPDATE spans SET name = name") # hold uncommittedWhile that transaction remained open, I constructed each version's SqliteSpanExporter(db_path=db_path) and called get_all_spans_for_session("archived-session"):
- Parent: returned the saved span.
- Head: constructor raised
sqlite3.OperationalError: database is lockedafter 30.108 seconds, before readback was possible. - After
writer.rollback(); writer.close(), head returned the span and its link. Both versions also passed the ordinary writable control. The main database bytes were unchanged across the comparison.
Could the already-current schema take a read-only fast path, with BEGIN IMMEDIATE and a second schema check used only when creation/migration is needed? That would retain serialization for competing migrations without requiring every trace reader to become a writer. A regression test should hold the independent WAL writer open against a fully current schema and verify public readback still succeeds.
Validation used Python 3.13.17 / OpenTelemetry SDK 1.44.0 on Ubuntu 24.04, with the original 30-second timeout. The complete owning test module passed (32 passed, 0 skipped), and normal scoped pre-commit hooks passed (10 passed, 4 nonapplicable skips) after supplying the parent history needed by the new-file hook. No source edits, full-tox run, or live model/provider calls were involved. This review is limited to the readback regression above.
Link to Issue or Description of Change
Related: #7484
SqliteSpanExporterdrops span links, so reloading a saved trace loses its references to other traces. This stores each link's context and attributes and reconstructs them when the span is read back.Existing databases get an additive
links_jsoncolumn. Old rows remain readable with no links, and writers using the old column list still work. The schema check and migration run in one write transaction so two exporter instances can open the same old database safely.This is the links-only follow-up asked about on the issue. #7486 handles status/events and #7504 handles kind; this does not duplicate either change or close the broader issue.
Testing Plan
Unit Tests:
32 SQLite exporter tests pass, including restart round trips, empty links, malformed stored JSON and concurrent migration. The migration tests cover the original table plus the status/events and kind columns, separately and together. Four core regressions fail on unmodified main at
cec52fcbecause the links read back empty.The fork validation run passed all 17 jobs for source commit
4069887:toxsuites: Python 3.11: 21,422 passed; 3.12 and 3.13: 21,413 passed each; 3.14: 21,415 passed. No failures. Existing skips and expected failures are in the logs.The Linux jobs use the same
--no-install-package lancedbsetting as upstream CI. The local Windows full telemetry run was not green: it had 5 golden-output failures; untouched main had 12, with a different failing set. The Linux full suites above passed.Manual End-to-End (E2E) Tests:
Used a real
TracerProviderandSimpleSpanProcessor, then shut down the exporter and loaded the SQLite file from a separate Python process. The link points to another trace and carries trace flags, ordered TraceState and attributes. Repeated with a database using the original nine-column schema.Before: both databases returned 0 links. After: both returned 1 link with the original context and attributes, and the legacy database kept its original span. The installed-wheel job repeats this in a clean environment with the smoke script. It exercises the exporter directly.
Checklist
Additional context
Links already discarded by an older version cannot be recovered. Resource persistence is outside this change.
AI assistance: Codex helped implement and test this change.