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

Sentry Integration for SDK #278

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Sentry Integration for SDK #278

wants to merge 14 commits into from

Conversation

tobitech
Copy link
Contributor

Story: https://app.shortcut.com/smileid/story/14626/

Summary

  • Install Sentry SDK for error collection
  • Setup ArkanaKeys to encrypt Sentry DSN
  • Setup new SentryErrorReporting class for capturing and sending errors to Sentry

Known Issues

N/A.

Test Instructions

N/A.

Screenshot

N/A

@tobitech tobitech self-assigned this Jan 14, 2025
@tobitech tobitech requested a review from a team as a code owner January 14, 2025 13:33
@prfectionist
Copy link

prfectionist bot commented Jan 14, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure:
The Sentry DSN is being stored in ArkanaKeys but is accessed directly in the code. Consider adding an additional layer of abstraction to prevent accidental exposure of the DSN in logs or crash reports.

⚡ Recommended focus areas for review

Configuration Concern
The Sentry configuration has debug mode and sampling rates set to maximum values which may not be suitable for production. These should be configurable based on environment.

Test Code in Production
A "Force a crash" button was added that calls fatalError() which should not be included in production code.

Disabled Feature
The Sentry setup call is commented out which means error reporting won't be initialized. This should either be enabled or the code should be removed.

@prfectionist
Copy link

prfectionist bot commented Jan 14, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Security
Protect debug-only functionality with conditional compilation to prevent accidental usage in production

Remove the test crash button from the production code as it could be accidentally
triggered in a release build.

Sources/SmileID/Classes/SelfieCapture/View/LivenessCaptureInstructionsView.swift [47-51]

+#if DEBUG
 Button {
     fatalError("Test SDK Crash")
 } label: {
     Text("Force a crash")
 }
+#endif
Suggestion importance[1-10]: 9

Why: Including crash-testing functionality in production code poses a significant risk. The suggestion correctly recommends using conditional compilation to prevent this code from being included in release builds.

9
Prevent debug testing code from being included in production builds

Wrap the crash test button in a DEBUG conditional compilation block to ensure it's
only available in development builds.

Example/SmileID/Home/HomeView.swift [21-25]

+#if DEBUG
 Button {
     fatalError("Test Demo app Crash")
 } label: {
     Text("Force a crash")
 }
+#endif
Suggestion importance[1-10]: 9

Why: Similar to the previous suggestion, this correctly identifies the need to wrap debug crash-testing code in conditional compilation blocks to prevent accidental crashes in production.

9
Best practice
Make monitoring configuration values environment-dependent to optimize performance in production

Consider making the Sentry configuration options (debug, tracesSampleRate,
profilesSampleRate) configurable based on the build environment rather than
hardcoding them.

ErrorReporting.swift [19-21]

-options.debug = true
-options.tracesSampleRate = 1.0
-options.profilesSampleRate = 1.0
+options.debug = Environment.isDevelopment
+options.tracesSampleRate = Environment.isDevelopment ? 1.0 : 0.1
+options.profilesSampleRate = Environment.isDevelopment ? 1.0 : 0.1
Suggestion importance[1-10]: 8

Why: Having high sampling rates and debug mode enabled in production could impact performance and generate unnecessary data. Environment-based configuration is a critical best practice for monitoring tools.

8
Possible issue
Remove invalid build file references that have null sources to prevent potential build issues

Remove the build file entries with "(null)" source as they are invalid and could
cause build issues. These appear to be orphaned references that should be cleaned
up.

Example/SmileID.xcodeproj/project.pbxproj [43-52]

-20F3D6F32C25F4D700B32751 /* (null) in Sources */ = {isa = PBXBuildFile; };
-620F1E9A2B691ABB00185CD2 /* (null) in Resources */ = {isa = PBXBuildFile; };
+# Remove these lines entirely
Suggestion importance[1-10]: 8

Why: The suggestion identifies invalid build file entries with "(null)" sources that could cause build failures or unexpected behavior. Removing these orphaned references is important for maintaining a clean and reliable build configuration.

8
Maintainability
Remove or implement commented-out functionality to maintain code cleanliness and completeness

The commented out setupSentry() call in the initialization code should either be
implemented or removed to avoid leaving incomplete functionality.

Sources/SmileID/Classes/SmileID.swift [112]

-// self.setupSentry()
+self.setupSentry()
Suggestion importance[1-10]: 7

Why: Commented-out code creates confusion and technical debt. The suggestion correctly identifies that the setupSentry() call should either be properly implemented or removed, especially since Sentry integration is being added in this PR.

7

Copy link

github-actions bot commented Jan 14, 2025

Warnings
⚠️ The source files were changed, but the tests remain unmodified. Consider updating or adding to the tests to match the source changes.
⚠️

Sources/SmileID/Classes/SmileID.swift#L174 - Function should have complexity 10 or less: currently complexity equals 11 (cyclomatic_complexity)

⚠️

Sources/SmileID/Classes/SmileID.swift#L201 - TODOs should be resolved (- Fix when Michael changes thi...). (todo)

Generated by 🚫 Danger Swift against 4347f6e

private init() {
// setup sentry options
let options = Sentry.Options()
options.dsn = ArkanaKeys.Global.sENTRY_DSN

Choose a reason for hiding this comment

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

@tobitech should this be SENTRY_DSN?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is autogenerated by the tool used to encrypt the Sentry DSN.

Copy link

@robin-ankele robin-ankele left a comment

Choose a reason for hiding this comment

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

How does the sdk know which sentry dsn to use on a partner's app? Not sure I fully understand this?

@@ -4,7 +4,7 @@ platform :ios, '13.0'
target 'SmileID_Example' do
pod 'SmileID', :path => '../'
pod 'netfox'
pod 'Sentry'
pod 'Sentry', '~> 8.43.0'

Choose a reason for hiding this comment

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

Do we want to fix this to a specific Sentry version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants