-
-
Notifications
You must be signed in to change notification settings - Fork 58
Rework Initialization #2227
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
Rework Initialization #2227
Conversation
| { | ||
| // Since there is no screenshot added we can capture the feedback right away | ||
| SentryUnity.CaptureFeedback(_description.text, _email.text, _name.text, addScreenshot: false); | ||
| SentrySdk.CaptureFeedback(_description.text, _email.text, _name.text, addScreenshot: false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the desired effect. All things reside within SentrySdk.
| namespace Sentry.Unity | ||
| { | ||
| public static class SentryInitialization | ||
| internal static class SentryInitialization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can safely make this internal. The SDK ships with it's own .asmdef that allows us to hide internal types from the user.
| @@ -0,0 +1,15 @@ | |||
| using System; | |||
|
|
|||
| namespace Sentry.Unity.NativeUtils; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting the services into a very distinctive namespace to "discourage" usage.
| /// Used for loading the SentryUnityOptions from the ScriptableSentryUnityOptions during runtime. | ||
| /// </remarks> | ||
| public static SentryUnityOptions? LoadSentryUnityOptions(ISentryUnityInfo unityInfo) | ||
| public static SentryUnityOptions? LoadSentryUnityOptions() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer need to provide the IUnityInfo since these are getting set up during startup as part of the new platform services that are statically available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should get source generated in a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a better approach than the test I recommended 👍
| } | ||
|
|
||
| unitySdk._dotnetSdk = SentrySdk.Init(options); | ||
| unitySdk._dotnetSdk = Sentry.SentrySdk.Init(options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A drawback is that we now need to fully qualify calls through the static API from within the Unity SDK. But our users won't have that problem.
| // act | ||
| MainThreadData.SentrySystemInfo = sysInfo; | ||
| SentryUnity.Init(options); | ||
| SentrySdk.Init(options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No more separation between SentryUnity and SentrySdk.
bruno-garcia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I didn't notice this before but while in general I think this is a great approach, we now expect the static fields to be set before our public methods are called but we don't verify anywhere that's the case and we have a lot of null propagation patterns which hinder the ability to debug what was going on. In particular when customers change the init code, possibly bypassing our auto init/static property set in order to do things "by hand".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a better approach than the test I recommended 👍
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
bruno-garcia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some QOL suggestions but in general lgtm!
| if (options.NativeContextWriter is { } contextWriter) | ||
| { | ||
| SentrySdk.ConfigureScope((scope) => | ||
| Sentry.SentrySdk.CurrentHub.ConfigureScope((scope) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add to the top (or via csproj iirc): using Sntry = Sentry.SentrySdk and use only Sntry throughout the file/project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will lead to ambiguous references for the namespace Sentry.Unity - between Sentry and SentrySdk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could Sntry, without e (as Bruno suggested) work though?
…try-unity into fix/initialization
Fixes #2200
Required getsentry/sentry-dotnet#4322
Goal
As stated in the issue: The SDK needs to initializeable in two ways: self, and manual. Both these paths need to provide the same functionality.
Approach
Sentry.Unity.SentrySdk- This way the Unity SDK can omit certain methods like the .NET SDKsInitSentryInitialization.csthat is getting compiled with the gameInit. This allows users to call the same method.Implementation Details
Hide the .NET SDK
This is done by conditionally making the .NET SDKs
SentrySdks accessorinternalgetsentry/sentry-dotnet#4322The .NET SDK now considers Unity as a platform and our very own
.propsallows us to conditionally set a constant.Rename Rename
SentryUnitytoSentrySdkIdeally, we'd only have one static API that allows for discoverability of what's available. This could either be
SentrySdkorSentryUnity. I'm going withSentrySdkhere as:usings. This change will require users to change them fromusing Sentry;tousing Sentry.Unity. Which is still easier than having to go through every instance of calling into the SDK and change these fromSentrySdktoSentryUnity.Sentry.Unitynamespace and avoids conflicts. Attempting to accessSentry.SentrySdk.Capturewill create an errorCannot access internal class 'SentrySdk' here.summaryto the internalSentrySdkguiding users towards the right namespaceMake
Sentry.Unity.SentrySdkapartialclassThe Unity SDK version of
SentrySdkis now split in two partial classes.SentrySdk.csis where the Unity SDK specific code resides and basically contains "overwritten" methods likeInit,GetLastRunState.SentrySdk.Dotnet.cs- TODO: This should be source generated and end up asSentrySdk.Dotnet.g.csThe .NET SDK
SentrySdkis now hidden and inaccessible. To give users access to the rest of the API we're putting all the rest of the static API in there hand have it forward the calls into the .NET SDK.Hide the
.csscriptsSentryInitializationandSentryIntegrationsOur package ships with
io.sentry.asmdefwhich creates our own assembly for us within the game's project. This allows us to make these classesinternal, but keep their name. They are now undiscoverable for users - and should have been so in the first place.Changes to
SentryInitializationThe
SentryInitializationstill receives the "first thing that happens when the game starts callback via theRuntimeLoadattribute. This allows us to set up the SDK all the way to be ready to be initialized. Instead of doing the actual initialization inside the script that gets compiled with the game, this now injects the platform specific configure calls into the SDK.Add a static
SentryPlatformServicesThis is now the place that holds the injected platform specific configure callbacks and the
IUnityInfothat we use to access version and platform specific code like IL2CPP runtime checks and backend calls for line number support.We're setting these as the very first thing and can so guarantee that they are available, regardless whether the SDK auto-initializes or has the user call
Initmanually.The
PlatformServicesstay on the options becauseDue to how things are getting initialized I had to add a
SentryEditorPlatformSetUpthat sets theUnityInfoby virtue of theInitializeOnLoadMethodattribute. This allows us to have access to this even during build time (no runtime initialization there) and we can keep requiring it on theSentryUnityOptions.Move all initialization code into the SDK
Having the platform specific configure callbacks available via the
SentryPlatformServicesclass allows us to call conditionally compiled code from within the pre-compiled SDK. All initialization paths funnel back together in that oneInitcall.Migration Path
Unfortunately, this is a breaking change. As such, we need to provide a migration path. When updating the SDK users will face the following error on the console
Since we can modify the now internal SDK's
<summary>we can already put some help thereIDEs like Rider will allow to automatically import the missing references
Things to follow up on
using Sentry.Unityvs.using SentryThe entirety of the API is now discoverable within
SentrySdk.but the namespace is splitSentry- Contains the .NET SDK's types, i.e.SentryIdSentry.Unity- Contains the static APISo users end up in situations where they need to always bring
using Sentry.Unityand sometimes additionally needusing Sentry. That was an issue before already, it's just something to be aware of.Consider turning
IsEnabledin the config window intoisSelfInitEnabledDuring build the SDK was opinionated regarding whether to bring a native SDK and tied setup and symbol upload to the
isEnabledcheck. When we merged all option-configuration callbacks into one in #1888 the line already blurred.And now, the SDK cannot reasonably be certain whether when it will be initialized at runtime. With this I think we have to consider
IsEnabledcannot be considered anymoreIsEnabledandInitializationType.BuildTime-> setup native projectSource Generator for
Sentry.DotnetRight now the
Sentry.Dotnet.csis created by hand. It forwards all calls to the now internal static API while omittingInit. This allows us to keep the Unity specific methods in theSentrySdk.csfile but it also requires us to manually update if and when the .NET SDK's public API changes.Alternative (attempted) approaches
extension membersa try by bumping to C#14. But this cannot be consumed by the user's code.SentrySdk.Initmethodsinternal. This somewhat works but gets us stuck in a place. We'd have the API split betweenSentryUnity.InitvsSentrySdk.Capture.SentryUnityas name for the type for the static API. But - and that's an assumption on my part - that would break the actual calls fromSentrySdk.CaptureException()SentryUnity.CaptureException()instead of having the IDE fix theusingsand be done with it.Sentrynamespace. This did not work for conflicting reasons.