Skip to content

Remove duplicate query logging that breaks Django's assertNumQueries#4367

Open
federicobond wants to merge 2 commits intoopen-telemetry:mainfrom
federicobond:fix/remove-redundant-query-log
Open

Remove duplicate query logging that breaks Django's assertNumQueries#4367
federicobond wants to merge 2 commits intoopen-telemetry:mainfrom
federicobond:fix/remove-redundant-query-log

Conversation

@federicobond
Copy link
Copy Markdown
Member

The SqlCommenter _QueryWrapper manually appends the SQL string to queries_log when a CursorDebugWrapper is active. However, CursorDebugWrapper.debug_sql already logs the query as a dict with "sql" and "time" keys. This causes two problems:

  1. Queries are double-counted in queries_log
  2. The raw string entries cause a TypeError in Django's assertNumQueries, which expects dict entries with a "sql" key

Remove the manual append and let Django handle query logging.

Type of change

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

How Has This Been Tested?

  • Tested against a Django project that was failing a test case:
         with self.assertNumQueries(14):
              ^^^^^^^^^^^^^^^^^^^^^^^^^
     E   TypeError: string indices must be integers, not 'str'

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

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

@federicobond federicobond force-pushed the fix/remove-redundant-query-log branch from 3f6ab47 to 14a2c7e Compare March 27, 2026 03:01
@federicobond federicobond requested a review from a team as a code owner March 27, 2026 03:01
The SqlCommenter _QueryWrapper manually appends the SQL string to
queries_log when a CursorDebugWrapper is active. However,
CursorDebugWrapper.debug_sql already logs the query as a dict with
"sql" and "time" keys. This causes two problems:

1. Queries are double-counted in queries_log
2. The raw string entries cause a TypeError in Django's
   assertNumQueries, which expects dict entries with a "sql" key

Remove the manual append and let Django handle query logging.
@federicobond federicobond force-pushed the fix/remove-redundant-query-log branch from 14a2c7e to 5b94cb1 Compare March 27, 2026 03:06
@tammy-baylis-swi tammy-baylis-swi moved this to Ready for review in Python PR digest Mar 27, 2026
@tammy-baylis-swi
Copy link
Copy Markdown
Contributor

Thanks for catching this! Could unit test coverage be added here on OTel Contrib side? Maybe it could borrow from your other-project test case that does the self.assertNumQueries(14).

@federicobond
Copy link
Copy Markdown
Member Author

@tammy-baylis-swi I tried but found it hard to come up with a meaningful test that it does NOT mess with the query log. Could be something like this, but still feels a bit artificial:

connection = MagicMock()
connection.queries_log = deque()
connection.force_debug_cursor = False

with CaptureQueriesContext(connection) as ctx:
    qw_instance = _QueryWrapper(requests_mock)
    execute_mock = MagicMock()
    context = {"cursor": MagicMock(), "connection": connection}
    qw_instance(
        execute_mock, "SELECT 1", MagicMock(), MagicMock(), context
    )

self.assertEqual(len(ctx), 0)

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

Labels

None yet

Projects

Status: Ready for review

Development

Successfully merging this pull request may close these issues.

2 participants