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

Raise an error rather than relying on Snuba telling us #69575

Closed
wants to merge 5 commits into from

Conversation

armenzg
Copy link
Member

@armenzg armenzg commented Apr 24, 2024

This PR raises an error if project_ids are not defined. It requires some extra work to fix a bunch of tests.
It's based on #69577

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 24, 2024
@armenzg armenzg force-pushed the feat/discover/complain_empty_projects/armenzg branch from 2e5f04e to ef8615e Compare April 24, 2024 15:30
@armenzg armenzg force-pushed the feat/discover/complain_empty_projects/armenzg branch from ef8615e to 6a8d5c5 Compare April 24, 2024 15:32
@armenzg armenzg added the WIP label Apr 24, 2024
Copy link

codecov bot commented Apr 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.65%. Comparing base (36b1430) to head (f34a3c3).
Report is 59 commits behind head on feat/discover/complain_empty_projects/armenzg.

Additional details and impacted files
@@                              Coverage Diff                               @@
##           feat/discover/complain_empty_projects/armenzg   #69575   +/-   ##
==============================================================================
  Coverage                                          79.65%   79.65%           
==============================================================================
  Files                                               6472     6472           
  Lines                                             287395   287475   +80     
  Branches                                           49536    49547   +11     
==============================================================================
+ Hits                                              228913   228999   +86     
+ Misses                                             58114    58108    -6     
  Partials                                             368      368           
Files Coverage Δ
src/sentry/search/events/builder/discover.py 96.30% <ø> (-0.01%) ⬇️
src/sentry/snuba/metrics/query_builder.py 93.78% <ø> (ø)
src/sentry/utils/snuba.py 93.62% <100.00%> (-0.15%) ⬇️

... and 24 files with indirect coverage changes

Base automatically changed from feat/discover/complain_empty_projects/armenzg to master April 25, 2024 11:58
armenzg added a commit that referenced this pull request Apr 25, 2024
…69577)

I discovered that we can make calls to Snuba with an empty list of `project_ids` (which is a required field) and that _always_ returns no data. This may not be happening in production, however, during development, this can take a lot of time to debug.

In production, we will raise an error rather than calling Snuba, thus, helping us find any issues in production.

During development, we will call Snuba _without_ `project_ids` which will cause an error from Snuba and help a developer notice the problem. We're doing this since there are a lot of tests in our codebase which require including the list of projects before calling Snuba (You can see some of the failures in #69575 ). We should eventually fix it, however, it requires significant effort.
@armenzg armenzg closed this Apr 30, 2024
@armenzg armenzg deleted the feat/raise-instead/armenzg branch April 30, 2024 12:14
@github-actions github-actions bot locked and limited conversation to collaborators May 15, 2024
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 WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant