Skip to content
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

ref(on-demand): Improve query builder integration #54363

Merged
merged 5 commits into from
Aug 9, 2023

Conversation

iambriccardo
Copy link
Member

@iambriccardo iambriccardo commented Aug 8, 2023

This PR improves the integration of the on demand metrics into the query builder by introducing a new memoized property called _on_demand_metric_spec which will fetch, irrespectively of the constructor's lifecycle, all the required data for a successful execution.
In addition, this PR adds some better tests for the query generation, since it's a very delicate piece of code.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 8, 2023
@iambriccardo iambriccardo marked this pull request as ready for review August 8, 2023 12:34
@iambriccardo iambriccardo requested a review from a team as a code owner August 8, 2023 12:34
@iambriccardo iambriccardo requested a review from a team August 8, 2023 12:34
Copy link
Member

@obostjancic obostjancic left a comment

Choose a reason for hiding this comment

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

Great! 🚢

@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Merging #54363 (8fd7767) into master (cac31d6) will increase coverage by 0.01%.
Report is 52 commits behind head on master.
The diff coverage is 87.80%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #54363      +/-   ##
==========================================
+ Coverage   79.64%   79.66%   +0.01%     
==========================================
  Files        4986     4989       +3     
  Lines      211350   211579     +229     
  Branches    36032    36065      +33     
==========================================
+ Hits       168332   168548     +216     
- Misses      37833    37839       +6     
- Partials     5185     5192       +7     
Files Changed Coverage Δ
src/sentry/search/events/builder/metrics.py 81.86% <87.50%> (-0.62%) ⬇️
src/sentry/search/events/builder/discover.py 91.86% <100.00%> (+<0.01%) ⬆️

... and 124 files with indirect coverage changes

@iambriccardo iambriccardo merged commit d7e2588 into master Aug 9, 2023
55 of 56 checks passed
@iambriccardo iambriccardo deleted the riccardo/ref/query-builder-on-demand branch August 9, 2023 07:58
@iambriccardo iambriccardo added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Aug 9, 2023
@getsentry-bot
Copy link
Contributor

PR reverted: 56fd332

getsentry-bot added a commit that referenced this pull request Aug 9, 2023
This reverts commit d7e2588.

Co-authored-by: iambriccardo <15819674+iambriccardo@users.noreply.github.com>
@iambriccardo iambriccardo restored the riccardo/ref/query-builder-on-demand branch August 9, 2023 09:11
@github-actions github-actions bot locked and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components Trigger: Revert Add to a merged PR to revert it (skips CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants