Publish appStateChanged/appMemoryLow events #677

georgezy-amzn · 2025-08-26T18:40:24Z

ℹ️ Description

Publish appStateChanged/appMemoryLow events
Publish appState event attribute

Issue #, if available

Type of change

🧪 How Has This Been Tested?

Tested with demo on real device, verifying if app enter foreground/background events are published
Unit tests added for the change

Unit test coverage

Class coverage:
Line coverage:

Additional Manual Test

Pause and resume remote video
Switch local camera
Rotate screen back and forth

📱 Screenshots, if available

provide screenshots/video record if there's a UI change in demo app

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

hensmi-amazon · 2025-08-26T19:00:51Z

.../java/com/amazonaws/services/chime/sdk/meetings/internal/audio/DefaultAudioClientObserver.kt

            GlobalScope.launch {
                audioClient?.stopSession()
+                appLifecycleObserver.stopObserving()


Does this seem like an odd place to put this? No higher level observer?

Yeah, this is the highest level we can get, an alternative is to monitor the state change of DefaultAudioClientController.state, but I don't like it.

hensmi-amazon · 2025-08-26T19:01:39Z

.../java/com/amazonaws/services/chime/sdk/meetings/internal/audio/DefaultAudioClientObserver.kt

@@ -570,8 +572,10 @@ class DefaultAudioClientObserver(

    private fun handleAudioClientStop(statusCode: MeetingSessionStatusCode?) {
        if (audioClient != null) {
+            // TODO: assess if only notifyAudioClientObserver should be in GlobalScope.launch


Curious about this, I think GlobalScope is a reasonable default, though it would be nice to allow configuration.

I'm OK with GlobalScope, what I mean is we should only have the notifyAudioClientObserver logic in the GlobalScope, the rest of the logic should stay on the same thread as where handleAudioClientStop is called, so that we can reduce the async overheads, but I need to think about it before making the change.

The overall code should look like this:

private fun handleAudioClientStop(statusCode: MeetingSessionStatusCode?) { if (audioClient != null) { audioClient?.stopSession() appLifecycleObserver.stopObserving() DefaultAudioClientController.audioClientState = AudioClientState.STOPPED GlobalScope.launch { notifyAudioClientObserver { observer -> observer.onAudioSessionStopped(MeetingSessionStatus(statusCode)) } } } else { logger.error(TAG, "Failed to stop audio session since audioClient is null") } }

hensmi-amazon · 2025-08-26T19:02:15Z

...main/java/com/amazonaws/services/chime/sdk/meetings/ingestion/DefaultAppLifecycleObserver.kt

+import com.amazonaws.services.chime.sdk.meetings.analytics.MeetingHistoryEventName
+import com.amazonaws.services.chime.sdk.meetings.utils.logger.Logger
+
+class DefaultAppLifecycleObserver(


should this be Monitor to differentiate from observer pattern?

Sure, I don't have a preference.

hensmi-amazon · 2025-08-26T19:05:30Z

...main/java/com/amazonaws/services/chime/sdk/meetings/ingestion/DefaultAppLifecycleObserver.kt

+class DefaultAppLifecycleObserver(
+    private val eventAnalyticsController: EventAnalyticsController,
+    private val logger: Logger
+) : AppLifecycleObserver, DefaultLifecycleObserver {


Can we subscribe to other observers as well? https://developer.android.com/reference/androidx/lifecycle/DefaultLifecycleObserver

We could technically make this just one event type + attribute as well

…te attribute

hensmi-amazon · 2025-09-02T17:11:41Z

.../java/com/amazonaws/services/chime/sdk/meetings/analytics/DefaultEventAnalyticsController.kt

    private var eventAnalyticsObservers = ConcurrentSet.createConcurrentSet<EventAnalyticsObserver>()

-    override fun publishEvent(name: EventName, attributes: EventAttributes?) {
+    override fun publishEvent(name: EventName, attributes: EventAttributes?, notifyObservers: Boolean) {


Curious, why is this optional?

If you mean attributes:EventAttributes?, my understanding is because not every event comes with attributes. Alternatively we can also use non-optional EventAttributes and set to empty map for default, I'm not sure why we didn't go this approach.

I meant notifyObservers

Currently there are publishEvent() and pushEvent() in DefaultEventAnalyticsController, the only diff between them is that pushEvent() does not notify event observers, the rest of the logic is the same, so i'm tying to remove pushEvent(), and use publishEvent() for all events, adding notifyObservers in order to make publishEvent() have the same pattern as pushEvent().

hensmi-amazon · 2025-09-02T17:12:06Z

...rc/main/java/com/amazonaws/services/chime/sdk/meetings/analytics/EventAnalyticsController.kt

@@ -16,7 +16,7 @@ interface EventAnalyticsController {
     * @param name: [EventName] - Name of event to publish
     * @param attributes: [EventAttributes] - Attributes of event to pass to builders.


Needs update, and if so probably explanation of why this may be false.

Nice catch!

pracheth · 2025-09-02T17:48:26Z

...dk/src/main/java/com/amazonaws/services/chime/sdk/meetings/ingestion/AppLifecycleObserver.kt

+package com.amazonaws.services.chime.sdk.meetings.ingestion
+
+interface AppLifecycleObserver {
+    fun startObserving()


Lets make these start() and stop(). Having the word Observing in the API name sounds redundant.

linsang21 · 2025-09-02T17:55:31Z

.../java/com/amazonaws/services/chime/sdk/meetings/analytics/DefaultEventAnalyticsController.kt

        eventReporter?.report(SDKEvent(name, eventAttributes))

-        ObserverUtils.notifyObserverOnMainThread(eventAnalyticsObservers) {
-            it.onEventReceived(name, eventAttributes)
+        if (notifyObservers) {


What's the reason for making notification optional?

This is the first step to replace pushHistory() with publishEvent() - the only different between those 2 functions is publishHistory() does not notify event observers, so we should merge those 2 functions(at least merge the logic if we still want to keep the signature). Having notification as optional will make the logic same(or closer) to pushHistory().

Maybe I missed the discussion, do we deprecate pushHistory? Why do we want to merge them?

There is no discussion, this is just a change I'm planning to make.

Why do we want to merge them?
Because those functions are doing almost the same thing except notifying the event observers, it's causing confusions and not necessary to have duplicate logics.

linsang21 · 2025-09-02T17:58:54Z

...rc/main/java/com/amazonaws/services/chime/sdk/meetings/analytics/EventAnalyticsController.kt

@@ -15,8 +15,9 @@ interface EventAnalyticsController {
     *
     * @param name: [EventName] - Name of event to publish
     * @param attributes: [EventAttributes] - Attributes of event to pass to builders.
+     * @param notifyObservers: [Boolean] - Whether to notify `EventAnalyticsObserver` of the events.


I'll prefer the explanation be more use case oriented, i.e. - when am I recommended to notify EventAnalyticsObserver and when not.

linsang21 · 2025-09-02T18:05:30Z

.../src/main/java/com/amazonaws/services/chime/sdk/meetings/ingestion/DefaultAppStateMonitor.kt

+
+    private var handler: AppStateHandler? = null
+
+    // App states should be posted only when the meeting session is running


Nit: meeting session isn't the hard requirement right? builder still can post events when use this monitor alone

It's a privacy concern, IMO it makes sense to monitor the states only when the session is running, if we monitor the app states all the time regardless if the session is running, it could lead to customer concerns. Builders can do whatever they want since they own the app.

linsang21 · 2025-09-02T18:24:22Z

...test/java/com/amazonaws/services/chime/sdk/meetings/ingestion/DefaultAppStateMonitorTests.kt

+        appStateMonitor.start()
+        appStateMonitor.start()
+
+        verify(exactly = 2) { mockLogger.info(any(), "Started monitoring app state and memory") }


These idempotent test cases are not how I exactly imagine - I think it should verify something only happen once even you call the API multiple time.

And wondering if any alternative for verification method instead of matching texts in log which is relatively less stable.

These idempotent test cases are not how I exactly imagine - I think it should verify something only happen once even you call the API multiple time.

I think idempotent means it will have the effect as it performed once when performed multiple times, it doesn't have to happen once, as long as the result is the same. In the start() logic, we are resetting the observers before subscribing, so the logger will be called twice.

And wondering if any alternative for verification method instead of matching texts in log which is relatively less stable.

I would say it's a more accurate verification by matching the texts, instead of less stable.

I agree "I think idempotent means it will have the effect as it performed once when performed multiple times", and that's why I feel the name is bit misleading to me, the effect is now 2 logs instead of 1 log. It is idempotent to me if only 1 log for multiple calls that prevent additional resources to be created. I'm good with naming like "start should resetting observers".

Stable here I meant if I update the log text, the test would fail unnecessarily, essentially the logic here isn't relying on the logging output but the API invocation. Ideally we could verify memoryHandler.post get called.

Make sense, will update!

linsang21 · 2025-09-02T18:27:16Z

guides/meeting_events.md

-|`contentShareSignalingDropped`     |The content share WebSocket failed or closed with an error.
+|`videoClientSignalingDropped`      |The video client signaling websocket failed or closed with an error.
+|`contentShareSignalingDropped`     |The content share client signaling websocket failed or closed with an error.
+|`appStateChanged`                  |The application state is changed.


Do we need to update. for detail attributes for the states?

We added appState attribute.

I meant do we need to list what are the available states?

georgezy-amzn added 3 commits August 25, 2025 15:31

Publish app enter foreground/background events

23d0bc1

Update tests

ea898af

Update CHANGELOG

10cf910

hensmi-amazon reviewed Aug 26, 2025

View reviewed changes

Replace fore/background events with appStateChanged event, add appSta…

2b7b825

…te attribute

georgezy-amzn changed the title ~~Publish app enter foreground/background events~~ Publish appStateChanged/appMemoryLow events Sep 2, 2025

hensmi-amazon reviewed Sep 2, 2025

View reviewed changes

pracheth reviewed Sep 2, 2025

View reviewed changes

Fix doc string

b3b289c

georgezy-amzn force-pushed the app-life-cycle branch from a93b6fb to b3b289c Compare September 2, 2025 17:53

linsang21 reviewed Sep 2, 2025

View reviewed changes

georgezy-amzn added 2 commits September 2, 2025 15:19

Update unit tests

1c8d19d

Update doc

22598c7

linsang21 approved these changes Sep 2, 2025

View reviewed changes

hensmi-amazon approved these changes Sep 4, 2025

View reviewed changes

		@@ -16,7 +16,7 @@ interface EventAnalyticsController {
		* @param name: [EventName] - Name of event to publish
		* @param attributes: [EventAttributes] - Attributes of event to pass to builders.


		private var handler: AppStateHandler? = null

		// App states should be posted only when the meeting session is running

Publish appStateChanged/appMemoryLow events #677

Are you sure you want to change the base?

Publish appStateChanged/appMemoryLow events #677

Uh oh!

Conversation

georgezy-amzn commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ℹ️ Description

Issue #, if available

Type of change

🧪 How Has This Been Tested?

Unit test coverage

Additional Manual Test

📱 Screenshots, if available

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgezy-amzn Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

georgezy-amzn commented Aug 26, 2025 •

edited

Loading

georgezy-amzn Aug 26, 2025 •

edited

Loading