Implement a SessionSubscriber for Firebase Performance #6683
Conversation
Release note changesNo release note changes were detected. If you made changes that should be |
Vertex AI Mock Responses Check
|
Generated by 🚫 Danger |
Coverage Report 1Affected Products
Test Logs |
Test Results 148 files 148 suites 3m 7s ⏱️ Results for commit afb0804. ♻️ This comment has been updated with latest results. |
Size Report 1Affected Products
Test Logs |
...-perf/src/main/java/com/google/firebase/perf/session/FirebasePerformanceSessionSubscriber.kt
Show resolved
Hide resolved
|
|
||
| class FirebasePerformanceSessionSubscriber(private val dataCollectionEnabled: Boolean) : | ||
| SessionSubscriber { | ||
| override val isDataCollectionEnabled: Boolean |
There was a problem hiding this comment.
Can you just put override val isDataCollectionEnabled: Boolean in the ctor?
| get() = dataCollectionEnabled | ||
|
|
||
| override val sessionSubscriberName: SessionSubscriber.Name | ||
| get() = SessionSubscriber.Name.PERFORMANCE |
There was a problem hiding this comment.
I don't think you need the explicit getter. Can you just say something like override val sessionSubscriberName = SessionSubscriber.Name.PERFORMANCE?
There was a problem hiding this comment.
If you copied this from Crashlytics, it's likely a result of Java Kotlin interop
| @@ -0,0 +1,49 @@ | |||
| /* | |||
| * Copyright 2024 Google LLC | |||
| } | ||
|
|
||
| /** Returns the AQS sessionId for the given session. */ | ||
| public void setAQSId(SessionSubscriber.SessionDetails aqs) { |
There was a problem hiding this comment.
Do we want the Perf session to have the id of the current AQS session when the Perf session starts, but then allow AQS to have different sessions due to background foreground etc? Because Crashlytics will have the current AQS session id at crash time added to the report.
There was a problem hiding this comment.
AQS knows the "first session id" and the current session id. If that is your concern, I think it's better to have the current aqs session id as the perf session id, and then if we need to do lookups we can join it to aqs data and know the first session id.
There was a problem hiding this comment.
I changed the behaviour a little compared to #6665.
This retains the PerfSession ID as is, which is used to identify sessions for gauge collection.
However, my plan is to use the AQS session ID to log the gauges. Whenever there is an AQS change, it will also result in a new PerfSession.
| return aqsSessionId; | ||
| } | ||
|
|
||
| /** Returns the AQS sessionId for the given session. */ |
| public String sessionId() { | ||
| return sessionId; | ||
| } | ||
|
|
||
| /** Returns the AQS sessionId for the given session. */ | ||
| public String aqsSessionId() { |
This PR doesn't change the use of session ID to AQS - except in GaugeMetadata. I've added TODOs to identify the missing locations.
This is a follow up for: #6678
This PR doesn't change the use of session ID to AQS - except in GaugeMetadata. I've added TODOs to identify the missing locations.