Add SessionEvent data class #4763
Conversation
Generated by 🚫 Danger |
Coverage Report 1Affected Products
Test Logs |
Size Report 1Affected Products
Test Logs |
| Log.i(TAG, "Initiate session start") | ||
| val sessionState = sessionGenerator.generateNewSession() | ||
| val sessionEvent = | ||
| SessionEvent( |
There was a problem hiding this comment.
Could we encapsulate this in a class or function that generates the event based on the sessionState input? Eventually this will expand a lot once we have different classes to populate the session.
There was a problem hiding this comment.
This constructor may be a constructor on SessionEvent itself if that's possible for data classes
There was a problem hiding this comment.
The only for the data class ctor to take it, is to make it a property which seems ugly. So I make a function instead.
firebase-sessions/src/main/kotlin/com/google/firebase/sessions/FirebaseSessions.kt
Outdated
Show resolved
Hide resolved
| val sessionState = sessionGenerator.generateNewSession() | ||
| val sessionEvent = SessionEvent.sessionStart(sessionState) |
There was a problem hiding this comment.
I think this is very close to the iOS implementation. Just to confirm, here is how I think the current piece of code would evolve for event dispatch. Please let me know if you think otherwise.
- Session Generator is stateful and it returns sessions related information for the current session event. (ID, Index, previous ID, parent ID and likes)
2.SessionEvent class takes those details to fill in more details - Firebase Sessions later embellishes the event with details like data collection state.
- Firebase Sessions will use Firelog services to hand over the event.
Curious as to where would the config layer fit in here and how would sampling be done to either capture or skip an event?
There was a problem hiding this comment.
Yup, and for sampling we'll handle that when we do Settings. We will pass in something like collectEvents = shouldCollectEvents(...) to the SessionGenerator.
| package com.google.firebase.sessions | ||
|
|
||
| /** | ||
| * Contains the relevant information around an App Quality Session. |
There was a problem hiding this comment.
Lets not call it "App Quality Session" yet. Let's reference it as "Firebase Session Start Event".
There was a problem hiding this comment.
Done. Called it "Firebase Session Event"
| /** The type of event being reported. */ | ||
| val eventType: EventType, |
There was a problem hiding this comment.
Do we need to remember this state? It is already a part of SessionStartEvent, can we use it from there? I'm trying to avoid multiple source of truth here.
There was a problem hiding this comment.
This is there to match the proto 1:1, this data class will be serialized and sent to firelog.
| * https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseSessions/ProtoSupport/Protos/sessions.proto | ||
| */ | ||
| // TODO(mrober): Add and populate all fields from sessions.proto | ||
| internal data class SessionEvent( |
There was a problem hiding this comment.
Just confirming that this class will eventually do more things than what it is today: Add more App related fields to the session event?
There was a problem hiding this comment.
Yup this will eventually have all the fields from the proto.
firebase-sessions/src/main/kotlin/com/google/firebase/sessions/SessionGenerator.kt
Outdated
Show resolved
Hide resolved
| * | ||
| * @hide | ||
| */ | ||
| internal class SessionGenerator(collectEvents: Boolean) { | ||
| internal class SessionGenerator(private var collectEvents: Boolean) { |
There was a problem hiding this comment.
wow! We can call parameters with access protectors? private var is so cool! TIL!
Add SessionEvent data class and start populating some of the fields.
Renamed the internal class to SessionState to keep it separated from the data classes to be serialized.
#no-changelog