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

DS issues on SDKs #49

Closed
5 tasks
marandaneto opened this issue Sep 13, 2022 · 11 comments
Closed
5 tasks

DS issues on SDKs #49

marandaneto opened this issue Sep 13, 2022 · 11 comments
Labels

Comments

@marandaneto
Copy link

marandaneto commented Sep 13, 2022

user.segment isn't unified.
iOS reads from user.data and Java from user.other.
#37 but the right behavior is adding a top-level property called segment.
https://github.com/getsentry/sentry-javascript/blob/beb3c973c0f10c538c8369983fc08c5bd031bfde/packages/types/src/user.ts#L8
Java issue

tracePropagationTargets isn't unified, but we'd need to offer at least one option.
Java reads from tracingOrigins and iOS does not have it at all.
#39

baggage behavior isn't unified.
Java always overwrites the baggage header which is wrong per spec, iOS does not.
https://develop.sentry.dev/sdk/performance/dynamic-sampling-context/#baggage
Java has an open draft that addresses that.

traceSampling SDK option.
Java is guarded by this option, iOS does not have it at all, not sure if such a flag should exist.
Apparently, it's only gated by tracePropagationTargets, we don't need such traceSampling flag.
Java issue

SDK's don't implement the dynamic_sampling_context_frozen logic, do we need it?
https://develop.sentry.dev/sdk/performance/dynamic-sampling-context/#unified-propagation-mechanism
https://develop.sentry.dev/sdk/performance/dynamic-sampling-context/#freezing-dynamic-sampling-context
Java has an open draft that addresses that.
This is experimental and we should not do anything for now, even less relevant for Mobile.

How do we track the adoption of DS on SDKs? do we need to?

Remove isTraceSampling option so we can propagate baggage by default

  • iOS
  • Android
  • Flutter
  • .NET
  • React Native

Related issues

@adinauer
Copy link
Member

adinauer commented Sep 13, 2022

SDK's don't implement the dynamic_sampling_context_frozen logic, do we need it?

That's also about to change for the Java SDK with this PR getsentry/sentry-java#2226

@adinauer
Copy link
Member

Do we just make setTraceSampling(boolean) a NOOP and have isTraceSampling() always return true?

@bruno-garcia
Copy link
Member

Do we just make setTraceSampling(boolean) a NOOP and have isTraceSampling() always return true?

How about we just flip the default to true. Possibly add an Obsolete annotation to mark for removal. This past would be best done when we have the new option tracePropagationTragets to point users to.

@marandaneto
Copy link
Author

@adinauer @bruno-garcia that was already described here getsentry/sentry-java#2244
Let's move the conversation in there to not polute this meta issue, I agree with the flipping + deprecating, adding tracePropagationTragets is also very trivial, pretty much a renaming of tracingOrigins, that would also deprecate and point to the new one.

@lbloder
Copy link

lbloder commented Sep 16, 2022

How about we just flip the default to true. Possibly add an Obsolete annotation to mark for removal. This past would be best done when we have the new option tracePropagationTragets to point users to.

@bruno-garcia that would also mean, that once the flag is removed, each "captureTransaction" will send the traceContext (dynamic sampling context) to sentry and there is no way for the user to disable this behavior.
Can you confirm that this is the behavior we want?

@marandaneto
Copy link
Author

tracePropagationTragets

That's the goal of tracePropagationTragets, you can filter out specific URLs or everything.

@marandaneto
Copy link
Author

@philipphofmann @brustolin is this being used?
is this the equivalent for isTraceSampling on Java that should be replaced?

@brustolin
Copy link

No, we dont have this flag anymore.
We need to remove this from test.

@marandaneto
Copy link
Author

No, we dont have this flag anymore. We need to remove this from test.

Please do it to avoid further confusion.

@philipphofmann
Copy link
Member

@brustolin, please create an issue for that on the Cocoa repo.

@philipphofmann
Copy link
Member

@brustolin or @marandaneto, do we already have an issue for that on the Cocoa repo? If yes, is it done?

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

No branches or pull requests

6 participants