Skip to content

Conversation

@supervacuus
Copy link
Collaborator

This is a very early approach to fix #1026

This is mostly meant for integration in the alpha/beta build sequence of sentry-native, sentry-android, and sentry-dotnet to test against the dotnet-maui on the Android repro project.

No review is required at this stage, just feedback from the downstream test integration.

cc: @kahest, @bitsandfoxes, @markushi

#skip-changelog

@github-actions
Copy link

github-actions bot commented Aug 5, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against ca5e83a

@codecov
Copy link

codecov bot commented Aug 5, 2024

Codecov Report

Attention: Patch coverage is 41.93548% with 18 lines in your changes missing coverage. Please review.

Project coverage is 82.40%. Comparing base (67cc95c) to head (ca5e83a).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1027      +/-   ##
==========================================
- Coverage   82.58%   82.40%   -0.19%     
==========================================
  Files          53       53              
  Lines        7729     7752      +23     
  Branches     1214     1216       +2     
==========================================
+ Hits         6383     6388       +5     
- Misses       1234     1252      +18     
  Partials      112      112              

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the detailed code comments, helped a lot. 🚀

@supervacuus
Copy link
Collaborator Author

LGTM! Thanks for the detailed code comments, helped a lot. 🚀

Great, thanks!

Just to clarify, I intended to provide this branch primarily for downstream testing. I will only merge/release once we know introducing this strategy solves the issue downstream. Also, there is still a TODO item (enable page allocator only when we have a native crash) in the PR that should be done before a merge, not counting any changes required after downstream feedback or adding integration tests that cover the new strategy.

@markushi
Copy link
Member

markushi commented Sep 2, 2024

Quick update: We published an alpha release for sentry-java (7.15.0-alpha1) for testing purposes

@bruno-garcia
Copy link
Member

Quick update: We published an alpha release for sentry-java (7.15.0-alpha1) for testing purposes

@markushi any details on usage/validation of this change?

@markushi
Copy link
Member

markushi commented Oct 7, 2024

Quick update: We published an alpha release for sentry-java (7.15.0-alpha1) for testing purposes

@markushi any details on usage/validation of this change?

@bruno-garcia @bitsandfoxes We don't have any data points right now. But the code path for Java/Android mostly stays the same. I mentioned it somewhere else already but we'd need a downstream .NET SDK release as well to enable and test this fully, as the new handler strategy needs to be configured:

SentryAndroidOptions.setNativeHandlerStrategy(NdkHandlerStrategy.SENTRY_HANDLER_STRATEGY_CHAIN_AT_START);

@bruno-garcia
Copy link
Member

Quick update: We published an alpha release for sentry-java (7.15.0-alpha1) for testing purposes

@markushi any details on usage/validation of this change?

@bruno-garcia @bitsandfoxes We don't have any data points right now. But the code path for Java/Android mostly stays the same. I mentioned it somewhere else already but we'd need a downstream .NET SDK release as well to enable and test this fully, as the new handler strategy needs to be configured:

SentryAndroidOptions.setNativeHandlerStrategy(NdkHandlerStrategy.SENTRY_HANDLER_STRATEGY_CHAIN_AT_START);

@bricefriha is this something you can help us with?

cc @jamescrosswell

@bricefriha
Copy link

@bricefriha is this something you can help us with?

sure!

* overwrite it only when the flag `SS_DISABLED` is set and the query didn't result in an error
* if the query was successful but the flag is anything but `SS_DISABLED`, only log the size and flags of the current stack
* if the query failed then log the corresponding error
@supervacuus supervacuus force-pushed the feat/inproc_handler_strategy branch from c9e2d33 to ca5e83a Compare November 15, 2024 15:46
@supervacuus supervacuus merged commit 7c1d428 into master Nov 15, 2024
24 checks passed
@supervacuus supervacuus deleted the feat/inproc_handler_strategy branch November 15, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Optionally allow the inproc handler to invoke the signal chain at the start

5 participants