Closed bawahakim closed 1 year ago
Thanks for your pull request. Is this your first contribution to a Snowplow open source project? Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
:memo: Please visit https://docs.snowplowanalytics.com/docs/contributing/contributor-license-agreement/ to learn more and sign.
Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.
I signed it!
Confirmed! @bawahakim has signed the Contributor License Agreement. Thanks so much.
Thank you for the contribution @bawahakim! We will review the PR as soon as we can.
Hi @bawahakim,
Thanks again for the contribution, great work!
There is a bit of code duplication in the AbstractEmitter
class. I tried to remove some by extracting the shared code between the SendRequests()
and SendRequestsAsync()
functions into some helper functions. There is a small breaking change in that I changed some of the protected methods in the AbstractEmitter
class, but I think that's not a big deal. I am sending a patch for the file attached as well as the complete file – could you please apply it if it makes sense to you?
One more thing that I'd like to ask you is if you could make the demo app in the repository work? I can see that it is still using the AsyncEmitter
(it's configured here). I tried to change it to the Webgl one, but I couldn't make it track events to my collector anyway.
@matus-tomlein Great changes, works perfectly and very clean! (although the Post request didn't work because it was always sending Get requests, fixed that).
I've merge in the master branch, resolved conflicts, and tested to work both in WebGl runtime & editor, and standalone editor.
Only concern left is there is some error, probably related to lite db which I haven't been able to look into (does not seem to break sending events, but probably breaks persistence): Utils: Error reading dictionary from file: at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.Int32 bufferSize, System.Boolean anonymous, System.IO.FileOptions options)
Motivation
Currently the plugin does not support WebGl, primarily because of threading and
HttpClient
.Solution
Adding asynchronous calls in the main thread, while awaiting a
UnityWebRequest
allows us to run in WebGl without blocking the main thread. An extension forUnityWebRequest
was taken from here.The code should be non breaking, as there are only additions to the abstract classes, and a new emitter for WebGl.
For the sake of keeping consistent, I mainly copy pasted the sync versions and made them async Task. It's not DRY and not the way I want it, but that was the best non-breaking way I could think of. Open to suggestions!
There is also the option to add UniTask, which would add support for cancellation token, which can be disposed when we stop the tracker. It would also remove the necessity for the awaiter extension.
Testing
I'm afraid I currently don't have time (or the much experience) in dotnet tests, hopefully someone else can pick this up :)
I have personally tested it working in a WebGl build in Unity 2021.3.7f1. I've also stress tested it by sending a request every 300ms without the thread blocking, and all requests fullfiling on our remote snowplow server.
I have not tested the async Get request as it is not used in our current project, but I suspect it should work as expected.
Known Issues
~Persistent data does not seem to be working~ Edit: It seems that the previous session ID is tracked correctly. Does that mean that persistent works?
Screenshots