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.

Sign up for GitHub

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

Jump to bottom

feat(CapMan): Pass tenant_ids to Snuba #44788

Merged
merged 10 commits into from
Feb 28, 2023
Merged

feat(CapMan): Pass tenant_ids to Snuba #44788

merged 10 commits into from
Feb 28, 2023

Conversation

rahul-kumar-saini
Copy link
Contributor

@rahul-kumar-saini rahul-kumar-saini commented Feb 16, 2023 •
edited
Loading

Overview

  • As part of the Capacity Management project over on S3, we're working on getting more information out of per Snuba Request in order to properly categorize and eventually set precise limits on API usage by a number of different variables (we're calling them tenants)

Changes

  • The first two tenant IDs we're implementing are organization_id and referrer (referrer already existed but this PR adds it to the dictionary)
  • This PR updates the Snuba SDK version in Sentry and starts the process of consolidating these tenants into a single dictionary called tenant_ids per request
  • I've extracted organization_id from some easy use cases of the Snuba API and will be working on more complicated ones in future PRs

Notes

  • Eventually, we will need Sentry's backend teams to help figure out how to get more of these tenant IDs into the requests if/when things get too complicated

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Feb 16, 2023
@rahul-kumar-saini rahul-kumar-saini marked this pull request as ready for review February 27, 2023 21:26
@rahul-kumar-saini rahul-kumar-saini requested review from a team as code owners February 27, 2023 21:26
@rahul-kumar-saini rahul-kumar-saini requested a review from a team February 27, 2023 21:26
@rahul-kumar-saini rahul-kumar-saini requested a review from a team as a code owner February 27, 2023 21:26
@rahul-kumar-saini rahul-kumar-saini requested a review from a team as a code owner February 28, 2023 00:33
volokluev
volokluev approved these changes Feb 28, 2023
View reviewed changes
Copy link
Member

@volokluev volokluev left a comment

Choose a reason for hiding this comment

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

just one question

evanh
evanh reviewed Feb 28, 2023
View reviewed changes
JoshFerge
JoshFerge approved these changes Feb 28, 2023
View reviewed changes
Copy link
Member

@JoshFerge JoshFerge left a comment

Choose a reason for hiding this comment

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

seems safe. couple questions on consistency:

  • why are some referrers defined in a constants file and others not?
  • why do some referrers use underscores and others dashes?

@rahul-kumar-saini
Copy link
Contributor Author

@JoshFerge

seems safe. couple questions on consistency:

  • why are some referrers defined in a constants file and others not?
  • why do some referrers use underscores and others dashes?

I only refactored existing referrers within the same function and added 1 new one following the same convention as another referrer in the same file.

I'm not sure what the guidance around referrers is but it seems most implementations have their own version, this inconsistency existed before I made any changes.

@rahul-kumar-saini rahul-kumar-saini merged commit d7a91df into master Feb 28, 2023
@rahul-kumar-saini rahul-kumar-saini deleted the rahul/feat/tenant_ids_to_snuba branch February 28, 2023 23:16
jan-auer added a commit that referenced this pull request Mar 1, 2023
@jan-auer
Merge branch 'master' into feat/crons-ingest-kafka
6e92ff2 
* master: (37 commits)
  ref(ppf): Don't use --commit-batch-size for futures queue length (#45182)
  feat(codecov-v2): Add more logging (#45225)
  fix(alerts): Center table items on alert history page (#45226)
  feat(CapMan): Pass `tenant_ids` to Snuba (#44788)
  ref(db): Drop `project_id` from Environment (model state) (#45207)
  chore(profiling): Rename context in profiles task (#45208)
  feat(replays): Improve index page query performance (#45098)
  chore(issue assignment): Add logging for`GroupOwner` auto assignment (#45142)
  fix(hybrid-cloud): Uncache organization when queueing it for deletion (#45213)
  fix(perf): Navigating to Transaction Summary from Trends widget should persist custom date selection (#45190)
  fix(pageFilter): Fix overflow (#45169)
  ref(git hooks): Only suggest autoupdate variable when pulling if not already set (#45179)
  fix(dashboard): Include dashboard filters in widget viewer (#45106)
  fix(alerts): Remove null projects from alerts list (#45202)
  feat(replay): Update Inline replay onboarding img to support dark mode (#45084)
  __iexact reduce call has default value now. (#45206)
  feat(replay): Use SDK value for LCP (#44868)
  chore(hybrid-cloud): breaking foreign keys (#45203)
  Revert "ref(db): Drop `project_id` from Environment (model state) (#45094)"
  ref(db): Drop `project_id` from Environment (model state) (#45094)
  ...
@rahul-kumar-saini rahul-kumar-saini mentioned this pull request Mar 1, 2023
Merged
rahul-kumar-saini added a commit that referenced this pull request Mar 2, 2023
@rahul-kumar-saini
feat(CapMan): More tenant_ids for TSDB Queries (#45274)
95bafd2 
### Overview
- Added `organization_id` to more Snuba Requests, specifically targeting
`tsdb-modelid:4` queries as they are the biggest chunk of queries Snuba
is handling in production
- Most calls to `tsdb.get_range` handled as well, some I wasn't able to
figure out
- More context: #44788
This was referenced Mar 3, 2023
Merged
Merged
rahul-kumar-saini added a commit that referenced this pull request Mar 7, 2023
@rahul-kumar-saini
feat(CapMan): More tsdb and legacy queries with tenant_ids (#45405)
0183b07 
### Overview
- Added `organization_id` to more Snuba Requests, specifically targeting
`tsdb-modelid:300` and `tsdb-modelid:100`
- Legacy queries now pass `tenant_ids` to Snuba as well
- Removed hardcoded referrer in tenant_ids, moved to populate referrer
in query functions
- More context: #44788
@rahul-kumar-saini rahul-kumar-saini mentioned this pull request Mar 7, 2023
Merged
rahul-kumar-saini added a commit that referenced this pull request Mar 8, 2023
@rahul-kumar-saini
feat(CapMan): Tagstore Org IDs passed to Snuba (#45454)
ddd37dc 
- All Tagstore Snuba queries now pass the `organization_id` tenant_id in
the request
- See #44788 for more context
@rahul-kumar-saini rahul-kumar-saini mentioned this pull request Mar 8, 2023
Merged
rahul-kumar-saini added a commit that referenced this pull request Mar 14, 2023
@rahul-kumar-saini
feat(CapMan): QueryBuilders + Search tenant_ids (#45505)
911d002 
### Overview
- All Snuba queries built and executed with `QueryBuilder` objects now
contain org ID in `tenant_ids` for their Snuba Requests
- Outcomes Timeseries Queries now have tenant_ids
- Search queries now have tenant_ids
- See #44788 for more context
rahul-kumar-saini added a commit that referenced this pull request Mar 15, 2023
@rahul-kumar-saini
feat(CapMan): Eventstore query tenant_ids (#45550)
e748f02 
### Overview
- All Eventstore queries now contain `tenant_ids`
- See #44788 for more context
@rahul-kumar-saini rahul-kumar-saini mentioned this pull request Mar 15, 2023
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@evanh evanh evanh left review comments

@JoshFerge JoshFerge JoshFerge approved these changes

@volokluev volokluev volokluev approved these changes

Assignees
No one assigned
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rahul-kumar-saini @JoshFerge @evanh @volokluev