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

feat(sdk-crashes): Add ReactNative options #59261

Merged
merged 5 commits into from
Nov 2, 2023

Conversation

philipphofmann
Copy link
Member

Add ReactNative options as a first step to enable SDK crash detection for ReactNative. As the sample rate is 0.0 and no config for ReactNative exists, this PR doesn't change any behavior.

This PR is based on #59257.

Extract building SDK crash detection configs to a new method, so adding
new SDKs doesn't pollute post_process.py with more code, and to increase
testability.
Add ReactNative options as a first step to enable SDK crash detection
for ReactNative. As the sample rate is 0.0 and no config for ReactNative
exists, this PR doesn't change any behavior.
@philipphofmann philipphofmann self-assigned this Nov 2, 2023
@philipphofmann philipphofmann requested a review from a team November 2, 2023 10:55
@philipphofmann philipphofmann added the Scope: Backend Automatically applied to PRs that change backend components label Nov 2, 2023
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #59261 (e868538) into master (7f6ccde) will increase coverage by 24.96%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head e868538 differs from pull request most recent head a12cd94. Consider uploading reports for the commit a12cd94 to get more accurate results

@@             Coverage Diff             @@
##           master   #59261       +/-   ##
===========================================
+ Coverage   55.70%   80.67%   +24.96%     
===========================================
  Files        5147     5161       +14     
  Lines      225050   226374     +1324     
  Branches    37893    38140      +247     
===========================================
+ Hits       125361   182624    +57263     
+ Misses      96004    38172    -57832     
- Partials     3685     5578     +1893     
Files Coverage Δ
src/sentry/options/defaults.py 100.00% <100.00%> (ø)
src/sentry/tasks/post_process.py 89.94% <100.00%> (+75.56%) ⬆️
...s/sdk_crashes/build_sdk_crash_detection_configs.py 100.00% <100.00%> (+64.28%) ⬆️
...ry/utils/sdk_crashes/sdk_crash_detection_config.py 100.00% <100.00%> (ø)

... and 1839 files with indirect coverage changes

options_prefix = f"issues.sdk_crash_detection.{sdk_name.value}"

project_id = options.get(f"{options_prefix}.project_id")
if not project_id or project_id == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

0 is a Falsy value, so the second check is redundant:

Suggested change
if not project_id or project_id == 0:
if not project_id:

(same with sample_rate below)

)

return [cocoa_config]
def build_config(sdk_name: SdkName) -> Optional[SDKCrashDetectionConfig]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I generally find the inner functions harder to read, but no strong opinions.

Base automatically changed from ref/sdk-crashes-extract-building-configs to master November 2, 2023 13:45
@iker-barriocanal iker-barriocanal requested a review from a team as a code owner November 2, 2023 13:45
@philipphofmann philipphofmann requested review from a team as code owners November 2, 2023 14:19
@philipphofmann philipphofmann requested review from a team November 2, 2023 14:19
@philipphofmann philipphofmann requested a review from a team as a code owner November 2, 2023 14:19
@philipphofmann philipphofmann requested review from a team November 2, 2023 14:19
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Nov 2, 2023
Copy link
Contributor

github-actions bot commented Nov 2, 2023

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

@philipphofmann philipphofmann enabled auto-merge (squash) November 2, 2023 15:33
@philipphofmann philipphofmann merged commit 431c595 into master Nov 2, 2023
@philipphofmann philipphofmann deleted the feat/sdk-crashes-react-native-options branch November 2, 2023 15:37
@github-actions github-actions bot locked and limited conversation to collaborators Nov 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants