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

Bugfix: add-sample-rate-native #2200

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

Conversation

JulianBissekkou
Copy link

@JulianBissekkou JulianBissekkou commented Jul 26, 2024

📜 Description

During development I discovered that the sample rate is not correctly forwarded to the native sdks.

💡 Motivation and Context

This PR makes sure that initialization of the native sdks also uses the configured sampleRate and traceSampleRate from the FlutterSentryOptions.

💚 How did you test it?

I tested this by setting a breakpoint and ensuring that my configuration was applied to the native options. If there is a better way, let me know :)

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

@JulianBissekkou
Copy link
Author

I checked "No breaking changes" since I think this is not breaking. However, the sdk might behave differently but I would say its no the correct expected behavior.

@buenaflor
Copy link
Contributor

hey, do you use the native sdk alongside the flutter sdk?

@buenaflor
Copy link
Contributor

I'm trying to find the context as to why this was not added before. It's hard to imagine that this was just an oversight but it could be.

@buenaflor
Copy link
Contributor

@JulianBissekkou
Copy link
Author

@buenaflor
I am only using the flutter sdk and my application is running on android and iOS and I was curious how the implementation of the initialization is done and then I also found that some of the properties where not parsed.

I was also suprised, but my assumption was that nobody found this since most react native and flutter developers are interested in the errors that are thrown in the TS or Dart code and the native exceptions are very rare.
The missing change in the sampleRate is therefore nothing that most people would directly notice since it doesn't affect many events.

I wasn't sure if there is some kind of magic initialization that I was missing so take your time with reviewing this.
Looking forward hearing from you :)

@buenaflor
Copy link
Contributor

After some discussion I don't think this should be added since there's the possibility of double instrumentation since the native sdks themselves have instrumentation for certain integrations. For example the cocoa sdk has all these automatic instrumentation which might result in duplicate data being sent.

In terms of feature set and design we also want to keep our hybrid sdks as consistent as possible to one another.

So adding this change brings a lot of implications and is actually a rather big decision.

@JulianBissekkou
Copy link
Author

@buenaflor
Can you explain why we get duplicate instrumentation? We are only updating configuration parameters of the iOS sdks. This can also be done by developers that are using the native iOS integration. The flutter/dart sdk will only provide data to sentry for the dart specific part.

@buenaflor
Copy link
Contributor

buenaflor commented Jul 30, 2024

@JulianBissekkou if we pass tracesSampleRate as well then we would get duplicate appstart performance transactions, one implemented in native ios/android and one implemented in flutter. Just one of many examples.

here is what we should do: I agree that sampleRate should be passed, this controls errors and crashes so it makes sense to pass it. but tracesSampleRate should stay as it is because of the mentioned reason.

@JulianBissekkou
Copy link
Author

@buenaflor Thanks for explaining.
I applied your suggestions 👍🏽

Copy link

codecov bot commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.72%. Comparing base (90a08ea) to head (7ea555d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2200      +/-   ##
==========================================
+ Coverage   88.03%   90.72%   +2.68%     
==========================================
  Files         247       73     -174     
  Lines        8585     2371    -6214     
==========================================
- Hits         7558     2151    -5407     
+ Misses       1027      220     -807     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@buenaflor
Copy link
Contributor

buenaflor commented Aug 1, 2024

@JulianBissekkou some tests are failing but it should be a very simple fix (you can ignore the danger ci)

also could you add this changelog entry under # Fixes in the unreleased section

- Pass `sampleRate` to native SDKs ([#2200](https://github.com/getsentry/sentry-dart/pull/2200))

Copy link
Collaborator

@denrase denrase left a comment

Choose a reason for hiding this comment

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

Thank you for contributing!

  • Please also add tests for the new values our native tests SentryFlutterTests.swift and SentryFlutterTest.kt.
  • Update the test expectations in init_native_sdk_test.dart to include the new value

@JulianBissekkou
Copy link
Author

I will make this PR ready later this week :) Thanks for your feedback

Copy link
Collaborator

@denrase denrase left a comment

Choose a reason for hiding this comment

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

LGTM, only issue left is a linting issue in the Kotlin file:

SentryFlutterTest.kt:162:28: error: Missing trailing comma before ")"

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.

3 participants