feat(sqlalchemy): Support span streaming#6132
feat(sqlalchemy): Support span streaming#6132alexander-alderman-webb wants to merge 18 commits intomasterfrom
Conversation
Codecov Results 📊✅ 2215 passed | ⏭️ 244 skipped | Total: 2459 | Pass Rate: 90.08% | Execution Time: 5m 51s 📊 Comparison with Base Branch
✨ No test changes detected All tests are passing successfully. ❌ Patch coverage is 9.84%. Project has 12704 uncovered lines. Files with missing lines (2)
Coverage diffGenerated by Codecov Action |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 98c67f0. Configure here.
sentrivana
left a comment
There was a problem hiding this comment.
Looking great overall! Just not sure about a handful of attributes.
| db_system = _get_db_system(conn.engine.name) | ||
| if db_system is not None: | ||
| span.set_data(SPANDATA.DB_SYSTEM, db_system) | ||
| set_on_span(SPANDATA.DB_SYSTEM, db_system) |
There was a problem hiding this comment.
db.name was deprecated in favor of db.system.name, so we should use that in streaming mode
| db_name = conn.engine.url.database | ||
| if db_name is not None: | ||
| span.set_data(SPANDATA.DB_NAME, db_name) | ||
| set_on_span(SPANDATA.DB_NAME, db_name) |
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def record_sql_queries_supporting_streaming( |
There was a problem hiding this comment.
Any reason not to adapt the existing record_sql_queries instead? Why a new func, if we still support both streaming and legacy mode in it?
| if params_list is not None: | ||
| data["db.params"] = params_list | ||
| if paramstyle is not None: | ||
| data["db.paramstyle"] = paramstyle | ||
| if executemany: | ||
| data["db.executemany"] = True | ||
| if record_cursor_repr and cursor is not None: | ||
| data["db.cursor"] = cursor |
There was a problem hiding this comment.
We're setting these as attributes couple lines later, but none of them seems to be in conventions?
| if not client.is_active(): | ||
| return |
There was a problem hiding this comment.
I believe this check should apply to both streaming and non streaming modes, so I'd move it outside of the isinstance branch

Description
Adapt
add_query_source()analogously to changes toadd_http_request_source()in #6084.Add
record_sql_queries_supporting_streaming()based onrecord_sql_queries()so that the streaming path is not triggered for other ORM integrations until they support the streaming trace lifecycle. Once the last ORM integration supports span-streaming the functions can be de-duplicated.Set
<unknown SQL query>as the name when the description was previouslyNone.Adapting Tests
sedcommands used for converting transaction context managers:sedcommands used for converting code source attributes:sedcommands used for converting event capture:sedcommands used for convertingop:sedcommands used for convertingdescription:sedcommands used for convertingdatatoattributes:sedcommands used for converting timestamps:Issues
Reminders
tox -e linters.feat:,fix:,ref:,meta:)