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

build: enable source context upload to Sentry #1966

Merged
merged 5 commits into from
Jul 3, 2024

Conversation

wzieba
Copy link
Member

@wzieba wzieba commented Mar 20, 2024

Description

Sentry pushed to their SaaS our contribution (getsentry/rust-proguard#36) which enables projects with R8 full mode to be correctly deobfuscated on Sentry dashboard.

So now it makes sense to send source context to their servers and benefit from more details when investigating issues on Sentry.

Testing Instructions

Not needed: see screenshots below.

Screenshots or Screencast

Source context is uploaded:

image

Sentry shows the source code around the stacktrace records

See: https://a8c.sentry.io/issues/5547984676/?project=6711064&query=is%3Aunresolved+issue.priority%3A%5Bhigh%2C+medium%5D&referrer=issue-stream&statsPeriod=1h&stream_index=0&utc=true

image

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@wzieba wzieba marked this pull request as ready for review June 28, 2024 10:56
@wzieba wzieba requested a review from a team as a code owner June 28, 2024 10:56
@wzieba wzieba requested review from MiSikora and removed request for a team June 28, 2024 10:56
@wzieba wzieba added this to the 7.68 milestone Jun 28, 2024
@wzieba wzieba added the [Type] Tooling Related to the Gradle build scripts and the setup or maintenance of the project build process. label Jun 28, 2024
@wzieba wzieba requested review from ParaskP7 and a team June 28, 2024 10:56
Copy link
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @wzieba !

I have reviewed this PR and everything LGTM, good job! 🌟

Sentry pushed to their SaaS our contribution (getsentry/rust-proguard#36) which enables projects with R8 full mode to be correctly deobfuscated on Sentry dashboard.

Awesome work there and congrats on this contribution of youors pushed into Sentry's Saas! 🎉

So now it makes sense to send source context to their servers and benefit from more details when investigating issues on Sentry.

👍 but what I don't understand is how to (kind of) test it. I know you said it is not needed, but I got curious and was trying to find a release issue on Sentry that would give me SourceFile instead of the correct name of the file.

Can you help point me to such issue? 🙏

For example looking at this issue I could see the correct name of the file already. I am more curious than anything else...

image

@wzieba
Copy link
Member Author

wzieba commented Jul 3, 2024

Thanks @ParaskP7 for the reviews! Let me give you more context

I got curious and was trying to find a release issue on Sentry that would give me SourceFile instead of the correct name of the file.

Because Sentry pushed the change to SaaS, you won't see SourceFile anymore - new and old events are now deobfuscated correctly on Sentry.

@wzieba wzieba merged commit c436555 into main Jul 3, 2024
15 checks passed
@wzieba wzieba deleted the enable_source_context_upload branch July 3, 2024 05:42
@ParaskP7
Copy link
Contributor

ParaskP7 commented Jul 3, 2024

Because Sentry pushed the change to SaaS, you won't see SourceFile anymore - new and old events are now deobfuscated correctly on Sentry.

Thanks for the additional context here @wzieba , this is exactly what I was thinking as well, but then, my more targeted question is, why do we need to set the includeSourceContext then, that is, if it is anyway working already as expected, that's what I don't understand? 🤔

@wzieba
Copy link
Member Author

wzieba commented Jul 3, 2024

if it is anyway working already as expected, that's what I don't understand? 🤔

"it" here means "deobfuscation". Deobfuscation is now working correctly on Sentry and this is the result of Sentry enabling our contribution. This PR is not fixing deobfuscation.

why do we need to set the includeSourceContext then

Because deobfuscation is now fixed, we're unblocked with sending source code to Sentry by setting includeSourceContext option.

I think examples can help here:

Stack trace with not correctly working deobfuscation and without source context

image

Stack trace with correctly working deobfuscation and without source context (before this PR)

image

Stack trace with correctly working deobfuscation and with source context (after this PR)

image

@ParaskP7
Copy link
Contributor

ParaskP7 commented Jul 4, 2024

Oh cool @wzieba , this helps a lot, thanks for providing examples to explain that to me, much appreciated! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Tooling Related to the Gradle build scripts and the setup or maintenance of the project build process.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants