fix(releases): Fix environment filtering for per-project new groups count in old releases serializer

[srest2021]

Previously, the per-project new groups count was being populated by ReleaseProject's new_groups. However, this count becomes incorrect when filtering by environment. Here we populate the per-project new groups count with the counts obtained from ReleaseProjectEnvironment when available. This fix only applies to the old serializer.

We also fix a Django aggregation that calculates the new groups counts when environments are present. Previously the query was grouping once per row, and ordering by first seen, like so:
... GROUP BY "sentry_releaseprojectenvironment"."id" ORDER BY "sentry_releaseprojectenvironment"."first_seen" DESC

But if we try to filter by multiple environments, this won't work because we can have multiple rows for a single (release, project) pairing, and whichever environment is ordered last will "win" and have its values set as the new groups counts.

In this PR, we make a new helper function that skips the unnecessary ordering, and we modify the query to group only by project and release id. We also add test coverage for new groups counts.

[srest2021]

@sentry review

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #101003      +/-   ##
===========================================
+ Coverage   76.52%    81.14%   +4.62%     
===========================================
  Files        8653      8658       +5     
  Lines      384048    384155     +107     
  Branches    24249     24186      -63     
===========================================
+ Hits       293880    311721   +17841     
+ Misses      89824     72090   -17734     
  Partials      344       344              

[cmanallen]

Ordering is not required and specifying order_by with no arguments does not order the results.

[cmanallen]

This can be removed since you're calling values_list below.

[srest2021]

Hi @cmanallen here is my explanation for why I changed the query (sorry for the wall of text)!

The old query will sometimes fail when we filter by multiple environments.

Assuming we’re working in release 1.0.0 (id “r123”) and we have 4 new groups for project A (id “a123”) and 2 new groups for project B (id "b123”):

• For project A: 3 issues in production, 1 issue in staging (total = 4)
• For project B: 2 issues in production, 0 issues in staging (total = 2)

This is the same setup as in test_new_groups_environment_filtering. Say we are filtering by the production and staging environments.

The old query will generate SQL like this:
SELECT "sentry_releaseprojectenvironment"."project_id" AS "project_id", "sentry_releaseprojectenvironment"."release_id" AS "release_id", SUM("sentry_releaseprojectenvironment"."new_issues_count") AS "aggregated_new_issues_count" FROM "sentry_releaseprojectenvironment" INNER JOIN "sentry_environment" ON ("sentry_releaseprojectenvironment"."environment_id" = "sentry_environment"."id") WHERE ("sentry_releaseprojectenvironment"."release_id" IN (r123) AND "sentry_environment"."name" IN (production, staging)) GROUP BY "sentry_releaseprojectenvironment"."id" ORDER BY "sentry_releaseprojectenvironment"."first_seen" DESC

Note that there’s an order-by at the very end despite no ordering being included in the code, and that we’re grouping by id. This will essentially group every single row individually.

We get a query set like so: <BaseQuerySet [(b123, r123, 0), (b123, r123, 2), (a123, r123, 1), (a123, r123, 3)]>

And as expected we get one item per row in the ReleaseProjectEnvironment table:

• (b123, r123, 0) is # new groups in staging for project B
• (b123, r123, 2) is # new groups in prod for project B
• (a123, r123, 1) is # new groups in staging for project A
• (a123, r123, 3) is # new groups in prod for project A

And when we calculate group_counts_by_release, we get this: {r123: {b123: 2, a123: 3}} instead of the correct counts: {r123: {b123: 2, a123: 4}}. Because we’re grouping each row individually, when we run the for project_id, release_id, new_groups loop on this queryset, all the rows for a project will overwrite each other and the last row for that project will “win”. So for example, because (a123, r123, 3) was the last row for project a, that’s why we get 3 new groups for project a instead of the correct total 3+1=4. That's why the old query will fail only if we want multiple environments / multiple rows per project/release pair.

So instead of grouping by row we want to group by (project_id, release_id). If we add .values("project_id", "release_id”) to the query, we get this SQL:
SELECT "sentry_releaseprojectenvironment"."project_id" AS "project_id", "sentry_releaseprojectenvironment"."release_id" AS "release_id", SUM("sentry_releaseprojectenvironment"."new_issues_count") AS "aggregated_new_issues_count" FROM "sentry_releaseprojectenvironment" INNER JOIN "sentry_environment" ON ("sentry_releaseprojectenvironment"."environment_id" = "sentry_environment"."id") WHERE ("sentry_releaseprojectenvironment"."release_id" IN (r123) AND "sentry_environment"."name" IN (production, staging)) GROUP BY 1, 2, "sentry_releaseprojectenvironment"."first_seen" ORDER BY "sentry_releaseprojectenvironment"."first_seen" DESC

Note that for some reason we’re also grouping by first_seen and we’re still ordering by first_seen as well. This query will give us the same exact query set and group_counts_by_release. I suspect it’s because we’re still not grouping only by project and release id.

Now if we also add .order_by() to the query, in addition to .values("project_id", "release_id”), we get this:
SELECT "sentry_releaseprojectenvironment"."project_id" AS "project_id", "sentry_releaseprojectenvironment"."release_id" AS "release_id", SUM("sentry_releaseprojectenvironment"."new_issues_count") AS "aggregated_new_issues_count" FROM "sentry_releaseprojectenvironment" INNER JOIN "sentry_environment" ON ("sentry_releaseprojectenvironment"."environment_id" = "sentry_environment"."id") WHERE ("sentry_releaseprojectenvironment"."release_id" IN (r123) AND "sentry_environment"."name" IN (production, staging)) GROUP BY 1, 2

Now we’re only grouping by project and release id. I think this is because adding order_by() clears whatever default ordering was being added on top of the original query.

And with this query we finally get the correct query set: <BaseQuerySet [(a123, r123, 4), (b123, r123, 2)]> and the correct group_counts_by_release: {r123: {b123: 2, a123: 4}}

Let me know if this makes sense to you.

[cmanallen]

@srest2021 The .values() method alters the query's result type. It does not modify the query. Using a bare order_by does reset ordering but ask yourself the question, why should ordering matter at all for this function?

Let's abstract the queries into their own functions. Then relentlessly unit test the functions and ensure they are deterministic and that you totally and completely understand what each component of the function is returning and being transformed to. Retrieving counts should not be order dependent otherwise we're making a mistake in calculation.

Radical simplicity should be your goal. Strip away everything. Start from scratch. They are small queries. Re-write them in their most ideal form.

[cursor]

Bug: N+1 Query Issue in Release Data Retrieval

The new _get_release_project_envs_unordered method, used when environments are specified, omits select_related("project"). This means __get_release_data_with_environments will trigger N+1 database queries when accessing the project relation.

 

[srest2021]

Is this not just accessing project id? We don't access any other attributes of release_project_envs.project.

[cmanallen]

Why are we joining the release model? This call to select_related seems unnecessary.

[srest2021]

We access release_project_env.release.version in other parts of __get_release_data_with_environments, for example:

sentry/src/sentry/api/serializers/models/release.py

Line 507 in 6f7b903

release_project_env.release.version not in first_seen