Fix encoding for Session Events #4870

samedson · 2023-04-06T20:29:55Z

Also updates logging to log the Encoded Session Event instead of the data object (only in debug mode) so that we can catch encoding issues

TODO (in a follow up)

Make sure we're setting the time correctly
Fill in Device Name

google-oss-bot · 2023-04-06T20:34:41Z

Coverage Report ¹

Affected Products

`firebase-sessions`

Overall coverage changed from ? (f7fff6b) to 55.14% (a47e519) by ?.

12 individual files with coverage change

Filename	Base (`f7fff6b`)	Merge (`a47e519`)	Diff
ApplicationInfo.kt	?	100.00%	?
EventGDTLogger.kt	?	75.00%	?
FirebaseSessions.kt	?	0.00%	?
FirebaseSessionsEarly.kt	?	0.00%	?
FirebaseSessionsRegistrar.kt	?	0.00%	?
SessionCoordinator.kt	?	76.47%	?
SessionEvent.kt	?	100.00%	?
SessionEvents.kt	?	97.06%	?
SessionGenerator.kt	?	22.73%	?
SessionInitiator.kt	?	0.00%	?
SessionsSettings.kt	?	0.00%	?
WallClock.kt	?	0.00%	?

Test Logs

Merge (a47e519): https://github.com/firebase/firebase-android-sdk/actions/runs/4660533285

https://storage.googleapis.com/firebase-sdk-metric-reports/ofHKRumnQd.html

github-actions · 2023-04-06T20:40:44Z

Unit Test Results

16 files 16 suites 25s ⏱️
17 tests 17 ✔️ 0 💤 0 ❌
34 runs 34 ✔️ 0 💤 0 ❌

Results for commit 482c401.

♻️ This comment has been updated with latest results.

google-oss-bot · 2023-04-06T20:43:33Z

Size Report ¹

Affected Products

base
Type Base (f7fff6b) Merge (a47e519) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 8.65 kB ? (?)
firebase-datatransport
Type Base (f7fff6b) Merge (a47e519) Diff
aar ? 4.94 kB ? (?)
apk (aggressive) ? 161 kB ? (?)
apk (release) ? 1.35 MB ? (?)
firebase-sessions
Type Base (f7fff6b) Merge (a47e519) Diff
aar ? 44.2 kB ? (?)
apk (aggressive) ? 251 kB ? (?)
apk (release) ? 1.77 MB ? (?)

Test Logs

Merge (a47e519): https://github.com/firebase/firebase-android-sdk/actions/runs/4660533285

https://storage.googleapis.com/firebase-sdk-metric-reports/HVQpsMEJ62.html

visumickey · 2023-04-10T19:56:40Z

Are these TODOs still to be done or are they verified?

visumickey · 2023-04-10T19:57:22Z

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/EventGDTLogger.kt

@@ -53,11 +54,14 @@ internal class EventGDTLogger(private val transportFactoryProvider: Provider<Tra
  }

  private fun encode(value: SessionEvent): ByteArray {
-    return SessionEvents.SESSION_EVENT_ENCODER.encode(value).toByteArray()
+    val jsonEvent = SessionEvents.SESSION_EVENT_ENCODER.encode(value)
+    Log.d(TAG, "Encoded Session Start event $jsonEvent")


Curious if you want to leave this as Debug or Verbose?

I'm down for Verbose - I wasn't sure when we use debug vs verbose

visumickey · 2023-04-10T19:58:09Z

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/EventGDTLogger.kt

  }

  companion object {
-    // TODO What do we put for the AQS Log Source
+    private const val TAG = "EventGDTLogger"


How would the log message look with this TAG? Would it contain FirebaseSessions anywhere? If not, I would recommend prefixing the tag with the library name.

It looks like: V/EventGDTLogger: Encoded Session Start event {"event_type":1...

We've been doing it like this so I'll update this file and others

visumickey · 2023-04-10T20:04:05Z

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/SessionEvent.kt

+internal data class DataCollectionStatus(
+  val performance: DataCollectionState = DataCollectionState.COLLECTION_ENABLED,
+  val crashlytics: DataCollectionState = DataCollectionState.COLLECTION_ENABLED,
+  val sessionSamplingRate: Double = 1.0,


Interesting to have sessionSamplingRate inside the DataCollectionStatus. I was thinking DataCollectionStatus is for external dependencies and used sessionSamplingRate as a remote config. But this looks a bit different from that intention. Curious to hear your rationale to have session sampling rate inside DataCollectionStatus.

This is how it is in our proto -
I wanted to be consistent with how it's defined: https://source.corp.google.com/piper///depot/google3/logs/proto/firebase/appquality/sessions/session_event.proto;l=67?q=DataCollectionStatus

visumickey · 2023-04-10T20:07:10Z

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/SessionEvent.kt

+  val performance: DataCollectionState = DataCollectionState.COLLECTION_ENABLED,
+  val crashlytics: DataCollectionState = DataCollectionState.COLLECTION_ENABLED,


Is the default COLLECTION_ENABLED (for now?)?

Yeah I'm putting it like this so it looks right in our data but we'll need to change this when we integrate with the product SDKs

visumickey · 2023-04-10T20:14:19Z

firebase-sessions/src/test/kotlin/com/google/firebase/sessions/SessionEventTest.kt

@@ -32,7 +32,8 @@ class SessionEventTest {
    val sessionEvent =
      SessionEvents.startSession(
        FakeFirebaseApp.fakeFirebaseApp(),
-        TestSessionEventData.TEST_SESSION_DETAILS
+        TestSessionEventData.TEST_SESSION_DETAILS,
+        TestSessionEventData.TEST_SESSION_TIMESTAMP_US,


Any reason why the event timestamp is configurable? Can we leave it as a constant?

startSession is also used by the SDK. When it leaves off the parameter it uses the current time. Since we can't have the time be different for every test I made it optional to just set the time

I think this is fine for now, but we need a proper time solution at some point. We can't just use go/javatime because it requires Android API level 26+. Maybe we can add another function in WallClock and try to abstract this away?

samedson · 2023-04-11T14:11:40Z

Are these TODOs still to be done or are they verified?

I'm doing the TODOs in a follow up PR

Fix encoding for other fields

5117a07

google-oss-bot added the size/L label Apr 6, 2023

firebase deleted a comment from google-oss-bot Apr 10, 2023

samedson added 2 commits April 10, 2023 14:21

Better

5547487

Update tests

26dee18

samedson added the no-changelog label Apr 10, 2023

firebase deleted a comment from google-oss-bot Apr 10, 2023

Fix tests

482c401

samedson requested review from mrober and visumickey April 10, 2023 19:25

samedson changed the title ~~Fix encoding for other fields~~ Fix encoding for Session Events Apr 10, 2023

visumickey approved these changes Apr 10, 2023

View reviewed changes

mrober approved these changes Apr 11, 2023

View reviewed changes

samedson merged commit 229d597 into firebase-sessions Apr 11, 2023

samedson deleted the fix-encoding branch April 11, 2023 14:12

samedson mentioned this pull request Apr 13, 2023

Move currentTimeUs into WallClock #4890

Merged

firebase locked and limited conversation to collaborators May 12, 2023

		val performance: DataCollectionState = DataCollectionState.COLLECTION_ENABLED,
		val crashlytics: DataCollectionState = DataCollectionState.COLLECTION_ENABLED,

Fix encoding for Session Events #4870

Fix encoding for Session Events #4870

Uh oh!

Conversation

samedson commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-bot commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

firebase-sessions

Test Logs

Uh oh!

github-actions bot commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

google-oss-bot commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

base

firebase-datatransport

firebase-sessions

Test Logs

Uh oh!

visumickey commented Apr 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samedson Apr 10, 2023 • 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!

samedson commented Apr 11, 2023

Uh oh!

Uh oh!

samedson commented Apr 6, 2023 •

edited

Loading

google-oss-bot commented Apr 6, 2023 •

edited

Loading

Coverage Report ¹

`firebase-sessions`

github-actions bot commented Apr 6, 2023 •

edited

Loading

google-oss-bot commented Apr 6, 2023 •

edited

Loading

Size Report ¹

`base`

`firebase-datatransport`

`firebase-sessions`

samedson Apr 10, 2023 •

edited

Loading