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.

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

fix(debugfile): Recognize chunk-uploaded Proguard files #81131

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

szokeasaurusrex
Copy link
Member

@szokeasaurusrex szokeasaurusrex commented Nov 21, 2024

The detect_dif_from_path function does not correctly identify Proguard files which are chunk uploaded because these files have a randomly-assigned temporary path, and the logic tries to guess whether the file is a Proguard file based on its path. However, the detect_dif_from_path function also takes an optional name option, which for chunk-uploaded files, is the file name that is specified by the client making the assemble call. This has not been a problem yet, since Sentry CLI does not support chunk uploading Proguard files, but we are adding support in getsentry/sentry-cli#2196, and so we need to fix the backend to allow chunk uploads of Proguard files.

Here, we update the logic to check both the path and the name for potentially matching a Proguard file.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 21, 2024
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/fix-proguard-chunk-upload branch from 7f18af5 to 2556e35 Compare November 21, 2024 17:36
@szokeasaurusrex
Copy link
Member Author

@asottile-sentry I updated the tests per your suggestion. Please let me know whether this looks good now

Copy link

codecov bot commented Nov 21, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
323 1 322 9
View the top 1 failed tests by shortest run time
::tests.sentry.models.test_debugfile
Stack Traces | 0s run time
No failure message available

To view more test analytics, go to the Test Analytics Dashboard
Got feedback? Let us know on Github

The `detect_dif_from_path` function does not correctly identify Proguard files which are chunk uploaded because these files have a randomly-assigned temporary path, and the logic tries to guess whether the file is a Proguard file based on its path. However, the `detect_dif_from_path` function also takes an optional `name` option, which for chunk-uploaded files, is the file name that is specified by the client making the assemble call. This has not been a problem yet, since Sentry CLI does not support chunk uploading Proguard files, but we are adding support in getsentry/sentry-cli#2196, and so we need to fix the backend to allow chunk uploads of Proguard files.

Here, we update the logic to check both the `path` and the `name` for potentially matching a Proguard file.
Copy link
Member

@asottile-sentry asottile-sentry left a comment

Choose a reason for hiding this comment

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

could make them real paths with tmp_path if you wanted but this seems good enough to me

I have no stake in this particular model / implementation so you may want to get sign off from the actual owners

Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@szokeasaurusrex
Copy link
Member Author

I have no stake in this particular model / implementation so you may want to get sign off from the actual owners

Agreed, do you know who the owners might be @asottile-sentry? There is no code owner assigned in GitHub, and it doesn't look like this file gets edited very often

@asottile-sentry
Copy link
Member

I have no stake in this particular model / implementation so you may want to get sign off from the actual owners

Agreed, do you know who the owners might be @asottile-sentry? There is no code owner assigned in GitHub, and it doesn't look like this file gets edited very often

have no idea -- which sdk generates these types of files? that would be my guess having no context (beyond probably reformatting this or fixing the types)

@szokeasaurusrex
Copy link
Member Author

I added @loewenheim and @Swatinem since these files get sent by Sentry CLI, and they were the previous maintainers before I took over. Looks like they have touched the file before

szokeasaurusrex added a commit to getsentry/sentry-cli that referenced this pull request Nov 21, 2024
Introduce an experimental chunk uploading feature for the `sentry-cli upload-proguard` command. The feature can be activated by setting the `SENTRY_EXPERIMENTAL_PROGUARD_CHUNK_UPLOAD` environment variable to `1`. The feature is only activated when users opt in via this environment variable.

The experimental chunk uploading feature is not backwards compatible. We attempt the upload regardless of whether the server supports receiving chunk-uploaded Proguard files. Server-side support will only be available once getsentry/sentry#81131 is released. The goal here was to create something that works, so some optimizations that we use for other chunk uploaded file types (e.g. first checking whether any chunks are present on the server, and only uploading the missing ones), are not implemented for Proguard.

Ref #2196
szokeasaurusrex added a commit to getsentry/sentry-cli that referenced this pull request Nov 21, 2024
Introduce an experimental chunk uploading feature for the `sentry-cli upload-proguard` command. The feature can be activated by setting the `SENTRY_EXPERIMENTAL_PROGUARD_CHUNK_UPLOAD` environment variable to `1`. The feature is only activated when users opt in via this environment variable.

The experimental chunk uploading feature is not backwards compatible. We attempt the upload regardless of whether the server supports receiving chunk-uploaded Proguard files. Server-side support will only be available once getsentry/sentry#81131 is released. The goal here was to create something that works, so some optimizations that we use for other chunk uploaded file types (e.g. first checking whether any chunks are present on the server, and only uploading the missing ones), are not implemented for Proguard.

Ref #2196
szokeasaurusrex added a commit to getsentry/sentry-cli that referenced this pull request Nov 21, 2024
Introduce an experimental chunk uploading feature for the `sentry-cli upload-proguard` command. The feature can be activated by setting the `SENTRY_EXPERIMENTAL_PROGUARD_CHUNK_UPLOAD` environment variable to `1`. The feature is only activated when users opt in via this environment variable.

The experimental chunk uploading feature is not backwards compatible. We attempt the upload regardless of whether the server supports receiving chunk-uploaded Proguard files. Server-side support will only be available once getsentry/sentry#81131 is released. The goal here was to create something that works, so some optimizations that we use for other chunk uploaded file types (e.g. first checking whether any chunks are present on the server, and only uploading the missing ones), are not implemented for Proguard.

Ref #2196
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants