fix(backend): surface SQLAlchemy instrumentation's silent no-op; document observability - #53
Merged
Merged
Conversation
Traced live traffic on the dev deployment (constraint violations, not-found errors, a rowversion-checked update) and found zero SQL-typed spans in AppDependencies across the entire session, despite the startup log claiming "SQLAlchemy database instrumentation enabled." Root cause: SQLAlchemyInstrumentor().instrument() defaults to raise_exception_on_conflict=False. opentelemetry-instrumentation-sqlalchemy declares support for sqlalchemy>=1.0.0,<2.1.0 (true as of its latest release, 0.64b0 — there is currently no compatible release), but this app requires the 2.1 line (see connection.py, for the mssql+mssqlpython dialect). With the default, that conflict is logged as a warning and instrument() returns normally WITHOUT actually patching sqlalchemy.create_engine — so instrument_sqlalchemy() had no way to notice, and every deploy has silently reported success while shipping zero database dependency spans. Pass raise_exception_on_conflict=True so the conflict raises instead, routing through the existing except block (which already logs exc_info and returns False) — an honest failure instead of a false success. This is a real, currently-unfixable upstream gap: no released version of opentelemetry-instrumentation-sqlalchemy supports SQLAlchemy 2.1. The app will correctly report no SQL dependency spans until that changes; this commit's job is making that visible rather than silently wrong. Also extends test_telemetry.py: instrument_sqlalchemy()'s return value must now match whether create_engine was actually replaced — verified directly by comparing object identity before/after, not by trusting the return value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Documents the telemetry stack (server + client App Insights export, structured access log correlation id, platform logs) and — concretely, verified by generating real traffic against the dev deployment via Chrome DevTools and cross-referencing every log source — how to actually trace one user action end-to-end. Two things worth calling out explicitly, since they'd otherwise surprise whoever next reaches for this: - App Insights' own trace-id (traceparent/OperationId) is per browser page session, not per user action — every request from one loaded page shares it. The per-action key is request_id (app/middleware.py), verified by querying AppTraces for the exact request_id off a live 409 constraint violation and getting back both the access-log line and the route handler's own error-detail log line for that one request. - The Known gap section documents the SQL-instrumentation limitation fixed in the previous commit: no released opentelemetry-instrumentation- sqlalchemy version supports SQLAlchemy 2.1, so AppDependencies has no SQL spans until upstream catches up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…QLAlchemy support Researched whether Microsoft/Azure ship an alternative SQL telemetry path, since SQLAlchemy can't be downgraded (the 2.1 line is required for the mssql+mssqlpython dialect). opentelemetry-instrumentation-dbapi — the same OpenTelemetry contrib family as the sqlalchemy instrumentor, one layer lower — patches a DB-API driver's connect() directly and has no SQLAlchemy version dependency at all, so it isn't blocked on the SQLAlchemy-specific instrumentor catching up to 2.1. Verified before committing to the approach: - trace_integration(mssql_python, "connect", "mssql") genuinely patches mssql_python.connect (confirmed via identity check) - the resulting spans carry db.statement (actual SQL text) and db.system - critically, a DB span emitted during a request shares that request's trace id and is parented to the request span — confirmed with a real FastAPI request through TestClient, not just an isolated connect() call instrument_sqlalchemy() now calls trace_integration against mssql_python directly; SQLAlchemyInstrumentor and the now-unneeded opentelemetry-instrumentation-sqlalchemy dependency are removed. Test changes: replaces the create_engine-identity check (no longer the patching mechanism) with two tests — one confirming mssql_python.connect gets patched, one driving a real request through an instrumented app and asserting the resulting DB span nests under the request span (shared trace id, real parent-child relationship), using sqlite3 as a stand-in DB-API 2.0 driver to isolate the assertion from needing a reachable SQL Server. README: updates the "Known gap" section — no longer a gap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Summary
Generated real traffic against the dev deployment via Chrome DevTools (CRUD, search, a rowversion-checked update, deliberate 409/422/404 errors) to verify the observability story end-to-end, then found and fixed a real SQL-tracing gap in two stages.
Stage 1 — surfaced the silent failure:
AppDependenciesshowed zero SQL statement spans despite the startup log claimingSQLAlchemy database instrumentation enabled. Root cause:SQLAlchemyInstrumentor().instrument()defaults toraise_exception_on_conflict=False—opentelemetry-instrumentation-sqlalchemydeclares support forsqlalchemy>=1.0.0,<2.1.0, this app requires the 2.1 line (for themssql+mssqlpythondialect, which can't change), and with the default that conflict silently logs a warning instead of failing. First fix:raise_exception_on_conflict=True, so the app honestly reports failure instead of a false success.Stage 2 — actually fixed it: since SQLAlchemy can't be downgraded, researched whether Microsoft/Azure ship an alternative.
opentelemetry-instrumentation-dbapi(same OpenTelemetry contrib family, one layer lower) patches a DB-API driver'sconnect()directly and has no SQLAlchemy version dependency at all — it isn't blocked on the SQLAlchemy-specific instrumentor ever catching up to 2.1.instrument_sqlalchemy()now callstrace_integrationagainstmssql_pythondirectly.SQLAlchemyInstrumentorand the now-unneededopentelemetry-instrumentation-sqlalchemydependency are removed.Also verified, tracing a live request end-to-end: the App Insights
traceparent/OperationIdis per browser page session, not per user action. The reliable per-action key isrequest_id(app/middleware.py), confirmed by pulling the exactrequest_idoff a live409 CONSTRAINT_VIOLATIONand queryingAppTraces | where Properties.request_id == "<id>", which returned both the access-log line and the route handler's own database-error detail line for that one request.README: adds an Observability section (what's wired up, how to trace one action end-to-end, and the now-resolved SQL-tracing note explaining why the DB-API approach was chosen).
Verification
instrument_sqlalchemy()patchesmssql_python.connect(identity-checked, not just trusting the return value)sqlite3as a stand-in DB-API 2.0 driver in the committed unit testsmssql_pythonconnection to the live container. Captured 7 realdb.statementspans (actual SQL text — SQLAlchemy's own dialect-detection queries plus a manualSELECT @@VERSION), every one sharing the exacttrace_idof theGET /pingrequest span and correctly parented underneath it. This is the strongest evidence available short of the actual Azure SQL connection.AppTraces | where Properties.request_id == "<id>") verified directly against the real Log Analytics workspace using arequest_idpulled from a live request🤖 Generated with Claude Code