Fireperf aqs sessionmanager phase2#7997
Fireperf aqs sessionmanager phase2#7997jrodiz wants to merge 8 commits intofirebase:fireperf-aqsfrom
Conversation
Delete the private no-arg constructor and static getInstance() method. The Dagger component factory in FirebasePerfRegistrar now constructs a fresh SessionManager(GaugeManager, PerfSession) instead of delegating to the singleton accessor. Add null-guards for gaugeManager in setApplicationContext, stopGaugeCollectionIfSessionRunningTooLong, logGaugeMetadataIfCollectionEnabled, and startOrStopCollectingGauges to support test-only SessionManager instances that are constructed with a null GaugeManager. Remove the testInstanceCreation unit test whose invariant is now maintained by the DI container rather than the class itself.
…artTrace pattern Add getInstance(SessionManager) seeded-init overload so FirebasePerfEarly can inject the DI-managed SessionManager at startup. The no-arg getInstance() remains as a safe "get-after-init" fallback (creates its own SessionManager if called before early init, e.g. in tests or AppStateUpdateHandler). Add getSessionManager() accessor and resetInstance() @VisibleForTesting helper to support test isolation. Update FirebasePerfEarly to call getInstance(sessionManager) instead of the no-arg version.
…() to TransportManager Trace's 5-arg convenience constructor now resolves SessionManager via sessionManagerFrom(appStateMonitor), which delegates to AppStateMonitor.getSessionManager() and falls back to a no-gauge instance when called in tests with a mocked AppStateMonitor. Trace's Parcel deserialization constructor now uses TransportManager.getInstance().getSessionManager() instead of SessionManager.getInstance(). This is safe because Parcel deserialization always occurs after FirebasePerfEarly has initialized the SDK. Add getSessionManager() accessor to TransportManager and thread sessionManager through initializeForTest() so test helpers can supply the correct instance.
The private single-arg constructor now obtains SessionManager via AppStateMonitor.getInstance().getSessionManager() instead of calling SessionManager.getInstance(). AppStateMonitor.getInstance() is safe here because FirebasePerfEarly (eager component) always seeds it before any network request can be intercepted by user code.
- FirebasePerfRegistrarTest: update component count (3→4) and dependency assertions to include SessionManager - FirebasePerformanceTest: construct spy SessionManager directly instead of spying on the now-deleted getInstance() return value - AppStartTraceTest: use base-class sessionManager field in @after reset instead of SessionManager.getInstance() - TraceTest: pre-seed AppStateMonitor with the test sessionManager in setUp; use sessionManager.updatePerfSession() in session-addition test - NetworkRequestMetricBuilderTest: same AppStateMonitor pre-seed pattern; use sessionManager.updatePerfSession() in session-addition test - TransportManagerTest: replace all SessionManager.getInstance() calls with the base-class sessionManager field; initializeForTest() now accepts and sets the sessionManager parameter
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
| private boolean isRegisteredForLifecycleCallbacks = false; | ||
| private boolean isColdStart = true; | ||
|
|
||
| public static AppStateMonitor getInstance(SessionManager sessionManager) { |
There was a problem hiding this comment.
Is there a way to use the getInstance() method instead of passing in the sessionManager as a parameter?
There was a problem hiding this comment.
Keeping the seeded overload intentionally — both methods are designed to coexist.
The no-arg getInstance() (line 95) is the fallback for call sites after early init (e.g. AppStateUpdateHandler, tests).
The seeded getInstance(SessionManager) is the DI injection point used by FirebasePerfEarly to propagate the Dagger-managed instance at startup. Replacing it with the no-arg form would mean constructing a throwaway SessionManager internally instead of the one supplied by the container, which defeats the purpose of this migration.
| if (appProcess.importance != ActivityManager.RunningAppProcessInfo.IMPORTANCE_FOREGROUND) { | ||
| continue; | ||
| } | ||
| // if (appProcess.processName.equals(appProcessName) |
There was a problem hiding this comment.
I'm guessing I might've accidentally commented this out in fireperf-aqs.
Can you uncomment it?
There was a problem hiding this comment.
Fix applied: Removed the dead-comment block — the variables it referenced (appProcessName, allowedAppProcessNamePrefix) were deleted in 35767dbe1 and no longer exist anywhere in the codebase. Uncommenting was not compilable; dropping the comment is the correct resolution.
| clock, | ||
| appStateMonitor, | ||
| gaugeManager, | ||
| sessionManagerFrom(appStateMonitor)); |
There was a problem hiding this comment.
IIUC this doesn't set the sessionManager instance in AppStateMonitor.
Is it possible to change this to appStateMonitor.getSessionManager()? If there's a case where it might return null, is it possible to document why that happens?
There was a problem hiding this comment.
Fix applied: Replaced sessionManagerFrom(appStateMonitor) with appStateMonitor.getSessionManager() directly. Added an inline comment explaining when it can be null (tests that don't pre-seed AppStateMonitor in setUp). Removed the now-unused sessionManagerFrom() helper. Also updated 5 test classes (TraceTest, TraceMetricBuilderTest, FirebasePerfTraceValidatorTest, FragmentStateMonitorTest, PerformanceTests) to stub mockAppStateMonitor.getSessionManager() — the null fallback in the old helper was silently hiding uninitialized mocks.
- Replace sessionManagerFrom(appStateMonitor) with appStateMonitor.getSessionManager() - Remove stale dead-comment block - Test fixes: Stub mockAppStateMonitor.getSessionManager() in setUp(); necessary because the null fallback in sessionManagerFrom() was silently hiding uninitialized mocks.
2 participants
Summary
Phase 2 of the SessionManager dependency-injection migration for Firebase Performance.
This continues the work started in #7675 (Phase 1: RemoteConfigManager refactor + SessionManager refactor groundwork) by fully eliminating the SessionManager.getInstance() static singleton in favor of Dagger-managed construction and explicit dependency passing.
Changes
instance.
NetworkRequestMetricBuilderTest, and TransportManagerTest to use the new DI-based patterns instead of the removed singleton.
Motivation
Removing the static singleton makes SessionManager's lifecycle explicit and testable, eliminates hidden global state, and aligns with the broader AQS (Android Quality Sessions) migration where session identity is provided by Firebase Sessions rather than managed internally.