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 transaction source for dynamic sampling #2454

Merged
merged 7 commits into from
Sep 6, 2022

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Sep 5, 2022

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

I've added default view source for all the integration, which can be overwritten by the user.

I've picked view as its description fitted the most to what it actually is. Example Home, Menu, ProfilePage etc.
https://develop.sentry.dev/sdk/event-payloads/transaction/#transaction-annotations

💡 Motivation and Context

At the moment the source is always custom, because we first create empty transaction and then update it with the correct route data, which causes the source to be set as custom.

getsentry/team-mobile#46

💚 How did you test it?

🚨 To be done when sentry backend will allow dynamic sampling for React Native.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

🔮 Next steps

@marandaneto
Copy link
Contributor

marandaneto commented Sep 5, 2022

@krystofwoldrich view looks fine, Android used component
https://github.com/getsentry/sentry-java/blob/0d55590e368f954db1a06777867f4941be424889/sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java#L192
Can you and @adinauer figure this out? They don't need to be the same but I'd expect them to be, at the end, they are both screen's names most likely, the difference is that on Android its an Activity, and on RN is a routing that ends up being the screen's name as well.
Maybe @lforst can chime in as well since there are integrations on the FE as well, I'd expect to align to the React integration on the Web or on Android I guess.

@lforst
Copy link
Member

lforst commented Sep 5, 2022

In browser land we almost exclusively use route/url.

I originally thought that view was the most fitting here judging by the name. But if we use component in android I'd use that just to be consistent. If we want we could update the docs to be a little more specific on what the differences are.

@adinauer
Copy link
Member

adinauer commented Sep 6, 2022

Seems android should also be view in some places where component is used at the moment. We can also open an issue on the Java SDK to change it.

src/js/tracing/utils.ts Outdated Show resolved Hide resolved
@marandaneto
Copy link
Contributor

@krystofwoldrich Thank you for doing that, I liked the refactoring, it makes it easier to read.

@marandaneto
Copy link
Contributor

@krystofwoldrich after merging and releasing this, let's update the docs.

@krystofwoldrich
Copy link
Member Author

@krystofwoldrich
Copy link
Member Author

The only missing part now is test with backend.

That can happen when the React Native will be allowed for Dynamic Sampling.

@krystofwoldrich
Copy link
Member Author

Test on production, source is send correctly. Dynamic sampling on server is also working. From 10 transaction only 2 were processed when sampling set to 20%.

@krystofwoldrich krystofwoldrich merged commit 2d0ec05 into main Sep 6, 2022
@krystofwoldrich krystofwoldrich deleted the krystofwoldrich/dynamicSampling branch September 6, 2022 14:42
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.

4 participants