Skip to content

fix(dbapi): avoid IndexError in get_operation_name for comment/whites… - #4934

Open
Atishyy27 wants to merge 3 commits into
open-telemetry:mainfrom
Atishyy27:fix/dbapi-operation-name-empty-tokens
Open

fix(dbapi): avoid IndexError in get_operation_name for comment/whites…#4934
Atishyy27 wants to merge 3 commits into
open-telemetry:mainfrom
Atishyy27:fix/dbapi-operation-name-empty-tokens

Conversation

@Atishyy27

Copy link
Copy Markdown

Description

get_operation_name in the DB-API instrumentation strips a leading SQL comment and
takes the first token as the operation name. If a statement is truthy but has no tokens
left after stripping (a comment-only or whitespace-only statement), .split()[0] raises
IndexError, which propagates out of the instrumented execute() call and breaks it.

This guards the result the same way the adjacent _Template branch already does
(tokens[0] if tokens else "") and returns an empty operation name instead. It extends the
empty-string handling added in #2643 to the comment-only / whitespace-only case.

No open issue for this one — found while reading the operation-name parsing. Related: #2643.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Added test_operation_name_no_tokens_after_comment_strip in test_dbapi_integration.py,
which executes a comment-only (/* comment only */) and a whitespace-only (" ")
statement and asserts instrumentation does not raise. Verified locally that it passes and
that the existing test_span_succeeded still passes.

Does This PR Require a Core Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@Atishyy27
Atishyy27 requested a review from a team as a code owner August 6, 2026 23:59
@Atishyy27
Atishyy27 force-pushed the fix/dbapi-operation-name-empty-tokens branch from ae76d02 to d95c7b3 Compare August 7, 2026 00:00

@henry3260 henry3260 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.

Thanks for the fix! Just some nits.
I think we should add a changelog fragment under .changelog/ instead of editing CHANGELOG.md directly.

@Atishyy27

Copy link
Copy Markdown
Author

also moved the changelog to a .changelog/4934.fixed fragment per your note.

@chuenchen309

Copy link
Copy Markdown

Ran both this and #4936 against main at 038eb07ba. The failure reproduces, and the repo has already decided this case once, in a way that argues for both patches.

Reproduced. Calling get_operation_name directly, with a control group so the harness is honest:

dbapi.CursorTracer.get_operation_name (str branch)
  '/* comment only */'  -> IndexError: list index out of range
  '   '                 -> IndexError: list index out of range
  ''                    -> ''            (the #2643 guard)
  'SELECT 1'            -> 'SELECT'
  '/* c */ SELECT 1'    -> 'SELECT'

psycopg.CursorTracer.get_operation_name (Composable branch, #4936)
  same six inputs, same six results

get_operation_name is called unconditionally from _traced_execution, so this comes out of the instrumented execute() rather than being contained.

There is already a precedent in-tree, and it is the same fix. asyncpg hits the identical expression in three places (_do_execute, _do_cursor_execute, _do_prepared_execute) and each one is wrapped:

try:
    name = self._leading_comment_remover.sub("", name).split()[0]
except IndexError:
    name = ""

So "no tokens left after stripping ⇒ empty operation name" is already this repo's answer to this exact input; dbapi and psycopg are the packages that never got it.

Two sites are still uncovered after both PRs. Same probe, same clean checkout:

site on main
psycopg2/__init__.py:335get_operation_name, str branch raises on both inputs
sqlalchemy/engine.py:319_operation_name, parts.append(...) raises on both inputs
asyncpg ×3 guarded already

psycopg2 is the same override shape #4936 fixes, one package over. Not asking you to widen these PRs — you already asked for one logical change each — but if you are doing them one at a time, that is the rest of the list.

One thing neither PR changes, mentioned so it is not mistaken for a gap. The regex is ^/\*.*?\*/: anchored, no re.DOTALL, and it does not know about -- comments. So '-- just a note\n' does not raise — it returns '--', which becomes the span name. Degenerate output rather than a crash, so it is a different question from this PR, but "empty after stripping" is not the only way that parse ends badly.

AI-assisted; every result above is output from running the code on 038eb07ba, not read off the source.

@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Aug 14, 2026

Copy link
Copy Markdown

Pull request dashboard status

Waiting on maintainers · refreshed 2026-08-21 18:39 UTC

Merge when ready.

Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

@github-project-automation github-project-automation Bot moved this to Reviewed PRs that need fixes in Python PR digest Aug 17, 2026
Atishyy27 added a commit to Atishyy27/opentelemetry-python-contrib that referenced this pull request Aug 17, 2026
Per xrmx's review on open-telemetry#4934: the test executed both regression cases in one
loop, then asserted span count/names in a second pass after the loop ended,
so a subTest failure wouldn't isolate which case actually failed. Moves the
assertion inside each subTest and clears the exporter per iteration, so each
case checks exactly its own single span.
…pace-only statements

A statement that is truthy but has no tokens left after leading-comment or
whitespace stripping (e.g. a comment-only or whitespace-only query) made
`.split()[0]` raise IndexError, which propagates out of the instrumented
execute call. Guard the result the same way the `_Template` branch already
does and return an empty operation name instead. Adds a regression test.

Signed-off-by: Atishyy27 <142108881+Atishyy27@users.noreply.github.com>
… empty-token test

Signed-off-by: Atishyy27 <142108881+Atishyy27@users.noreply.github.com>
Per xrmx's review on open-telemetry#4934: the test executed both regression cases in one
loop, then asserted span count/names in a second pass after the loop ended,
so a subTest failure wouldn't isolate which case actually failed. Moves the
assertion inside each subTest and clears the exporter per iteration, so each
case checks exactly its own single span.
@Atishyy27
Atishyy27 force-pushed the fix/dbapi-operation-name-empty-tokens branch from 90e00cb to b3d2c41 Compare August 20, 2026 23:57
Atishyy27 added a commit to Atishyy27/opentelemetry-python-contrib that referenced this pull request Aug 20, 2026
…espace-only Composed statements

A Composed statement that is truthy but has no tokens after leading-comment or
whitespace stripping made `.split()[0]` raise IndexError in the psycopg
instrumentation's get_operation_name override. Guard it the same way the dbapi
base does, returning an empty operation name instead. Adds a regression test.
Follow-up to open-telemetry#4934.

Signed-off-by: Atishyy27 <142108881+Atishyy27@users.noreply.github.com>
Atishyy27 added a commit to Atishyy27/opentelemetry-python-contrib that referenced this pull request Aug 20, 2026
…tespace-only statements

A statement that is truthy but has no tokens after leading-comment or whitespace
stripping made .split()[0] raise IndexError in the psycopg2 instrumentation's
get_operation_name override. Guard it like the dbapi base does. Adds a regression
test. Follow-up to open-telemetry#4934.

Signed-off-by: Atishyy27 <142108881+Atishyy27@users.noreply.github.com>
Atishyy27 added a commit to Atishyy27/opentelemetry-python-contrib that referenced this pull request Aug 20, 2026
…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>
@opentelemetry-pr-dashboard

This comment has been minimized.

@Atishyy27

Copy link
Copy Markdown
Author

Addressed the review feedback and added the related fixes for psycopg, psycopg2, and SQLAlchemy. The PR is ready for maintainer review/merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Reviewed PRs that need fixes

Development

Successfully merging this pull request may close these issues.

4 participants