feat(appconnect): Get dSYMs URL from the API [NATIVE-294] #29513

flub · 2021-10-22T15:58:03Z

Instead of using the legacy iTunes endpoints to download the dSYMs
start using the API for this.

Very much broken currently, lots of edge-cases unanswered, but it's Fri evening before a holiday.

related: #29531

Instead of using the legacy iTunes endpoints to download the dSYMs start using the API for this.

relaxolotl · 2021-10-26T23:30:20Z

rebased off of master in case this needed to be updated to match sentry's bump up to 3.8.

relaxolotl · 2021-10-27T05:48:52Z

src/sentry/tasks/app_store_connect.py

+                # Moving on to the next build so we don't check off fetched. This url will
+                # eventuallyTM be populated, so revisit it at a later time.


in the long run there should be a way to specify a maximum number of retries in this situation. after we hit that max number this should just be demoted to a build with no dsyms.

relaxolotl · 2021-10-27T05:49:27Z

src/sentry/tasks/app_store_connect.py

+            # pending downloads.
+            except appstoreconnect_api.RequestError as e:
+                sdk.capture_exception(e)
+                continue
            except requests.RequestException as e:


unsure if we actually need to keep this particular case, probably could just remove it

src/sentry/utils/appleconnect/appstore_connect.py

relaxolotl · 2021-10-27T05:54:53Z

src/sentry/utils/appleconnect/appstore_connect.py

+        status = res.status_code
+        if status == HTTPStatus.UNAUTHORIZED:
+            raise UnauthorizedError
+        elif status == HTTPStatus.FORBIDDEN:
+            raise ForbiddenError
+        elif status != HTTPStatus.OK:
+            raise RequestError(f"Bad status code downloading dSYM: {status}")


the implementation using iTunes never checked the status code on the actual dSYM download, and instead checked these when grabbing the URLs to download with.

we don't really separately grab the URLs anymore so porting that check over to App Store Connect isn't really doable. this implementation is slightly more cautious in that it checks the status code of the download itself, but i'm unsure if this is being overly cautious.

relaxolotl · 2021-10-27T06:00:22Z

filled in the implementation.

there are a few things left that need to be addressed, all of which i've left comments on. properly removing iTunes from this entire integration is still something that needs to be done. considering its full removal most likely will require front end work, i'm hoping it would be safe to assume that deprecating iTunes should be left for a follow-up PR.

i have removed one iTunes class, but it appears to have been exclusively used in the flow being replaced in this PR.

tested this with an existing project and a new project on my local instance; i'm sure there are more things that need to be tested, such as when the app store connect credentials (/api token) are outdated or revoked.

src/sentry/utils/appleconnect/appstore_connect.py

Swatinem · 2021-10-27T12:17:38Z

src/sentry/utils/appleconnect/appstore_connect.py

            # sort newer releases first
            "&sort=-uploadedDate"
            # only include valid builds
            "&filter[processingState]=VALID"
            # and builds that have not expired yet
            "&filter[expired]=false"
+            # fetch the maximum number of build bundles
+            "&limit[buildBundles]=50"


since the buildBundles are put into the included key, is this the total number of buildBundles, or per build?
Also related to the change above that this returns a list per-build, what does that mean? What should we do in case we have multiple of these per build?

Answer:

it seems to be per-build, as the relation itself is paginated

we are using the first of the available buildBundles, raising an error when there is more than one.

since the buildBundles are put into the included key, is this the total number of buildBundles, or per build?

as you've inferred below, i believe it is indeed per-build. each build contains a relation to its build bundles that's structured like so:

"buildBundles": { "meta": { "paging": { "total": 1, "limit": 1 } }, "data": [ { "type": "buildBundles", "id": "59467f37-371e-4755-afcd-0116775a6eab" } ] },

this comes from an example where i restricted the limit to 1 bundle, but given that it has paging data nested in the relationship i'm guessing you're exactly right on this being per-bundle.

Also related to the change above that this returns a list per-build, what does that mean? What should we do in case we have multiple of these per build?

mostly just reiterating what @flub did a great job of explaining in one of our calls: we're currently using the first available build bundle, and logging cases where we do find builds with an unexpected number of bundles (0 or 2+).

the logic i've included in this PR performs additional filtering on top of what was mentioned above. we skip any bundles that do not have a link to a dSYM bundle, and use the first one that does have one.

this also weaves into another comment that i'd brought up below. there's this multi-line TODO that discusses includesSymbols and how that might factor into filtering build bundles if there are more than one of them.

src/sentry/utils/appleconnect/appstore_connect.py

Swatinem · 2021-10-27T12:24:11Z

tests/sentry/utils/appleconnect/test_appstore_connect.py

+                    "attributes": {
+                        "bundleId": "io.sentry.sample.iOS-Swift",
+                        "bundleType": "APP",
+                        "dSYMUrl": "http://iosapps.itunes.apple.com/itunes-assets/Purple126/v4/ee/c7/d1/eec7d1ef-5c42-5db8-c53f-5f2e12ed586c/appDsyms.zip?accessKey=1635111323_6541937231607596467_GytzzDTwJudm5GzcKXxzu%2ByO1e9gu%2F2%2FxSiQC4a3PMxk6qW540YpTeZmwgxFJqlspgLnwHJs27HQpaTA%2Fvhbrq2VLI1L%2FhMomqCc9RPy3eCa%2FgixG7VyyGlSbU1YXEhQZJWFzPq2KtWoUkSCmJ0wK0M76Lv1rCdzGpSkCN9rFk8%3D",


I think you should obfuscate the accessKey in particular. And maybe trim the complete fixture down to only the props we need.

well, this accessKey is already public forever... but these are time-limited and trying to use them now results in an unauthorized response so i don't think this is a problem.

I'd rather keep this data/fixture easy to re-generate instead of trimming it. but will experiment a bit more with making this decently tested.

- Do not compare empty strings - Separate out multiple relation fetching from single relation fetching, same functionality but a little clearer on types. - Some string formatting

- move BuildInfo to where it probably should belong - make BuildInfo.dsym_url a strongly-typed union - report a sentry error when multiple buildBundles are found, regardless of whether the dSYMUrl is present. - leave a bunch of comments and a TODO to still figure out

src/sentry/utils/appleconnect/appstore_connect.py

flub · 2021-10-29T17:19:26Z

src/sentry/utils/appleconnect/appstore_connect.py

+    if bundles is None:
+        return NoDsymUrl.NOT_NEEDED
+
+    if len(bundles) != 1:


given len(bundles) == 0 is NOT_NEEDED should this condition not be > 1?

i'm going to merge the two checks (not none, len is 0) that result in NOT_NEEDED into one if as neither no longer need to be reported together with this fix.

flub · 2021-10-29T17:23:40Z

if these two comments are resolved than LGTM (i can't approve the PR as it's my own):
https://github.com/getsentry/sentry/pull/29513/files#r739150592
https://github.com/getsentry/sentry/pull/29513/files#r739411297

Tries to cut down on instances of SENTRY-FOR-SENTRY-F0V. We're requesting more data per build as of #29513, so we should make the timeout a little more lenient to reflect that.

feat(appconnect): Get dSYMs URL from the API

6c3ee57

Instead of using the legacy iTunes endpoints to download the dSYMs start using the API for this.

relaxolotl force-pushed the appconnect/dsym_url_api branch from 0e46f99 to 6c3ee57 Compare October 26, 2021 23:29

vercel bot deployed to Preview – sentry October 26, 2021 23:29 View deployment

vercel bot deployed to Preview – storybook October 26, 2021 23:29 View deployment

relaxolotl added 2 commits October 27, 2021 01:45

basic fetching of dsyms through app store connect

1141b46

remove iTunes client used exclusively to fetch dSYMS

49b3eab

vercel bot deployed to Preview – storybook October 27, 2021 05:47 View deployment

vercel bot deployed to Preview – sentry October 27, 2021 05:47 View deployment

relaxolotl reviewed Oct 27, 2021

View reviewed changes

src/sentry/utils/appleconnect/appstore_connect.py Outdated Show resolved Hide resolved

forgot to update docstring

ae664a1

vercel bot deployed to Preview – storybook October 27, 2021 05:52 View deployment

vercel bot deployed to Preview – sentry October 27, 2021 05:52 View deployment

relaxolotl reviewed Oct 27, 2021

View reviewed changes

Swatinem reviewed Oct 27, 2021

View reviewed changes

relaxolotl added 2 commits October 27, 2021 18:12

document new param and what it returns

1688b47

indentation

13f3e42

vercel bot deployed to Preview – storybook October 28, 2021 00:42 View deployment

vercel bot deployed to Preview – sentry October 28, 2021 00:42 View deployment

Some mintor tweaks:

919699c

- Do not compare empty strings - Separate out multiple relation fetching from single relation fetching, same functionality but a little clearer on types. - Some string formatting

vercel bot deployed to Preview – sentry October 28, 2021 15:43 View deployment

vercel bot deployed to Preview – storybook October 28, 2021 15:43 View deployment

vercel bot deployed to Preview – storybook October 28, 2021 17:40 View deployment

vercel bot deployed to Preview – sentry October 28, 2021 17:40 View deployment

relaxolotl added 2 commits October 28, 2021 15:44

remove unnecessary filtering

fd44898

move related queries together

9eaf05c

flub marked this pull request as ready for review October 29, 2021 16:20

flub requested a review from a team October 29, 2021 16:20

flub requested a review from a team as a code owner October 29, 2021 16:20

flub commented Oct 29, 2021

View reviewed changes

src/sentry/utils/appleconnect/appstore_connect.py Outdated Show resolved Hide resolved

relaxolotl added 2 commits October 29, 2021 13:07

assert order

8627973

there's no way to get pending dsym urls with our current filter

88f487a

vercel bot deployed to Preview – sentry October 29, 2021 17:08 View deployment

vercel bot deployed to Preview – storybook October 29, 2021 17:08 View deployment

double nested set context

2a7f723

vercel bot deployed to Preview – sentry October 29, 2021 17:14 View deployment

vercel bot deployed to Preview – storybook October 29, 2021 17:14 View deployment

flub commented Oct 29, 2021

View reviewed changes

relaxolotl added 2 commits October 29, 2021 14:21

don't mark malformed urls as fetched

c907742

no need to report scenarios where zero bundles are returned from the API

6e64c39

vercel bot deployed to Preview – sentry October 29, 2021 18:22 View deployment

vercel bot deployed to Preview – storybook October 29, 2021 18:22 View deployment

relaxolotl approved these changes Oct 29, 2021

View reviewed changes

remove commented out code as it'll be fixed and added later

8580da2

vercel bot deployed to Preview – sentry October 29, 2021 18:45 View deployment

vercel bot deployed to Preview – storybook October 29, 2021 18:45 View deployment

missed a spot

744e810

vercel bot deployed to Preview – sentry October 29, 2021 21:40 View deployment

vercel bot deployed to Preview – storybook October 29, 2021 21:40 View deployment

relaxolotl removed the request for review from a team October 29, 2021 21:58

relaxolotl changed the title ~~feat(appconnect): Get dSYMs URL from the API~~ feat(appconnect): Get dSYMs URL from the API [NATIVE-294] Nov 1, 2021

relaxolotl merged commit 88c7af7 into master Nov 1, 2021

relaxolotl deleted the appconnect/dsym_url_api branch November 1, 2021 17:15

github-actions bot locked and limited conversation to collaborators Nov 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(appconnect): Get dSYMs URL from the API [NATIVE-294] #29513

feat(appconnect): Get dSYMs URL from the API [NATIVE-294] #29513

flub commented Oct 22, 2021 •

edited by relaxolotl

Loading

relaxolotl commented Oct 26, 2021

relaxolotl Oct 27, 2021

relaxolotl Oct 27, 2021

relaxolotl Oct 27, 2021

relaxolotl commented Oct 27, 2021

Swatinem Oct 27, 2021

relaxolotl Oct 27, 2021

Swatinem Oct 27, 2021

flub Oct 28, 2021

flub Oct 29, 2021

relaxolotl Oct 29, 2021 •

edited

Loading

flub commented Oct 29, 2021

		# Moving on to the next build so we don't check off fetched. This url will
		# eventuallyTM be populated, so revisit it at a later time.

feat(appconnect): Get dSYMs URL from the API [NATIVE-294] #29513

feat(appconnect): Get dSYMs URL from the API [NATIVE-294] #29513

Conversation

flub commented Oct 22, 2021 • edited by relaxolotl Loading

relaxolotl commented Oct 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relaxolotl commented Oct 27, 2021

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

relaxolotl Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

flub commented Oct 29, 2021

flub commented Oct 22, 2021 •

edited by relaxolotl

Loading

relaxolotl Oct 29, 2021 •

edited

Loading