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

Expose ribActionEvents stream #599

Merged
merged 20 commits into from
Jun 23, 2023
Merged

Conversation

FranAguilera
Copy link
Contributor

@FranAguilera FranAguilera commented Jun 14, 2023

Description:
Expose ribActionEvents stream that emits state of each RIB "action" (e.g didBecomeActive, willLoad) For example:

Event to be emitted just before didBecomeActive
Interactor.didBecomeActive internal call
Even to be emitted after didBecomeActive call is completed

One example application of consuming these events is monitoring binding duration of Workers as shown in ApplicationLevelWorkerLogger from rib-workers demo app
...

WARNING: [RibEvents.enableRibActionEmissions] must be called at the earliest app scope before subscribing/collecting ribAction events. This is intentional in order to avoid unnecessary object creations for each RIB action in case no one is listening

Breaking changes:

Change Notes
WorkerBinder.initializeMonitoring Removed. Moving forward can use new flow API
WorkerBinderListener Removed. Moving forward can use new flow API
RibEvents Converted to Object, no need for Singleton from now on
RibEvent Named to RibRouterEvent
events within RibEvents Renamed events to routerEvents

Tests

  • JUnit tests
  • Manual validation on internal repo
  • Demo on rib-workers sample app
    image

Related issue(s):

Provide a new RibActionEvent stream that will allow for monitoring/logging options for all RIB components (Interactor, Presenter, Router, Worker)

@FranAguilera FranAguilera marked this pull request as draft June 14, 2023 17:06
@FranAguilera FranAguilera force-pushed the add_rib_monitoring branch 4 times, most recently from 2bb5dde to 6e1018b Compare June 14, 2023 20:21
@FranAguilera FranAguilera added the Android Android related tickets label Jun 15, 2023
@FranAguilera FranAguilera force-pushed the add_rib_monitoring branch 6 times, most recently from 9b2f281 to 2ac48ff Compare June 16, 2023 16:25
@FranAguilera FranAguilera changed the title Drafting monitoring option for RIBs [Draft] Expose flow with RibEvent information duration Jun 16, 2023
@FranAguilera FranAguilera requested a review from tyvsmith June 16, 2023 18:12
@FranAguilera FranAguilera force-pushed the add_rib_monitoring branch 4 times, most recently from c7674d8 to 60a8308 Compare June 16, 2023 19:19
@FranAguilera FranAguilera changed the title [Draft] Expose flow with RibEvent information duration [Draft] Expose flow with RibEventDuration information Jun 16, 2023
@FranAguilera FranAguilera force-pushed the add_rib_monitoring branch 3 times, most recently from 29c7d6d to 715defd Compare June 16, 2023 19:57
@psteiger
Copy link
Contributor

psteiger commented Jun 16, 2023

As discussed offline, we could have a DSL like:

inline fun record(startEvent: StartEvent, endEvent: EndEvent, block: () -> Unit) {
  // emit start event
  block()
  // emit end event
}

And in interactors (e.g.) we could have

fun didBecomeActive() {
  record(Interactor.Start, Interactor.Stop) {
      // code for didBecomeActive
  }
}

If we don't do the calculation on the record, and instead do it on consumer side, we can offload the calculation to a worker thread.

Also, we could have an atomic identifier for each record call, incremented at every call, making us sure that the 'end event' corresponds to a certain 'start event' (if needed)

@FranAguilera FranAguilera marked this pull request as ready for review June 21, 2023 16:57
@FranAguilera FranAguilera requested review from psteiger and rysh88 June 21, 2023 17:08
- Use MainScope for ApplicationLevelWorkerLogger demo
- Add thread name to always keep track of original caller thread name

fun interface BackendReporter {
fun report(genericMessage: String)
public enum class RibComponentType {
Copy link
Member

@tyvsmith tyvsmith Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coroutine Worker? Why reserve for next release?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may need to tweak a bit more RibCoroutine worker in order to provide accurate monitoring as mentioned by @psteiger earlier

Given there are barely any users internally of RibCoroutineWorker, I'm not concerned to leave these changes to be implemented on a separate PR

* @param ribEventType RIB event type (e.g. ATTACH/DETACH)
*/
internal inline fun <T : Any> triggerRibActionAndEmitEvents(
ribCallerClassType: T,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we lock this down more than T? With the use of lambda's, this, it, and T here it seems like the risk is high that we end up not tracking the correct class we're expecting.

Is there a common interface we can use here? If not, should we have one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, let me explore

Copy link
Contributor Author

@FranAguilera FranAguilera Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a "RibComponent" contract for these common components, still thinking a better naming for this

internal inline fun <T : Any> triggerRibActionAndEmitEvents(
ribCallerClassType: T,
internal inline fun triggerRibActionAndEmitEvents(
ribComponent: RibComponent,
ribComponentType: RibComponentType,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to specify a property on the new RibComponent to be overriden, unfortunately given that Worker and RibCoroutineWorker are interfaces we cannot restrict visibility of those

@@ -34,7 +34,7 @@ protected constructor(
public open val interactor: I,
private val ribRefWatcher: RibRefWatcher,
private val mainThread: Thread,
) {
) : RibComponent {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably too generic for a name for this set of callbacks. How about something more explicit around emitting callbacks around lifecycles, like RibEventEmitter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I like this better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to go with RibActionEmitter to avoid confusion with existing RibEventType

@FranAguilera FranAguilera requested a review from tyvsmith June 22, 2023 20:21
@tyvsmith tyvsmith merged commit c1cf74c into uber:main Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Android related tickets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants