Skip to content

Conversation

@rahul-kumar-saini
Copy link
Contributor

Overview

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 8, 2023
@rahul-kumar-saini rahul-kumar-saini marked this pull request as ready for review March 14, 2023 17:47
@rahul-kumar-saini rahul-kumar-saini requested a review from a team as a code owner March 14, 2023 17:47
@rahul-kumar-saini rahul-kumar-saini requested review from a team March 14, 2023 17:47
Comment on lines 323 to 331
query: QueryDefinition, tenant_ids: dict[str, Any] | None = None
query: QueryDefinition,
tenant_ids: dict[str, Any] | None = None,
referrer: str = "outcomes.timeseries",
) -> ResultSet:
"""
Runs an outcomes query. By default the referrer is `outcomes.timeseries` and this should not change
unless there is a very specific reason to do so. Eg. getsentry uses this function for billing
metrics, so the referrer is different as it's no longer a "product" query.
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can PR this separately but it's a quick change not very relevant to the entire PR. Previously, I added tenant_ids to all outcomes.timeseries and outcomes.totals queries but I was still seeing timeseries queries coming without this data. Turns out this function is used in getsentry for a billing metric, and there should should really be a different referrer for that use case, so I made referrer an optional param with a quick description of why.

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.

everything looks okay to me except for the one in reprocessing2

batch_size=settings.SENTRY_REPROCESSING_PAGE_SIZE,
state=query_state,
referrer="reprocessing2.reprocess_group",
tenant_ids={"organization_id": models.Project.objects.get(id=project_id).organization_id},
Copy link
Member

Choose a reason for hiding this comment

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

this one could be problematic as this is an ingest flow. would check with @untitaker on if this is okay to do here.

Copy link
Member

Choose a reason for hiding this comment

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

reprocessing is not perf sensitive, but if you can, use get_from_cache across the board just because it's low-effort

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.

lgtm

@rahul-kumar-saini rahul-kumar-saini merged commit e748f02 into master Mar 15, 2023
@rahul-kumar-saini rahul-kumar-saini deleted the rahul/feat/eventstore_tenant_ids branch March 15, 2023 20:33
@github-actions github-actions bot locked and limited conversation to collaborators Mar 31, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants