Merged sessions-sharedrepo into main #7039
Conversation
Use multi-process DataStore instead of Preferences DataStore. This change allows multiple processes to share the same datastore file safely. This reduces settings fetch to one per app run, not one per process. Also updated the TimeProvider to provide an object with explicit time units. This will make time less error prone. Removed all instances of `System.currentTimeMillis()` from tests, making them deterministic.
With the multi-process data store change, all processes will read the settings cache from the same file safely. This means if a second process started, it would read the cache the first process persisted. But if 2 processes were already running, and one fetched and cached new settings, it wouldn't automatically share it with the other running process. This change fixes that by having all processes watch the file. Also cleaned up settings a bit, and made everything in seconds to avoid converting units. Cleaned up unit tests. Also removed the need to lazy load the cache by doing a double check in the getter instead. There is more potential to clean up, but let's do it later.
Fix Robolectric tests showing warning about datastore_shared_counter by falling back to the normal DataStore instance. This is a known issue, see [b/352047731](http://b/352047731).
This removes all the bound service implementation, and starts the shared sessions repository implementation. The shared repository is responsible for sharing session data between processes in a simple way. This does not detect cold starts yet. The `SharedSessionRepository` class might be getting too much, we will consider splitting it up as implementation goes further.
send session event based on data collection and setting config
Fix where we generate and send session details to firelog. Before this, we might send false positives to firelog, or even update the session details with a stale copy. Now the session generate and send to firelog happen inside the datastore transform with the data in datastore.
Implement fake datastore for unit tests. This fake can act like a simple in memory datastore, but it can also throw provided exceptions on specific actions. It can throw on update, throw on collect, throw on init. This will help write unit tests for when datastore fails.
Using try catch block to make datastore fall back to a pre multi-process supported version. Generate a new session locally then notify to subscribers
Make background time nullable For each new session generated, reset background time Update unit tests
Implement new cold app start detection heuristic. This is just the reading part, and all the scaffolding. Next step will be to start writing the process data map. Tested by unit tests and manual testing with the sessions test app
Implement ProcessDataManager in the way we designed in https://docs.google.com/document/d/1yEd8dCIzwNwLhRRwkDtzWFx9vxV5d-eQU-P47wDL40k/edit?usp=sharing Tested manually in the sessions test app, and with unit tests
Implement cold start detection logic as described in https://docs.google.com/document/d/1yEd8dCIzwNwLhRRwkDtzWFx9vxV5d-eQU-P47wDL40k/edit?usp=sharing Also renamed process related functions to be more consistent to make the code less error prone. This is also described in the doc Tested by unit tests and with the sessions test app
Tested manually using sessions test app. This is a trivial change
Fix the profile case with clear app data. Now the app data will clear in the setup block, instead of the measure block. This also reads the output to make sure app data has cleared before proceeding to the measure block.
Use the same tag for all logs in sessions. This will make it easier to dogfood, and is consistent with Perf and Crashlytics If any context is lost by losing the tag named after the class, we can update those log lines as we run into them. A future change we can do is refactor to use the firebase common logger
Add more trace buttons to sessions test app to match the detailed test cases easier
Firebase AI Mock Responses Check
|
📝 PRs merging into main branchOur main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @mrober, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request integrates a new shared session repository implementation, which is a major architectural overhaul for the Firebase Sessions SDK. The primary goal is to facilitate Perf+AQS integration and enhance Crashlytics session reporting, particularly for early crashes, by enabling multi-process session awareness and improving cold app start detection. This involves a significant shift in how session data and settings are stored and managed across application processes.
Highlights
- Multi-Process Session Management: I've introduced a new
SharedSessionRepositoryand migrated session data persistence to a customDataStoreimplementation, leveragingkotlinx.serialization. This enables robust session management across all application processes, replacing the previousSessionLifecycleServiceandPreferences DataStoreapproach. - Improved Cold Start Detection: I've added a
ProcessDataManagercomponent to manage process-specific data and enhance the heuristic for detecting cold app starts. This is crucial for more accurate session reporting, especially for early crashes. - Architectural Refinement: The session lifecycle management has been significantly refactored. The SDK now uses direct
ActivityLifecycleCallbacksthat interact with theSharedSessionRepository, streamlining the overall architecture and removing the need for a dedicated service. - Settings Management Upgrade: Session settings persistence has been upgraded from
Preferences DataStoreto a customDataStorethat storesSessionConfigsobjects. This change improves data integrity, flexibility, and aligns with the new serialization strategy. - Test Infrastructure Enhancement: I've added new fake implementations (
FakeDataStore,FakeProcessDataManager,FakeSettingsCache) and updated existing tests to fully support the new multi-process and DataStore-based architecture, ensuring comprehensive test coverage for the refactored components.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This PR merges the sessions-sharedrepo branch, enabling multi-process support for Firebase Sessions. The core architectural changes involve adopting MultiProcessDataStoreFactory with Kotlinx Serialization, introducing SharedSessionRepository for centralized session logic, and enhancing cold start detection with ProcessDataManager. The lifecycle management has also been refactored. The changes appear robust, with a couple of points noted for consideration regarding exception logging and potential blocking calls during initialization.
| } | ||
| } | ||
| } catch (ex: Exception) { | ||
| Log.d(TAG, "App appForegrounded, failed to update data. Message: ${ex.message}") |
There was a problem hiding this comment.
Consider logging exceptions at a higher severity level (e.g., Log.w or Log.e) for better visibility of unexpected failures. Including the full stack trace (ex) would also aid debugging. Additionally, there's a minor typo: "App appForegrounded" should likely be "App foregrounded".
firebase-sessions/src/main/kotlin/com/google/firebase/sessions/settings/SettingsCache.kt
Show resolved
Hide resolved
Test Results 1 058 files ± 0 1 058 suites ±0 36m 2s ⏱️ +10s Results for commit 21215eb. ± Comparison against base commit f08e0c7. This pull request removes 28 and adds 21 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Size Report 1Affected Products
Test Logs |
Coverage Report 1Affected Products
Test Logs |
...essions/benchmark/src/main/kotlin/com/google/firebase/benchmark/sessions/StartupBenchmark.kt
Outdated
Show resolved
Hide resolved
Merged
sessions-sharedrepointomainThis merge brings in the new sessions shared repo implementation, which facilitates the Perf+AQS integration. This also includes improvements that will benefit Crashlytics e.g. providing sessions for early crashes
All individual commits on the
sessions-sharedrepobranch were reviewed and approved in their respective pull requests