Repository navigation
feat: WebGL - dotnet support - #657
Conversation
|
vaind
changed the title
|
@vaind - Please try layering on top of the changes I made with getsentry/sentry-dotnet#1560 and let me know if you have any pain points. I think it should let your code be much cleaner. Thanks. |
e9f9faf to
5a15262
Compare
edd5d89 to
db050d4
Compare
bruno-garcia
left a comment
bruno-garcia
left a comment
There was a problem hiding this comment.
This is awesome! 
I left some notes for and nits for reference
|
|
||
| public bool EnqueueEnvelope(Envelope envelope) | ||
| { | ||
| _ = _behaviour.StartCoroutine(_transport.SendEnvelopeAsync(envelope)); |
There was a problem hiding this comment.
It's possible that we'll need some form of back pressure in a follow up PR.
If we capture events in a tight loop, would this negatively affect performance?
In other SDKs (I believe the JavaScript SDK does this) we cap the number of futures we spawn to a number by tracking how many async operations are in-flight. In the .NET SDK it's done through the background worker, but here we could have a counter and only start a new coroutine if less than N are currently running
| public int QueuedItems { get; } | ||
| } | ||
|
|
||
| internal class UnityWebRequestTransport : HttpTransportBase |
There was a problem hiding this comment.
Even though it's not the goal of this PR. this transport has the potential to unblock usages outside WebGL. Mainly Console platforms. We can refactor things out to be reused later, so that perhaps a selected number of platforms. Possibly even: Anything that's not desktop and mobile
There was a problem hiding this comment.
that's why I've moved it to a separate file in the Sentry.Unity csproj - should be a good starting point without major git-diffs when we want to use for another platform.
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
|
Hi @vaind ! Thank you for your work on WebGL support for this SDK. We're Sentry customers utilizing the tool on other platforms, so we're excited to be able to use it for WebGL too. I had a quick question: as far as you know, are there Unity Editor version minimums for Sentry Unity <> WebGL compatibility? |
Glad you find it useful. It should work on standard versions as for the rest of the SDK, i.e. 2019, 2020, 2021 at the moment. We're testing WebGL specifically in our integration test CI on those. |
Thanks for getting back to me so fast. Sounds good! |
|
Awesome work! So glad to have WebGL dotnet support! |
7 participants
Approaches
I've gone through multiple iterations/alternative approaches. You can see them in previous commits:
HttpTransport- https://github.com/getsentry/sentry-unity/blob/472cb003bb1cf9c04d380b4909c4ae3faeeff451/src/Sentry.Unity/WebGL/SentryWebGL.csHttpMessageHandlerwhile using the fullHttpTransportwithout any changes needed - this looked promising up until the point where I couldn't getawaitthat polute the wholeHttpTransportto ever complete (continue) on WebGL - https://github.com/getsentry/sentry-unity/blob/bb993e70ad4bc1a999b4097e2c86b2aff8b69eb3/src/Sentry.Unity/WebGL/SentryWebGL.csHttpTransportwith a custom implementation for the async stuff -> noawait- the most complete approach so far - https://github.com/getsentry/sentry-unity/blob/2bbbcd905125398683dc39441409b280c68c4f23/src/Sentry.Unity/WebGL/SentryWebGL.csMVP TODOs
Make HttpTransport public (for other SDKs) sentry-dotnet#1553Add HttpTransport extensibility and synchronous serialization support sentry-dotnet#1560 is mergedFuture PRs
"com.unity.modules.unitywebrequest": "1.0.0"package is disabled - otherwise Unity just throws a vague error:Cannot create web request without initializing the systemthat I couldn't get any useful resolution for by googling - requires editor build script so could be also part of WebGL JavaScript & native error handling #668 which brings that.