feat(proguard): associate release with proguard mapping files #48511

buenaflor · 2023-05-04T11:27:04Z

Ref: getsentry/sentry-android-gradle-plugin#40

This allows to create a weak association between a release and a proguard mapping file

…ng-files

github-actions · 2023-05-04T11:30:42Z

This PR has a migration; here is the generated SQL for src/sentry/migrations/0436_add_proguard_release_association.py ()

--
-- Create model ProguardArtifactRelease
--
CREATE TABLE "sentry_proguardartifactrelease" ("id" bigserial NOT NULL PRIMARY KEY, "organization_id" bigint NOT NULL, "project_id" bigint NOT NULL, "release_name" varchar(250) NOT NULL, "proguard_uuid" uuid NOT NULL, "date_added" timestamp with time zone NOT NULL);
CREATE UNIQUE INDEX CONCURRENTLY "sentry_proguardartifactr_organization_id_project__0a751b1a_uniq" ON "sentry_proguardartifactrelease" ("organization_id", "project_id", "release_name");
ALTER TABLE "sentry_proguardartifactrelease" ADD CONSTRAINT "sentry_proguardartifactr_organization_id_project__0a751b1a_uniq" UNIQUE USING INDEX "sentry_proguardartifactr_organization_id_project__0a751b1a_uniq";
CREATE INDEX CONCURRENTLY "sentry_proguardartifactrelease_organization_id_bb2944ba" ON "sentry_proguardartifactrelease" ("organization_id");
CREATE INDEX CONCURRENTLY "sentry_proguardartifactrelease_project_id_d890d161" ON "sentry_proguardartifactrelease" ("project_id");

codecov · 2023-05-04T11:59:06Z

Codecov Report

Merging #48511 (999226d) into master (e295002) will decrease coverage by 0.02%.
The diff coverage is 97.29%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #48511      +/-   ##
==========================================
- Coverage   80.94%   80.93%   -0.02%     
==========================================
  Files        4819     4819              
  Lines      201975   202326     +351     
  Branches    11446    11500      +54     
==========================================
+ Hits       163492   163750     +258     
- Misses      38228    38321      +93     
  Partials      255      255

Impacted Files	Coverage Δ
src/sentry/api/urls.py	`100.00% <ø> (ø)`
src/sentry/api/endpoints/debug_files.py	`86.60% <95.83%> (+1.03%)`	⬆️
src/sentry/models/debugfile.py	`74.56% <100.00%> (+0.99%)`	⬆️

... and 54 files with indirect coverage changes

…ng-files

github-actions · 2023-05-09T12:43:16Z

This PR has a migration; here is the generated SQL for src/sentry/migrations/0438_add_proguard_release_association.py ()

--
-- Create model ProguardArtifactRelease
--
CREATE TABLE "sentry_proguardartifactrelease" ("id" bigserial NOT NULL PRIMARY KEY, "organization_id" bigint NOT NULL, "project_id" bigint NOT NULL, "release_name" varchar(250) NOT NULL, "proguard_uuid" uuid NOT NULL, "date_added" timestamp with time zone NOT NULL);
CREATE UNIQUE INDEX CONCURRENTLY "sentry_proguardartifactr_organization_id_project__0a751b1a_uniq" ON "sentry_proguardartifactrelease" ("organization_id", "project_id", "release_name");
ALTER TABLE "sentry_proguardartifactrelease" ADD CONSTRAINT "sentry_proguardartifactr_organization_id_project__0a751b1a_uniq" UNIQUE USING INDEX "sentry_proguardartifactr_organization_id_project__0a751b1a_uniq";
CREATE INDEX CONCURRENTLY "sentry_proguardartifactrelease_organization_id_bb2944ba" ON "sentry_proguardartifactrelease" ("organization_id");
CREATE INDEX CONCURRENTLY "sentry_proguardartifactrelease_project_id_d890d161" ON "sentry_proguardartifactrelease" ("project_id");

markstory · 2023-05-10T14:25:38Z

src/sentry/api/endpoints/debug_files.py

+        proguard_uuid = request.data.get("proguard_uuid")
+        if not all([release_name, proguard_uuid]):
+            return Response(
+                data={"error": "Missing required fields"}, status=status.HTTP_400_BAD_REQUEST


Would be good to let the user know which fields are required.

markstory · 2023-05-10T14:28:41Z

src/sentry/api/endpoints/debug_files.py

+            )
+        releases = releases.values()
+
+        return Response(list(releases))


I would be wary of top level lists in response payloads. It makes API responses much harder to evolve as you can't add new keys for additional metadata. For newer endpoints we have been making the response bodies a dictionary with a single key for the results.

markstory · 2023-05-10T14:30:32Z

src/sentry/models/debugfile.py

+    organization_id = BoundedBigIntegerField(db_index=True)
+    project_id = BoundedBigIntegerField(db_index=True)


While it can be done in a future pull request. It would be good to have this model added to project deletions so that when customers remove a project proguard metadata is also removed.

wedamija · 2023-05-10T16:44:57Z

src/sentry/models/debugfile.py

+
+    organization_id = BoundedBigIntegerField(db_index=True)
+    project_id = BoundedBigIntegerField(db_index=True)
+    release_name = models.CharField(max_length=250)


Why do we use the release name here vs a link to the actual release?

This is wanted, the goal here, like with sourcemaps, is to have a weak release association. With this association, you will optimistically tell the user to specify a future release that will be created later on down the pipeline. Ofc this won't result in any database consistency mechanisms helping us to maintain data consistency but we aligned on such an approach.

Swatinem · 2023-07-12T09:07:41Z

src/sentry/migrations/0507_add_proguard_release_association.py

+            ],
+            options={
+                "db_table": "sentry_proguardartifactrelease",
+                "unique_together": {("organization_id", "project_id", "release_name")},


The definition here is not uptodate with the one in the model file. In particular, proguard_uuid is missing here.
Also, the project implies the org, so putting it into this uniqueness constraint does not make too much sense.

Swatinem · 2023-07-12T09:09:56Z

src/sentry/models/debugfile.py

+    organization_id = BoundedBigIntegerField(db_index=True)
+    project_id = BoundedBigIntegerField(db_index=True)
+    release_name = models.CharField(max_length=250)
+    proguard_uuid = models.UUIDField()


As this is "just a uuid", it does not have a link back to the uploaded proguard file.
We are working on eventually expiring and auto-removing expiring debug files, which also applies to proguard files.

So the underlying proguard projectdsymfile may be going away, and you will be left with some "dead" rows here.

This detail highly depends on the reading assumptions that the application layer does. Seeing how information is queried, what @Swatinem says it's very good. Either we enforce constraints at the db level or we make sure that the application will do it.

I personally would prefer to link to the file that contains the proguard_uuid and have the db handle the consistency requirements. Of course you would also keep the proguard_uuid, similarly to how the DebugIdArtifactBundle table is designed.

I would definitely link to the ProjectDebugFile, and also query it on post. Right now you can just post any random uuid / release_name, and there is no validation whatsoever, and no cleanup. So malicious users could just spam random uuids to fill up the database with junk.

Yup, makes sense, thx for taking a look

The problem still exists, but to a lesser extent with loose release association, also relevant for artifact bundles (@iambriccardo):
malicious users could spam random release names and fill up the database.
However if you have a strong relationship to the ProjectDebugFile, or an ArtifactBundle, that junk is at least being cleaned up once the debug file expires and is being cleaned up.

iambriccardo

Nice work, I would now try to figure out how to deal with the File object which contains the proguard_uuid.

iambriccardo · 2023-07-12T10:36:17Z

src/sentry/models/debugfile.py

+    organization_id = BoundedBigIntegerField(db_index=True)
+    project_id = BoundedBigIntegerField(db_index=True)
+    release_name = models.CharField(max_length=250)
+    proguard_uuid = models.UUIDField()


This detail highly depends on the reading assumptions that the application layer does. Seeing how information is queried, what @Swatinem says it's very good. Either we enforce constraints at the db level or we make sure that the application will do it.

I personally would prefer to link to the file that contains the proguard_uuid and have the db handle the consistency requirements. Of course you would also keep the proguard_uuid, similarly to how the DebugIdArtifactBundle table is designed.

iambriccardo · 2023-07-12T10:42:16Z

src/sentry/models/debugfile.py

+class ProguardArtifactRelease(Model):  # type: ignore
+    __include_in_export__ = False
+
+    organization_id = BoundedBigIntegerField(db_index=True)


Regarding indexing, I would suggest to reason about the size of the result set once an index run is done, that is, if I use the org_id in the index, will I get a very small amount of rows to filter sequentially? This depends on data distribution and I have no idea about that, so you might try to think what is the best approach here.

I would expect that having an index on project and org as you did is fine, unless the proguard_uuid has mostly unique entries, in that case, it would be better to add an index there.

Of course the best solution would be to use a composite index but it would be useful only in case we always query nearly all composite fields and each field has a very high cardinality, in the other cases it will just occupy more space on disk.

In this case proguard uuids are mostly unique. Thx for the insight

asottile-sentry · 2023-07-12T15:03:50Z

src/sentry/models/debugfile.py

@@ -363,6 +363,24 @@ def _analyze_progard_filename(filename: str) -> Optional[str]:
        return None


+@region_silo_only_model
+class ProguardArtifactRelease(Model):  # type: ignore


what's the # type: ignore here? mypy should be fine with subclassing Model now

Yup should not be there, thx

iambriccardo

LGTM, great work!

…ng-files

buenaflor and others added 10 commits April 27, 2023 17:02

feat(frontend): add associated releases to proguardRow

8eed285

feat(proguard): add proguard weak association

b6681e6

Merge branch 'master' into feat/associate-release-with-proguard-mappi…

c20278c

…ng-files

feat(proguard): update migration

b2d3878

feat(proguard): update naming

f63ae28

feat(proguard): catch IntegrityError

4f7ed19

Merge branch 'master' into feat/associate-release-with-proguard-mappi…

eb10b62

…ng-files

style(lint): Auto commit lint changes

472f0c4

Merge branch 'master' into feat/associate-release-with-proguard-mappi…

f946523

…ng-files

revert frontend changes

30a1de1

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label May 4, 2023

fix(migrations): dependencies

a0dbe47

fix(typing): ignore model type

b46aecb

vercel bot deployed to Preview May 4, 2023 11:37 View deployment

tests: add test for endpoint

5f6c002

vercel bot deployed to Preview May 4, 2023 13:55 View deployment

buenaflor added 3 commits May 9, 2023 12:39

Merge branch 'master' into feat/associate-release-with-proguard-mappi…

13f5a6b

…ng-files

Add GET endpoint and tests

8b30284

Revert on_results in DebugFilesEndpoint

999226d

buenaflor marked this pull request as ready for review May 10, 2023 09:48

buenaflor requested review from a team as code owners May 10, 2023 09:48

buenaflor requested review from iambriccardo and removed request for a team May 10, 2023 09:48

markstory reviewed May 10, 2023

View reviewed changes

wedamija reviewed May 10, 2023

View reviewed changes

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jul 11, 2023

buenaflor force-pushed the feat/associate-release-with-proguard-mapping-files branch from 4c342fa to 6c08935 Compare July 11, 2023 13:41

Swatinem reviewed Jul 12, 2023

View reviewed changes

iambriccardo reviewed Jul 12, 2023

View reviewed changes

buenaflor added Status: In Progress and removed Scope: Frontend Automatically applied to PRs that change frontend components Status: Stale labels Jul 12, 2023

buenaflor added 2 commits July 12, 2023 15:58

Add link to ProjectDebugFile and change index to proguard_uuid

293ff71

Formatting

9736a97

buenaflor requested review from iambriccardo and Swatinem July 12, 2023 14:03

vercel bot deployed to Preview July 12, 2023 14:05 View deployment

asottile-sentry reviewed Jul 12, 2023

View reviewed changes

iambriccardo approved these changes Jul 13, 2023

View reviewed changes

buenaflor and others added 3 commits July 13, 2023 13:09

Remove # type: ignore

541f5a5

Merge branch 'master' into feat/associate-release-with-proguard-mappi…

fb28dd5

…ng-files

🛠️ apply pre-commit fixes

7911d52

vercel bot deployed to Preview July 13, 2023 11:20 View deployment

Fix typing errors

eed6037

vercel bot deployed to Preview July 13, 2023 13:24 View deployment

Merge branch 'master' into feat/associate-release-with-proguard-mappi…

f0ec9ad

…ng-files

vercel bot deployed to Preview July 13, 2023 13:31 View deployment

Merge branch 'master' into feat/associate-release-with-proguard-mappi…

deb8683

…ng-files

vercel bot deployed to Preview July 13, 2023 15:19 View deployment

Swatinem approved these changes Jul 14, 2023

View reviewed changes

Merge branch 'master' into feat/associate-release-with-proguard-mappi…

b9871c2

…ng-files

vercel bot deployed to Preview July 14, 2023 08:49 View deployment

buenaflor merged commit 2324f4e into master Jul 14, 2023

buenaflor deleted the feat/associate-release-with-proguard-mapping-files branch July 14, 2023 09:21

buenaflor removed the Status: In Progress label Jul 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(proguard): associate release with proguard mapping files #48511

feat(proguard): associate release with proguard mapping files #48511

buenaflor commented May 4, 2023

github-actions bot commented May 4, 2023

codecov bot commented May 4, 2023 •

edited

Loading

github-actions bot commented May 9, 2023

markstory May 10, 2023

markstory May 10, 2023

markstory May 10, 2023

wedamija May 10, 2023

iambriccardo May 11, 2023

Swatinem Jul 12, 2023

Swatinem Jul 12, 2023

iambriccardo Jul 12, 2023

Swatinem Jul 12, 2023

buenaflor Jul 12, 2023

Swatinem Jul 14, 2023

iambriccardo left a comment

iambriccardo Jul 12, 2023

iambriccardo Jul 12, 2023

buenaflor Jul 12, 2023

asottile-sentry Jul 12, 2023

buenaflor Jul 13, 2023

iambriccardo left a comment

		organization_id = BoundedBigIntegerField(db_index=True)
		project_id = BoundedBigIntegerField(db_index=True)

feat(proguard): associate release with proguard mapping files #48511

feat(proguard): associate release with proguard mapping files #48511

Conversation

buenaflor commented May 4, 2023

github-actions bot commented May 4, 2023

codecov bot commented May 4, 2023 • edited Loading

Codecov Report

github-actions bot commented May 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iambriccardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iambriccardo left a comment

Choose a reason for hiding this comment

codecov bot commented May 4, 2023 •

edited

Loading