veeta-tv / roku-gamp

Google Analytics Measurement Protocol for Roku
MIT License
13 stars 4 forks source link

Add async HTTP req functionality #7

Closed doapp-ryanp closed 7 years ago

doapp-ryanp commented 7 years ago

This PR does not preserve backwards compat as it breaks existing initGAMobile() signature by adding a new mainThreadMsgPort param.

However, I feel the breaking change is justified as this is how GA should be used on Roku (best practice is to send Async AND to handle the events in the main event loop).

@silentpickle please code review. I think I was able to pull this off pretty cleanly, while maintaining good performance (I used a map cuz as you mention arrays are slow).

For what is worth, I have this working in a screen graph app on firmware 7.2.2+

doapp-ryanp commented 7 years ago

Forgot to mention this addresses https://github.com/veeta-tv/roku-gamp/issues/5 for others that run across that issue.

cdthompson commented 7 years ago

Before commenting on code specifics, I think the architecture can be simplified based on the expectation that the application will never be concerned about responses from GA. In that case, the pseudocode from @silentpickle on #5 looks like the approach we should take. The handleGaMobileHttpResponseEvent() cleanup logic can be run inside of the gamobileSendHit() function and there won't be a need for an application passing in a roMessagePort to initGAMobile().

Thoughts?

doapp-ryanp commented 7 years ago

So essentially you are saying run the async http resource garbage collection every time gamobileSendHit() is called? Feels kinda gross/inefficient and seems to limit flexibility if there is ever eventually a case to get HTTP response from GA.

With that said, I guess the requirement of roMessagePort could be added if/when that happens, and the full self-containment is probably worth the extra cpu overhead. I'll refactor and push here soon.

cdthompson commented 7 years ago

I have a few reasons to recommend the garbage collection every time gamobileSendHit().

First is ease of adoption into an existing channel. There typically are many messageports in use in an app with different scopes and lifetimes. To tie something global like GA to one messageport is extra overhead for the channel developer. For instance, sending a "Screen exit" right before the user pops the screen stack means the messageport goes out of scope and the developer would have to refactor code to track events on this messageport. It's easier to just drop in a single line of code for "sendHit()" and continue on with the app.

Second is that the overhead of walking the pending requests can probably be made up for by reusing the roUrlTransfer object from a finished request for the newly submitted request. This would be beneficial because a) CreateObject is more expensive than reusing an existing object, b) reusing a roUrlTransfer should take advantage of HTTP keepalive.

doapp-ryanp commented 7 years ago

Ok cleanup is now all self contained. I did not add logic to re-use of completed roUrlTransfer as I don't have time to implement at the moment.