Skip to content

Fix behaviour with multiple subscribers #5031

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
May 25, 2023
Merged

Fix behaviour with multiple subscribers #5031

merged 2 commits into from
May 25, 2023

Conversation

mrober
Copy link
Contributor

@mrober mrober commented May 24, 2023

Immediately call the callback if Sessions generated a session before the subscriber subscribed. iOS does this too. This will fix the FirebaseSessionsTest when having multiple subscribers in the test app.

@mrober mrober requested review from samedson and visumickey May 24, 2023 18:22
@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 24, 2023 •
edited
Loading

Coverage Report 1

Affected Products

  • firebase-crashlytics

    Overall coverage changed from ? (4303a9f) to 10.05% (17f511f) by ?.

    113 individual files with coverage change

    FilenameBase (4303a9f)Merge (17f511f)Diff
    AnalyticsDeferredProxy.java?0.00%?
    AnalyticsEventLogger.java?0.00%?
    AnalyticsEventReceiver.java?0.00%?
    AppData.java?50.00%?
    AutoCrashlyticsReportEncoder.java?57.82%?
    AutoValue_CrashlyticsReport.java?0.00%?
    AutoValue_CrashlyticsReportWithSessionId.java?0.00%?
    AutoValue_CrashlyticsReport_ApplicationExitInfo.java?66.34%?
    AutoValue_CrashlyticsReport_ApplicationExitInfo_BuildIdMappingForArch.java?0.00%?
    AutoValue_CrashlyticsReport_CustomAttribute.java?0.00%?
    AutoValue_CrashlyticsReport_FilesPayload.java?0.00%?
    AutoValue_CrashlyticsReport_FilesPayload_File.java?0.00%?
    AutoValue_CrashlyticsReport_Session.java?0.00%?
    AutoValue_CrashlyticsReport_Session_Application.java?0.00%?
    AutoValue_CrashlyticsReport_Session_Application_Organization.java?0.00%?
    AutoValue_CrashlyticsReport_Session_Device.java?0.00%?
    AutoValue_CrashlyticsReport_Session_Event.java?0.00%?
    AutoValue_CrashlyticsReport_Session_Event_Application.java?0.00%?
    AutoValue_CrashlyticsReport_Session_Event_Application_Execution.java?0.00%?
    AutoValue_CrashlyticsReport_Session_Event_Application_Execution_BinaryImage.java?0.00%?
    AutoValue_CrashlyticsReport_Session_Event_Application_Execution_Exception.java?0.00%?
    AutoValue_CrashlyticsReport_Session_Event_Application_Execution_Signal.java?0.00%?
    AutoValue_CrashlyticsReport_Session_Event_Application_Execution_Thread.java?0.00%?
    AutoValue_CrashlyticsReport_Session_Event_Application_Execution_Thread_Frame.java?0.00%?
    AutoValue_CrashlyticsReport_Session_Event_Device.java?0.00%?
    AutoValue_CrashlyticsReport_Session_Event_Log.java?0.00%?
    AutoValue_CrashlyticsReport_Session_OperatingSystem.java?0.00%?
    AutoValue_CrashlyticsReport_Session_User.java?0.00%?
    AutoValue_StaticSessionData.java?0.00%?
    AutoValue_StaticSessionData_AppData.java?6.00%?
    AutoValue_StaticSessionData_DeviceData.java?0.00%?
    AutoValue_StaticSessionData_OsData.java?0.00%?
    BackgroundPriorityRunnable.java?0.00%?
    BatteryState.java?0.00%?
    BlockingAnalyticsEventLogger.java?0.00%?
    BreadcrumbAnalyticsEventReceiver.java?0.00%?
    BreadcrumbHandler.java?0.00%?
    BreadcrumbSource.java?0.00%?
    BuildIdInfo.java?62.50%?
    BytesBackedNativeSessionFile.java?0.00%?
    CachedSettingsIo.java?0.00%?
    CLSUUID.java?0.00%?
    CommonUtils.java?0.00%?
    CrashlyticsAnalyticsListener.java?0.00%?
    CrashlyticsBackgroundWorker.java?56.25%?
    CrashlyticsController.java?16.49%?
    CrashlyticsCore.java?0.62%?
    CrashlyticsFileMarker.java?30.77%?
    CrashlyticsLifecycleEvents.java?0.00%?
    CrashlyticsNativeComponent.java?0.00%?
    CrashlyticsNativeComponentDeferredProxy.java?25.00%?
    CrashlyticsOriginAnalyticsEventLogger.java?0.00%?
    CrashlyticsRegistrar.java?0.00%?
    CrashlyticsReport.java?2.73%?
    CrashlyticsReportDataCapture.java?3.54%?
    CrashlyticsReportJsonTransform.java?1.18%?
    CrashlyticsReportPersistence.java?3.16%?
    CrashlyticsReportWithSessionId.java?0.00%?
    CrashlyticsUncaughtExceptionHandler.java?0.00%?
    CurrentTimeProvider.java?0.00%?
    CustomKeysAndValues.java?0.00%?
    DataCollectionArbiter.java?0.00%?
    DataTransportCrashlyticsReportSender.java?48.28%?
    DefaultSettingsJsonTransform.java?0.00%?
    DefaultSettingsSpiCall.java?0.00%?
    DeliveryMechanism.java?90.91%?
    DevelopmentPlatformProvider.java?0.00%?
    DisabledBreadcrumbSource.java?0.00%?
    ExecutorUtils.java?0.00%?
    FileBackedNativeSessionFile.java?0.00%?
    FileLogStore.java?0.00%?
    FileStore.java?36.36%?
    FirebaseCrashlytics.java?0.00%?
    HttpGetRequest.java?0.00%?
    HttpRequestFactory.java?0.00%?
    HttpResponse.java?0.00%?
    IdManager.java?3.13%?
    ImmutableList.java?0.00%?
    InstallerPackageNameProvider.java?0.00%?
    InstallIdProvider.java?0.00%?
    KeysMap.java?24.32%?
    LogFileManager.java?60.71%?
    Logger.java?40.54%?
    MetaDataStore.java?23.38%?
    MiddleOutFallbackStrategy.java?0.00%?
    MiddleOutStrategy.java?0.00%?
    NativeSessionFile.java?0.00%?
    NativeSessionFileGzipper.java?0.00%?
    NativeSessionFileProvider.java?0.00%?
    OnDemandCounter.java?0.00%?
    QueueFile.java?0.00%?
    QueueFileLogStore.java?7.58%?
    RemoveRepeatsStrategy.java?0.00%?
    ReportQueue.java?0.00%?
    ResponseParser.java?0.00%?
    SessionReportingCoordinator.java?44.76%?
    Settings.java?95.00%?
    SettingsCacheBehavior.java?0.00%?
    SettingsController.java?0.00%?
    SettingsJsonConstants.java?0.00%?
    SettingsJsonParser.java?0.00%?
    SettingsJsonTransform.java?0.00%?
    SettingsProvider.java?0.00%?
    SettingsRequest.java?0.00%?
    SettingsSpiCall.java?0.00%?
    SettingsV3JsonTransform.java?0.00%?
    StackTraceTrimmingStrategy.java?0.00%?
    StaticSessionData.java?12.50%?
    SystemCurrentTimeProvider.java?0.00%?
    TrimmedThrowableData.java?0.00%?
    UnavailableAnalyticsEventLogger.java?0.00%?
    UserMetadata.java?25.93%?
    Utils.java?0.00%?

  • firebase-sessions

    Overall coverage changed from ? (4303a9f) to 68.25% (17f511f) by ?.

    18 individual files with coverage change

    FilenameBase (4303a9f)Merge (17f511f)Diff
    ApplicationInfo.kt?100.00%?
    EventGDTLogger.kt?75.00%?
    FirebaseSessions.kt?0.00%?
    FirebaseSessionsDependencies.kt?91.30%?
    FirebaseSessionsRegistrar.kt?0.00%?
    LocalOverrideSettings.kt?88.46%?
    RemoteSettings.kt?86.25%?
    RemoteSettingsFetcher.kt?3.03%?
    SessionCoordinator.kt?75.00%?
    SessionEvent.kt?100.00%?
    SessionEvents.kt?98.73%?
    SessionGenerator.kt?19.05%?
    SessionInitiator.kt?66.67%?
    SessionsSettings.kt?90.00%?
    SessionSubscriber.kt?75.00%?
    SettingsCache.kt?94.12%?
    SettingsProvider.kt?0.00%?
    Time.kt?0.00%?

Test Logs

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

@github-actions
Copy link
Contributor

github-actions bot commented May 24, 2023 •
edited
Loading

Unit Test Results

  30 files    30 suites   1m 10s ⏱️
  62 tests   62 ✔️ 0 💤 0 ❌
124 runs  124 ✔️ 0 💤 0 ❌

Results for commit 2c39e8f.

♻️ This comment has been updated with latest results.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 24, 2023 •
edited
Loading

Size Report 1

Affected Products

  • base

    TypeBase (4303a9f)Merge (17f511f)Diff
    apk (aggressive)?8.39 kB? (?)
    apk (release)?8.65 kB? (?)

  • firebase-crashlytics

    TypeBase (4303a9f)Merge (17f511f)Diff
    aar?353 kB? (?)

  • firebase-datatransport

    TypeBase (4303a9f)Merge (17f511f)Diff
    aar?4.94 kB? (?)
    apk (aggressive)?161 kB? (?)
    apk (release)?1.35 MB? (?)

  • firebase-sessions

    TypeBase (4303a9f)Merge (17f511f)Diff
    aar?107 kB? (?)

Test Logs

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

visumickey
visumickey approved these changes May 24, 2023
View reviewed changes
firebase-sessions/src/main/kotlin/com/google/firebase/sessions/SessionGenerator.kt
@@ -46,6 +46,10 @@ internal class SessionGenerator(
lateinit var currentSession: SessionDetails
private set

/** Returns if a session has been generated. */
val hasGenerateSession: Boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do we use this value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used as the condition to send the current session when a subscriber subscribes in FirebaseSessions.

@mrober mrober merged commit 62e9e4b into firebase-sessions May 25, 2023
@mrober mrober deleted the sessions-fixtest branch May 25, 2023 21:49
@firebase firebase locked and limited conversation to collaborators Jun 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@visumickey visumickey visumickey approved these changes

@samedson samedson Awaiting requested review from samedson

Assignees
No one assigned
Labels
no-changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mrober @google-oss-bot @visumickey