fix(sqlalchemy): avoid IndexError in _operation_name for comment/whitespace-only statements - #4943
Open
Atishyy27 wants to merge 4 commits into
Open
Conversation
Atishyy27
added a commit
to Atishyy27/opentelemetry-python-contrib
that referenced
this pull request
Aug 10, 2026
Signed-off-by: Atishyy27 <142108881+Atishyy27@users.noreply.github.com>
henry3260
reviewed
Aug 12, 2026
henry3260
left a comment
henry3260
left a comment
Contributor
There was a problem hiding this comment.
Looks very nice! just leave some nits
Comment on lines
+321
to
+323
| tokens = self._leading_comment_remover.sub("", statement).split() | ||
| if tokens: | ||
| parts.append(tokens[0]) |
Contributor
There was a problem hiding this comment.
Falling back to the db name for the span name makes sense, but the same value is also used as db.operation.name, so that field ends up holding the database name (or the vendor, e.g. sqlite, when there is no db name). The conflation isn't introduced by this PR _operation_name has fed db.operation.name since #4110 , just noting it here. Might be worth a follow-up.
Contributor
There was a problem hiding this comment.
Yeah, care to file a bug so we can track it?
Atishyy27
added a commit
to Atishyy27/opentelemetry-python-contrib
that referenced
this pull request
Aug 13, 2026
…n the regression test Per henry3260's review on open-telemetry#4943: constructing EngineTracer directly registers SQLAlchemy event listeners on the engine that the test never removed. Clean them up via EngineTracer.remove_all_event_listeners() in a finally block so they don't leak past the test. Signed-off-by: Atishyy27 <142108881+Atishyy27@users.noreply.github.com>
Pull request dashboard statusWaiting on the author · refreshed 2026-08-21 18:24 UTC Respond to 1 review item (e.g. link a commit, explain why not, ask a follow-up):
Status above doesn't look right?
|
xrmx
reviewed
Aug 17, 2026
Atishyy27
added a commit
to Atishyy27/opentelemetry-python-contrib
that referenced
this pull request
Aug 17, 2026
…xecute() Per xrmx's review on open-telemetry#4943: the regression test called the internal _operation_name method directly instead of going through actual instrumentation. Rewritten to instrument a real engine and execute comment-only/whitespace-only statements through it, asserting the resulting span names (falls back to just the db name, ':memory:', with no operation token) instead of calling the internal method.
…espace-only statements A statement that is truthy but has no tokens after leading-comment or whitespace stripping made .split()[0] raise IndexError in _operation_name. Guard it so such a statement falls back to the db name / vendor instead of crashing. Adds a regression test. Follow-up to open-telemetry#4934. Signed-off-by: Atishyy27 <142108881+Atishyy27@users.noreply.github.com>
Signed-off-by: Atishyy27 <142108881+Atishyy27@users.noreply.github.com>
…n the regression test Per henry3260's review on open-telemetry#4943: constructing EngineTracer directly registers SQLAlchemy event listeners on the engine that the test never removed. Clean them up via EngineTracer.remove_all_event_listeners() in a finally block so they don't leak past the test. Signed-off-by: Atishyy27 <142108881+Atishyy27@users.noreply.github.com>
…xecute() Per xrmx's review on open-telemetry#4943: the regression test called the internal _operation_name method directly instead of going through actual instrumentation. Rewritten to instrument a real engine and execute comment-only/whitespace-only statements through it, asserting the resulting span names (falls back to just the db name, ':memory:', with no operation token) instead of calling the internal method.
Atishyy27
force-pushed
the
fix/sqlalchemy-operation-name-empty-tokens
branch
from
August 20, 2026 23:57
626b010 to
f477eaf
Compare
xrmx
approved these changes
Aug 21, 2026
|
Hi @Atishyy27 — just a friendly reminder that this pull request is waiting on you. The dashboard status comment has the open items and is kept current.
|
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
3 participants
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.
Same class as #4934, in the SQLAlchemy instrumentation's
_operation_name: a comment-only / whitespace-only statement is truthy but has no tokens after leading-comment stripping, so.split()[0]raisesIndexError. Guards it so such a statement falls back to the db name / vendor instead of crashing.Type of change: Bug fix (non-breaking)
How Has This Been Tested? Added
test_operation_name_comment_or_whitespace_only(unit test on_operation_name); existing tests still pass. Verified locally.Does This PR Require a Core Repo Change? No.