Hybrid SDKs should set the `sentryClientName` on Native SDKs during SDK init.

marandaneto commented 2 years ago

The reason is that sentryClientName is logged out by Relay when there are errors.

For Hybrid SDKs, this is going to be always the Native SDKs because they are the ones sending the event over the wire. Ideally, the Hybrid SDKs overwrite this value so when there's errors, it's logged out as the Hybrid SDK itself instead of eg Android or iOS. The other benefit is that we can collect event metrics per SDK (Native events on an RN app will be counted as RN and not Native anymore).

Another option is that Relay logs the event.sdk instead of the USER_AGENT or sentry_client from the authHeader.

Android already has the SentryOptions#setSentryClientName, iOS to be confirmed, likely needs to be exposed in the PrivateSentrySDKOnly class, @brustolin can you confirm?

### Tasks
- [ ] https://github.com/getsentry/sentry-dart/issues/1185
- [ ] https://github.com/getsentry/sentry-react-native/issues/2813
- [ ] https://github.com/getsentry/sentry-dart/issues/1333
- [ ] https://github.com/getsentry/sentry-react-native/issues/2960
- [ ] https://github.com/getsentry/sentry-java/issues/2678
- [ ] https://github.com/getsentry/sentry-dotnet/pull/2389
- [ ] https://github.com/getsentry/sentry-dotnet/pull/2171
- [ ] https://github.com/getsentry/sentry-capacitor/issues/379

brustolin commented 2 years ago

iOS already have this option exposed in the `PrivateSentrySDKOnly' class.

+ (void)setSdkName:(NSString *)sdkName andVersionString:(NSString *)versionString;

marandaneto commented 2 years ago

iOS already have this option exposed in the `PrivateSentrySDKOnly' class.
+ (void)setSdkName:(NSString *)sdkName andVersionString:(NSString *)versionString;

Are you sure this field is being used for the HTTP request metadata and not only for the sdk.event? iOS uses SentryMeta, if setSdkName overwrites the SentryMeta field, then we don't need to change anything on iOS.

brustolin commented 2 years ago

Yes!

marandaneto commented 2 years ago

Consider the fact that this makes sense if the events are captured in the Hybrid SDKs, but if the Native SDK is capturing the events themselves, they should still keep the original sentryClientName.

marandaneto commented 1 year ago

We should set the sentry_client in the authHeader. https://develop.sentry.dev/sdk/overview/#authentication

brustolin commented 1 year ago

@marandaneto, isn't this what we are doing here:

https://github.com/getsentry/sentry-cocoa/blob/8.0.0/Sources/Sentry/SentryNSURLRequest.m#L107-L110

Maybe I didn't get your last comment.

marandaneto commented 1 year ago

@marandaneto, isn't this what we are doing here:

getsentry/sentry-cocoa@8.0.0/Sources/Sentry/SentryNSURLRequest.m#L107-L110

Maybe I didn't get your last comment.

It is, but the Hybrid SDKs don't overwrite this value yet.

marandaneto commented 1 year ago

@krystofwoldrich @mattjohnsonpint create an issue for your next major in case it's needed.

mattjohnsonpint commented 1 year ago

It seems there are three places where SDK name and version get sent to Sentry.

The sentry_client portion of the X-Sentry-Auth HTTP header
The sdk attribute in the envelope header
The sdk interface on the event payloads (both errors and transactions)

It also seems that all are optional. It would be good to have a better understanding of which should be sent. Perhaps all three?

Also, in the case of hybrid SDKs, what name values should we use? I'll use .NET as an example, but I think we should standardize. The sdk interface says the format should be two or three parts: entity.ecosystem[.flavor].

This is what I currently have:

Base .NET SDK (non-mobile): sentry.dotnet
.NET SDK for MAUI (any target platform): sentry.dotnet.maui
.NET SDK for Android without MAUI: sentry.dotnet.android
.NET SDK for iOS/MacCatalyst without MAUI: sentry.dotnet.cocoa

In thinking about this, I'm not sure that I should actually have sentry.dotnet.android / sentry.dotnet.cocoa at all. Perhaps those should just be sentry.dotnet? It uses the same managed .NET SDK code, and we can see the platform details separately in the device and runtime contexts.

I don't currently change the name of either the embedded native SDKs. If I did, would it be something like sentry.cocoa.dotnet / sentry.android.dotnet? The native SDK name coming first?

There seem to be two concerns in general with regard to name:

Attribution in reports (Looker, etc.) so we can properly understand utilization
Association with correct version number for both bug tracking and for tie-in with Sentry Release Registry (for version update nagging, wizards, etc.)

So... Say I set sentryClientName to sentry.cocoa.dotnet for iOS/MacCat and call SentryOptions.setSentryClientName with sentry.android.dotnet for Android. Does that satisfy both requirements? I think if all usage does a starts-with expression, then we're good. But I'm not sure that is the case?

krystofwoldrich commented 1 year ago

So the goal of this is to always report as the top layer SDK? In the case of React Native, it would be always sentry.javascript.react-native. No matter where the event comes from?

marandaneto commented 1 year ago

It seems there are three places where SDK name and version get sent to Sentry.

The sentry_client portion of the X-Sentry-Auth HTTP header

The sdk attribute in the envelope header

The sdk interface on the event payloads (both errors and transactions)

It also seems that all are optional. It would be good to have a better understanding of which should be sent. Perhaps all three?

Also, in the case of hybrid SDKs, what name values should we use? I'll use .NET as an example, but I think we should standardize. The sdk interface says the format should be two or three parts: entity.ecosystem[.flavor].

This is what I currently have:

Base .NET SDK (non-mobile): sentry.dotnet

.NET SDK for MAUI (any target platform): sentry.dotnet.maui

.NET SDK for Android without MAUI: sentry.dotnet.android

.NET SDK for iOS/MacCatalyst without MAUI: sentry.dotnet.cocoa

In thinking about this, I'm not sure that I should actually have sentry.dotnet.android / sentry.dotnet.cocoa at all. Perhaps those should just be sentry.dotnet? It uses the same managed .NET SDK code, and we can see the platform details separately in the device and runtime contexts.

I don't currently change the name of either the embedded native SDKs. If I did, would it be something like sentry.cocoa.dotnet / sentry.android.dotnet? The native SDK name coming first?

There seem to be two concerns in general with regard to name:

Attribution in reports (Looker, etc.) so we can properly understand utilization

Association with correct version number for both bug tracking and for tie-in with Sentry Release Registry (for version update nagging, wizards, etc.)

So... Say I set sentryClientName to sentry.cocoa.dotnet for iOS/MacCat and call SentryOptions.setSentryClientName with sentry.android.dotnet for Android. Does that satisfy both requirements? I think if all usage does a starts-with expression, then we're good. But I'm not sure that is the case?

Better to confirm with @bruno-garcia

Relay folks log the sdk from different sources depending on where the problem is, and you mentioned a few, the sentry_client header, the envelope header, and the event.sdk attribute, so I'd say all of them. The goal is to be able to pinpoint the faulty SDK, if this is sentry.dotnet.maui or something else, you tell me. On RN and Flutter we have decided to always use the Hybrid SDK no matter what, even if the problem is on the Android-native bits, we know it started from the RN bits, and from there we can investigate further, I know by looking at the event if this is an RN - Android issue or RN - iOS issue, so I don't need to specify a very specific SDK in there. I just wanna know if its a RN app with Native support or if its a pure Native app, no Hybrid SDKs at all.

Pay attention that the event metrics would go up/down with this change, which was fine after consulting folks.

The other benefit is that we can collect event metrics per SDK-platform (Native events on an RN app will be counted as RN and not Native anymore).

mattjohnsonpint commented 1 year ago

FYI, for Android, setSentryClientName did not take effect. Perhaps it's being overwritten somewhere. However, setting io.sentry.sdk.name in the app manifest did work. (Unity is doing that already.)

For iOS, PrivateSentrySdkOnly.setSdkName worked fine.

mattjohnsonpint commented 1 year ago

After a side discussion with @bruno-garcia - I think I see what happened here. This issue was originally only about the sentry_client header, set with sentryClientName. I was conflating that with all usages of the SDK name.

So perhaps for Android, setSentryClientName should be given a different value than the SDK name set by io.sentry.sdk.name in the manafest? If so, then to what value, and where would I see the result in the Sentry UI?

For iOS, I don't see anything other than PrivateSentrySdkOnly.setSdkName, so I'm not sure that the client name in the header can differ or not?

marandaneto commented 1 year ago

SDK docs update about the naming convention.

marandaneto commented 1 year ago

@mattjohnsonpint do we need to raise an issue for MAUI/etc for changing the SDK name for https://github.com/getsentry/sentry-native ?

marandaneto commented 1 year ago

@kahest can you check Capacitor? I guess it's not either, not sure if we want to anyway.

mattjohnsonpint commented 1 year ago

Maui doesn't use sentry-native directly. It does use sentry-cocoa (ios) or sentry-java (android) and sets the SDK names for both. Is there some setting on each of those to change the native sdk name that they bring in? I didn't think we needed to go more than one layer deep, no?

lucas-zimerman commented 1 year ago

Maui doesn't use sentry-native directly. It does use sentry-cocoa (ios) or sentry-java (android) and sets the SDK names for both. Is there some setting on each of those to change the native sdk name that they bring in? I didn't think we needed to go more than one layer deep, no?

When initialziing the SDK on Android you can change the Native SDK name by doing SentryAndroidOptions.setNativeSdkName

https://github.com/getsentry/sentry-java/blob/46b17825b8f7bec80e38cc66b3007baab2639fb7/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java#L416

marandaneto commented 1 year ago

Maui doesn't use sentry-native directly. It does use sentry-cocoa (ios) or sentry-java (android) and sets the SDK names for both. Is there some setting on each of those to change the native sdk name that they bring in? I didn't think we needed to go more than one layer deep, no?

That gives us better stats of events, otherwise, Native events (C/C++) on Android running MAUI etc will be counted as not coming from a .NET environment. We already did the changes on the other SDKs.

mattjohnsonpint commented 1 year ago

OK. Easy enough. Is there a similar option for Cocoa?

marandaneto commented 1 year ago

OK. Easy enough. Is there a similar option for Cocoa?

Yes, since v8, https://github.com/getsentry/sentry-cocoa/blob/4053ee90b75473db21ccfc2e1f361f22e2d0544c/Sources/Sentry/PrivateSentrySDKOnly.m#L93-L96

mattjohnsonpint commented 1 year ago

Right, but that's for the Cocoa SDK name. We set that to "sentry.cocoa.dotnet" already.

That's similar to how in Android we set io.sentry.sdk.name to "sentry.java.android.dotnet" in the app manifest.

Looking closer, I don't think the Cocoa SDK is using the Sentry Native SDK, so there's no equivalent of setNativeSdkName. Right?

marandaneto commented 1 year ago

Right, but that's for the Cocoa SDK name. We set that to "sentry.cocoa.dotnet" already.

That's similar to how in Android we set io.sentry.sdk.name to "sentry.java.android.dotnet" in the app manifest.

Looking closer, I don't think the Cocoa SDK is using the Sentry Native SDK, so there's no equivalent of setNativeSdkName. Right?

That is correct, Sentry-Native is Android only, and Sentry Cocoa does not embed any other SDK.

getsentry / team-mobile

Hybrid SDKs should set the `sentryClientName` on Native SDKs during SDK init. #48