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

chore(Mobile): Fixes tracing options casing, updates options #10305

Merged
merged 8 commits into from
Jun 10, 2024

Conversation

kahest
Copy link
Member

@kahest kahest commented Jun 6, 2024

DESCRIBE YOUR PR

  • Fixes options casing for all Mobile platforms
    fixes Android SDK: wrong casing for option keys #10237
  • Removes outdated tracing-origins options (Android, KMP)
  • Adds missing trace-propagation-targets option (RN)
  • Adds whole section for Dart/Flutter

IS YOUR CHANGE URGENT?

Help us prioritize incoming PRs by letting us know when the change needs to go live.

  • ideally June 11, latest June 20 :)

SLA

  • Teamwork makes the dream work, so please add a reviewer to your PRs.
  • Please give the docs team up to 1 week to review your PR unless you've added an urgent due date to it.
    Thanks in advance for your help!

PRE-MERGE CHECKLIST

Make sure you've checked the following before merging your changes:

  • Checked Vercel preview for correctness, including links
  • PR was reviewed and approved by any necessary SMEs (subject matter experts)
  • PR was reviewed and approved by a member of the Sentry docs team

EXTRA RESOURCES

@kahest kahest requested a review from a team June 6, 2024 12:57
Copy link

vercel bot commented Jun 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sentry-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 10, 2024 8:07am

Copy link

codecov bot commented Jun 6, 2024

Bundle Report

Changes will decrease total bundle size by 14 bytes ⬇️

Bundle name Size Change
sentry-docs-server 7.45MB 3 bytes ⬇️
sentry-docs-edge-server 462.26kB 3 bytes ⬇️
sentry-docs-client 6.19MB 8 bytes ⬇️

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

From the Apple standpoint, LGTM

@kahest kahest requested review from markushi and buenaflor June 7, 2024 08:25
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!

Copy link
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

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

looks good on kmp, dart/flutter 👍

@kahest kahest requested a review from a team June 7, 2024 09:55
@kahest kahest changed the title chore(Mobile): Fixes traces options casing, updates options chore(Mobile): Fixes tracing options casing, updates options Jun 7, 2024
Copy link
Contributor

@vivianyentran vivianyentran left a comment

Choose a reason for hiding this comment

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

Left a suggestion. Thanks for updating!

docs/platforms/android/configuration/options.mdx Outdated Show resolved Hide resolved
docs/platforms/dart/configuration/options.mdx Show resolved Hide resolved

<ConfigKey name="traces-sample-rate">

A number between 0 and 1, controlling the percentage chance a given transaction will be sent to Sentry. (0 represents 0% while 1 represents 100%.) Applies equally to all transactions created in the app. Either this or <PlatformIdentifier name="traces-sampler" /> must be defined to enable tracing.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Either this or must be defined to enable tracing."

Doesn't it default to 1 if these aren't defined?

Copy link
Member Author

Choose a reason for hiding this comment

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

only if enableTracing is set to true - then tracesSampleRate will be set to the recommended value of 1.0.

I know the wording for enableTracing that we copy-pasted and now have on all SDK docs is a bit confusing:

A boolean value, if true, transactions and trace data will be generated and captured. This will set the traces-sample-rate to the recommended default of 1.0 if traces-sample-rate is not defined

The default value is 0.0. The recommended value if you enable tracing is 1.0. We should probably re-word to something like

A boolean value that determines whether tracing is enabled and trace data will be generated and captured. This will set the traces-sample-rate to the recommended value of 1.0 if traces-sample-rate is not defined

OTOH there is an ongoing DACI to decide if we remove enableTracing altogether

@kahest
Copy link
Member Author

kahest commented Jun 10, 2024

Left a suggestion. Thanks for updating!

Thank you! I applied one suggestion. If we decide we want to update the wording on enableTracing, we should do that in a separate PR for all platforms at once (but I suggest we wait until the DACI is decided, see #10305 (comment)).

@kahest kahest merged commit 915b618 into master Jun 10, 2024
6 checks passed
@kahest kahest deleted the kahest/tracing-options-mobile branch June 10, 2024 08:24
@github-actions github-actions bot locked and limited conversation to collaborators Jun 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android SDK: wrong casing for option keys
7 participants