Skip to content

Conversation

@sl0thentr0py
Copy link
Member

we need to call the original function first for db_params to be correctly populated in the generator case

(see diff without whitespace)

@sl0thentr0py sl0thentr0py requested a review from a team as a code owner August 26, 2025 14:20
**kwargs: Any,
) -> Any:
span = getattr(self.connection, "_sentry_span", None)
if span is None:
Copy link
Member Author

Choose a reason for hiding this comment

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

moved this to an early return

Comment on lines +178 to +188
rv = original_send_data(
self, sample_block, data, types_check, columnar, *args, **kwargs
)

if should_send_default_pii() and db_params:
# need to do this after the original function call to make sure
# db_params is populated correctly
saved_db_data["db.params"] = db_params
span.set_attribute("db.params", _serialize_span_attribute(db_params))

return rv

Choose a reason for hiding this comment

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

Potential bug: If original_send_data fails, the exception prevents span attributes from being set, losing debugging context for the failed database operation.
  • Description: If the original_send_data call raises an exception, execution is halted before span attributes like db.params can be set. This is problematic because the integration's purpose is to provide observability, which is most critical during failures. The current implementation leads to a loss of valuable debugging context when a database operation fails. For operations involving generators, any partially collected parameters are also lost, further hindering debugging efforts. The span itself may also not be finished correctly, leading to incomplete traces.

  • Suggested fix: Wrap the original_send_data call in a try...finally block. The logic to set span attributes like db.params should be moved into the finally block. This ensures that the debugging information is attached to the span even if original_send_data raises an exception, after which the exception can propagate normally.
    severity: 0.7, confidence: 0.95

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

@sl0thentr0py sl0thentr0py merged commit 42cac90 into potel-base Aug 26, 2025
123 of 124 checks passed
@sl0thentr0py sl0thentr0py deleted the neel/fix-clickhouse branch August 26, 2025 14:31
@codecov
Copy link

codecov bot commented Aug 26, 2025

Codecov Report

❌ Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.78%. Comparing base (8d64be5) to head (f58461c).
⚠️ Report is 2 commits behind head on potel-base.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
sentry_sdk/integrations/clickhouse_driver.py 90.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           potel-base    #4722      +/-   ##
==============================================
- Coverage       84.80%   84.78%   -0.03%     
==============================================
  Files             158      158              
  Lines           15812    15816       +4     
  Branches         2518     2519       +1     
==============================================
- Hits            13410    13409       -1     
- Misses           1636     1642       +6     
+ Partials          766      765       -1     
Files with missing lines Coverage Δ
sentry_sdk/integrations/clickhouse_driver.py 88.88% <90.00%> (+3.62%) ⬆️

... and 3 files with indirect coverage changes

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants