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

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

Closed
8 tasks done
marandaneto opened this issue Aug 19, 2022 · 23 comments
Closed
8 tasks done
Labels
bug Something isn't working

Comments

@marandaneto
Copy link

marandaneto commented Aug 19, 2022

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

  1. Breaking-change Effort: Small Impact: Large Platform: Dart
    marandaneto
  2. Effort: Small Impact: Medium Platform: React-Native Status: Backlog
    krystofwoldrich
  3. Blocked Effort: Medium Impact: Small Platform: Dart repository maintenance
    denrase
  4. Effort: Small Impact: Small Platform: React-Native Status: Backlog
    lucas-zimerman
  5. Effort: Small Platform: Android
    markushi
  6. Platform: Capacitor Status: Backlog
@brustolin
Copy link

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

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

@marandaneto
Copy link
Author

marandaneto commented Aug 19, 2022

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
Copy link

Yes!

@marandaneto
Copy link
Author

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
Copy link
Author

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

@brustolin
Copy link

@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
Copy link
Author

@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
Copy link
Author

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

@mattjohnsonpint
Copy link

mattjohnsonpint commented Dec 13, 2022

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

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:

  1. Attribution in reports (Looker, etc.) so we can properly understand utilization
  2. 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
Copy link
Member

krystofwoldrich commented Feb 7, 2023

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
Copy link
Author

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

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:

  1. Attribution in reports (Looker, etc.) so we can properly understand utilization
  2. 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
Copy link

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
Copy link

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
Copy link
Author

SDK docs update about the naming convention.

@marandaneto
Copy link
Author

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

@marandaneto
Copy link
Author

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

@mattjohnsonpint
Copy link

mattjohnsonpint commented May 23, 2023

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
Copy link

lucas-zimerman commented May 23, 2023

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
Copy link
Author

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
Copy link

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

@marandaneto
Copy link
Author

@mattjohnsonpint
Copy link

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

6 participants