Skip to content

Conversation

@wmak
Copy link
Member

@wmak wmak commented Nov 20, 2025

  • Also add error logging to top events
  • This adds a log event on events queries

- Also add error logging to top events
- This adds a log event on events queries
@wmak wmak requested a review from a team as a code owner November 20, 2025 20:58
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 20, 2025
Comment on lines +820 to +824
except Exception as e:
# add the rpc to the error so we can include it in the response
if params.debug:
setattr(e, "debug", MessageToJson(rpc_request))
raise
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The rpc_request variable is incorrectly reassigned in a loop, causing the exception handler and set_debug_meta to use the wrong request's debug information.
Severity: HIGH | Confidence: High

🔍 Detailed Analysis

The rpc_request variable is reassigned within the debug logging loop (lines 813-814). After the loop, rpc_request incorrectly holds the last item from the requests list (the other_request when include_other=True). This leads to incorrect debug information being attached to exceptions (lines 820-824) and in set_debug_meta (line 846) when params.debug=True and include_other=True. If the first request causes an error, the debug info will misleadingly show the second request's details, hindering effective debugging.

💡 Suggested Fix

Rename the loop variable rpc_request to avoid reassigning the outer scope's rpc_request. Alternatively, store the rpc_request that caused the error in a separate variable before the try block, or ensure the correct rpc_request is accessible in the except block.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/snuba/rpc_dataset_common.py#L820-L824

Potential issue: The `rpc_request` variable is reassigned within the debug logging loop
(lines 813-814). After the loop, `rpc_request` incorrectly holds the last item from the
`requests` list (the `other_request` when `include_other=True`). This leads to incorrect
debug information being attached to exceptions (lines 820-824) and in `set_debug_meta`
(line 846) when `params.debug=True` and `include_other=True`. If the first request
causes an error, the debug info will misleadingly show the second request's details,
hindering effective debugging.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2862835

except Exception as e:
# add the rpc to the error so we can include it in the response
if params.debug:
setattr(e, "debug", MessageToJson(rpc_request))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Wrong request attached to exception in error handler

The exception handler references rpc_request from the logging loop above, which contains only the last request from the requests list. When multiple requests are sent (e.g., when include_other is True), the wrong request gets attached to the exception. The error could be caused by any request in the list, but only the last one from the logging loop is captured.

Fix in Cursor Fix in Web

if len(timeseries_rpc_response) > 1:
other_response = timeseries_rpc_response[1]
if params.debug:
for rpc_request in requests:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Variable shadowing causes wrong request in debug metadata

The logging loop reuses the variable name rpc_request, shadowing the first request that was assigned on line 786. This causes set_debug_meta on line 832 to receive the wrong request (the last one from the loop instead of the first one), mismatching it with rpc_response which is always the first response.

Fix in Cursor Fix in Web

@codecov
Copy link

codecov bot commented Nov 20, 2025

Codecov Report

❌ Patch coverage is 5.55556% with 17 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/snuba/rpc_dataset_common.py 5.55% 17 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #103766      +/-   ##
===========================================
- Coverage   80.62%    80.60%   -0.02%     
===========================================
  Files        9278      9278              
  Lines      396075    396191     +116     
  Branches    25247     25247              
===========================================
+ Hits       319348    319369      +21     
- Misses      76267     76362      +95     
  Partials      460       460              

@wmak wmak merged commit e7a2b37 into master Nov 20, 2025
68 checks passed
@wmak wmak deleted the wmak/feat/add-logging-to-debug-queries branch November 20, 2025 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants