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

Add Split Apks info as extras #3193

Merged
merged 9 commits into from
Feb 6, 2025
Merged

Conversation

ganadist
Copy link
Contributor

@ganadist ganadist commented Feb 6, 2024

📜 Description

Fix #3192

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Need to check test and unittest cases.

Sorry, something went wrong.

@romtsn
Copy link
Member

romtsn commented Feb 6, 2024

hi @ganadist thanks for opening an issue for this and the PR! Before proceeding, I'd like to better understand your use-case - would you like to search for events with a specific split name?

In general, we don't set tags by default in the SDKs, but rather extract them on backend, if there's a good case for those and cardinality is not high.

@ganadist
Copy link
Contributor Author

ganadist commented Feb 6, 2024

Hello, @romtsn
I added more description at #3192
And I checked the implementation of setSideLoadedInfo and implemented setSplitApksInfo similarly.
Is there any additional work required on the backend side?

@romtsn
Copy link
Member

romtsn commented Feb 6, 2024

@ganadist thanks, but can you please answer the question: would you like to search for events with a specific split name? If you don't need to search for it but just see it in the issue details, we could just send this info as part of App context then.

Sending this as tag would require us to store and index it, and given that this field is high-cardinality (afaik apk splits are unique in their names and if multiply it by number of apks our customers might have that's huge), so this is unlikely that we'll take that route.

setSideLoadedInfo is doing it wrong and we have an issue to fix that, so don't see it as a reference for implementation.

@ganadist
Copy link
Contributor Author

ganadist commented Feb 6, 2024

: would you like to search for events with a specific split name?

If it is possible to search for these events, it will be helpful to know the crash rate that occurs due to these problems.
Because I have experienced several instances of increased crashes due to random distribution by the vendor store with missing split apks.

However, if this change may cause strain to the backend, it would be okay with being part of the app context instead of tags.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@ganadist ganadist changed the title Add Split Apks info as tags Add Split Apks info as extras Feb 7, 2024
@ganadist
Copy link
Contributor Author

ganadist commented Feb 7, 2024

Added 42a229f to use extras instead of tags.

@romtsn
Copy link
Member

romtsn commented Feb 13, 2024

it will be helpful to know the crash rate that occurs due to these problems.

Unfortunately crash rate is based on Sessions dataset and it's not possible to filter session by custom tags/values yet. If search is not crucial for you, we should just add it as part of the App context - you will be able to see this information on individual events/issues then

@romtsn romtsn requested a review from lcian as a code owner February 6, 2025 22:36
@romtsn
Copy link
Member

romtsn commented Feb 6, 2025

hi @ganadist sorry for abandoning this for so long, finally got the time to move the info to the app context. We'll ship this improvement as part of version 8.2.0. Thanks for your contribution!

Google Chrome 2025-02-06 23 34 52

@romtsn romtsn enabled auto-merge (squash) February 6, 2025 22:42
@romtsn romtsn merged commit b79b57c into getsentry:main Feb 6, 2025
30 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Split Apks information as tag
2 participants