Skip to content

fix(telemetry): preserve span links in SQLite traces - #7755

Open
Samecho wants to merge 1 commit into
google:mainfrom
Samecho:codex/sqlite-span-links
Open

Samecho wants to merge 1 commit into
google:mainfrom
Samecho:codex/sqlite-span-links

Conversation

@Samecho

@Samecho Samecho commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Link to Issue or Description of Change

Related: #7484

SqliteSpanExporter drops 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_json column. 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 cec52fc because the links read back empty.

The fork validation run passed all 17 jobs for source commit 4069887:

  • Full tox suites: 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.
  • Full pre-commit checks, A2A 0.3 and MCP 2 compatibility checks passed.
  • No new mypy errors on Python 3.11–3.13: base and branch both report 269.

The Linux jobs use the same --no-install-package lancedb setting 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 TracerProvider and SimpleSpanProcessor, 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

  • Read CONTRIBUTING.md and the project skills.
  • Self-reviewed the change and documented the database upgrade behavior.
  • Added tests that fail before the fix.
  • New and existing unit tests pass in the Linux matrix.
  • Tested the installed wheel end-to-end with a real SQLite file.
  • No dependent downstream changes are required.

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.

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 BlueRaddish left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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 uncommitted

While 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 locked after 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.

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.

2 participants