search

getsentry / team-mobile

Meta issues for the Mobile team
4 stars 1 forks source link

DS issues on SDKs #49

Closed marandaneto closed 2 years ago

marandaneto commented 2 years ago

user.segment isn't unified. iOS reads from user.data and Java from user.other. https://github.com/getsentry/team-mobile/issues/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. https://github.com/getsentry/team-mobile/issues/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

Related issues

adinauer commented 2 years ago

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 https://github.com/getsentry/sentry-java/pull/2226

adinauer commented 2 years ago

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

bruno-garcia commented 2 years ago

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 commented 2 years ago

@adinauer @bruno-garcia that was already described here https://github.com/getsentry/sentry-java/issues/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 commented 2 years ago

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 commented 2 years ago

tracePropagationTragets

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

marandaneto commented 2 years ago

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

brustolin commented 2 years ago

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

marandaneto commented 2 years ago

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

Please do it to avoid further confusion.

philipphofmann commented 2 years ago

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

philipphofmann commented 2 years ago

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