Skip to content

fix(clickhouse): Don't eat the generator data #4669

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 7, 2025

Conversation

szokeasaurusrex
Copy link
Member

Currently, the Clickhouse integration consumes any data passed as a generator when reading it for insertion as db_params. Instead, since generators cannot be cloned, we need to wrap the generator to add the params as we iterate over it.

Fixes #4657


Thank you for contributing to sentry-python! Please add tests to validate your changes, and lint your code using tox -e linters.

Running the test suite on your PR might require maintainer approval.

@szokeasaurusrex szokeasaurusrex requested a review from a team as a code owner August 4, 2025 15:32
Copy link

codecov bot commented Aug 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.11%. Comparing base (84adbb7) to head (d9ee8c8).
⚠️ Report is 6 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4669      +/-   ##
==========================================
+ Coverage   85.09%   85.11%   +0.02%     
==========================================
  Files         156      156              
  Lines       15797    15803       +6     
  Branches     2671     2673       +2     
==========================================
+ Hits        13442    13451       +9     
+ Misses       1578     1577       -1     
+ Partials      777      775       -2     
Files with missing lines Coverage Δ
sentry_sdk/integrations/clickhouse_driver.py 94.38% <100.00%> (+0.40%) ⬆️

... and 1 file with indirect coverage changes

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/clickhouse-generator-bugfix branch 2 times, most recently from 3e3bd5e to e38354b Compare August 5, 2025 09:39
Explicitly list the `send_data` parameters in the wrapped function. The parameters are coming from [here](https://github.com/mymarilyn/clickhouse-driver/blob/8a4e7c5b99b532df2b015651d893a6f36288a22c/clickhouse_driver/client.py#L634). Continue also providing `*args` and `**kwargs`, but only for forwards-compatibility.

Also, don't take the `send_data` function as a parameter in the wrap function, rather just use it directly.
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/clickhouse-send_data-params branch from f76b786 to b0ae2d9 Compare August 5, 2025 11:28
Currently, the Clickhouse integration consumes any data passed as a generator when reading it for insertion as `db_params`. Instead, since generators cannot be cloned, we need to wrap the generator to add the params as we iterate over it.

Fixes #4657
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/clickhouse-generator-bugfix branch from e38354b to d9ee8c8 Compare August 5, 2025 11:28
Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for the fix!

Base automatically changed from szokeasaurusrex/clickhouse-send_data-params to master August 7, 2025 15:37
@szokeasaurusrex szokeasaurusrex merged commit 3ef02a1 into master Aug 7, 2025
137 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/clickhouse-generator-bugfix branch August 7, 2025 15:37
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.

clickhouse-driver integration eats inserted data when generator is used
2 participants