Fireperf + AQS Draft PR #7091
Closed
Fireperf + AQS Draft PR #7091
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Based on the behaviour of AQS w/ Fireperf, an AQS session isn't available when (currently) logging gauge metadata. Changes: - Remove the current logging of gauge metadata - will be re-introduced in a future PR. - Switch Gauge collection from `scheduleAtFixedRate` to `scheduleWithFixedDelay`. As [documented](https://stackoverflow.com/a/78405653), this *should* prevent a potentially large amounts of gauge collection if a process is cached, and then restored during a verbose session - which *should* make it work better w/ AQS. - Remove API restricted behaviour which is no longer relevant.
This PR doesn't change the use of session ID to AQS - except in GaugeMetadata. I've added TODOs to identify the missing locations.
- Override PerfSession session id to aqs session id when possible - Add assertion for debug mode to double check if aqs session id available by the time of use
Leverage on `ProcessDetailsProvider` to retrieve process name info
- Update Gauge metrics collection frequency to be independent of Session Change. - Update GaugeMetric logging to log them based on metrics count - Necessary changes in SessionManager (remove app state monitor) There's a non-trivial number of unit test failures in GaugeManager. This PR marks them as Ignore with a TODO.
…6953) - Fixes relevant unit tests - specifically requiring a definite `ApplicationProcessState` - Adds new unit tests for the updated behaviour - Deletes unit tests that *should* be OK to delete.
- This is based on `firebase-sessions/src/main/kotlin/com/google/firebase/sessions/ProcessDetailsProvider.kt` - Usage in `AppStartTrace` and `TransportManager` - Reverts changes in `firebase-sessions/src/main/` to `main`
As part of looking into refactoring `GaugeCounter` I noticed I'd missed this increment.
- Update frequency of Gauge logging to better match the existing implementation. - Update GaugeMetadata logging to only log metadata if it's a verbose session.
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
Fix cold starts incorrectly using the previous firstSessionId and sessionIndex This came up during the dogfooding session When a cold start happens, we must always create a new session. Before this fix, it would preserve the current session's firstSesisonId and sessionIndex when that session hadn't expired. But a cold start is regardless of the current session expiring
Make the Sessions test app more convenient to test Initialize Firebase on the Application, not Activity, so it can be initialized in background services Fixed the process name for older Android API levels Made all the logcat tags be the same, so it will be easier to give test instructions like `tag:SessionsTestApp tag:FirebaseSessions` Added 2 background services with easy actions to interact with in the background, e.g., `am startservice -n com.google.firebase.testing.sessions/.MyServiceA -a PING`
Notify Sessions when a fatal crash occurs. Sessions will treat it like a background event. This will preserve session background time when a process ends due to a crash
📝 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. |
Firebase AI Mock Responses Check
|
There was a problem hiding this comment.
Summary of Changes
Hello @tejasd, 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 introduces a fundamental shift in how Firebase Performance Monitoring manages sessions by integrating it with the new Firebase Sessions SDK. The change centralizes session lifecycle, enables robust multi-process support, and enhances crash reporting integration, leading to more accurate and consistent session data.
Highlights
- Firebase Sessions Integration: Firebase Performance Monitoring (Fireperf) is now deeply integrated with the new Firebase Sessions SDK. Fireperf registers itself as a session subscriber, enabling centralized session management and consistent session IDs across Firebase products.
- Multi-Process Session Management: The Firebase Sessions SDK introduces robust multi-process session awareness. This includes new components like
ProcessDataManagerandSharedSessionRepositorywhich use DataStore for persistent session state, allowing for accurate cold start detection and session continuity even in complex multi-process Android applications. - Crashlytics Notification for Sessions: Firebase Crashlytics now notifies the Firebase Sessions SDK about fatal crashes via a new
CrashEventReceiverAPI. This ensures that the Sessions SDK can update the session state appropriately (e.g., marking the app as backgrounded) when a crash occurs. - Refactored Gauge Collection: Performance Monitoring's CPU and Memory gauge collection has been updated. A new
GaugeCountermechanism now triggers metric logging based on the number of collected data points, replacing fixed-rate flushing. Session sampling for gauges is also now deterministic, based on the session ID. - API and Internal Codebase Updates: Significant refactoring has occurred across both Fireperf and Firebase Sessions. This includes renaming
isGaugeAndEventCollectionEnabled()toisVerbose(), simplifyingPerfSessioncreation, and enhancing how application and process information is collected and reported in logged metrics.
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 pull request integrates Firebase Performance with the new Firebase Sessions SDK. The architectural improvements, such as adopting a centralized session management model and using a more robust multi-process data persistence layer, are commendable. I've identified a couple of critical issues related to API level compatibility that could lead to crashes on older Android versions. Addressing these points will be crucial for ensuring the stability of the SDK.
Comment on lines
+230
to
231
| return sysconf(OsConstants._SC_CLK_TCK); | ||
| } |
There was a problem hiding this comment.
The previous implementation had a check for VERSION.SDK_INT >= VERSION_CODES.LOLLIPOP before calling sysconf(OsConstants._SC_CLK_TCK). This was removed. OsConstants._SC_CLK_TCK is only available on API 21+. If the minSdkVersion for this module is less than 21, this will cause a NoSuchFieldError on older devices. Please ensure the minSdkVersion is at least 21 or restore the version check to prevent crashes.
Coverage Report 1Affected Products
Test Logs |
Size Report 1Affected Products
Test Logs |
Startup Time Report 1Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS. Notes
Startup Times
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.