Skip to content
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.

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

FirebaseApp: send Core Diagnostics data on appDidBecomeActive #3437

Merged
merged 6 commits into from Jul 29, 2019

Conversation

maksymmalyhin
Copy link
Contributor

@maksymmalyhin maksymmalyhin commented Jul 25, 2019

It is needed to send reliable heartbeats even if an application is not relaunched for a long time.

@@ -796,4 +806,31 @@ - (void)sendLogsWithServiceName:(NSString *)serviceName
}
#pragma clang diagnostic pop

#pragma mark - App Life Cycle
Copy link
Contributor

@mikehaney24 mikehaney24 Jul 25, 2019

Choose a reason for hiding this comment

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

Just FYI, there is some lifecycle code in GDTLifecycle. I'm not sure if you want to adopt that protocol or not.

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Jul 26, 2019

Choose a reason for hiding this comment

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

It will be nice to reuse it. Is GoogleDataTransport meant to be a required dependency for FirebaseCore?
cc @paulb777

Copy link
Contributor

@mikehaney24 mikehaney24 Jul 26, 2019

Choose a reason for hiding this comment

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

GDT won't come in if FirebaseCoreDiagnostics isn't present, so I wouldn't add a hard dependency on it.


- (void)logDiagnostics {
if ([self isDataCollectionDefaultEnabled]) {
[FIRCoreDiagnosticsConnector logConfigureCoreWithOptions:_options];
Copy link
Contributor

@mikehaney24 mikehaney24 Jul 25, 2019

Choose a reason for hiding this comment

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

Did you want to create a new logging method, or keep as-is?

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Jul 26, 2019

Choose a reason for hiding this comment

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

I want to log the same data. Now I think, logConfigureCoreWithOptions can be renamed to something like logCoreDataWithOptions. WDYT?

*
* @param options The options object containing data to log.
*/
+ (void)logConfigureCoreWithOptions:(FIROptions *)options;
+ (void)logCoreDataWithOptions:(FIROptions *)options;
Copy link
Contributor

@mikehaney24 mikehaney24 Jul 26, 2019

Choose a reason for hiding this comment

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

maybe logCoreTelemetry? CoreData has a pretty specific meaning in iOS.

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Jul 26, 2019

Choose a reason for hiding this comment

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

yeah, logCoreTelemetry sounds better, thanks

@maksymmalyhin maksymmalyhin merged commit f2536cf into master Jul 29, 2019
2 checks passed
@maksymmalyhin maksymmalyhin deleted the mm/core-heartbeat branch Jul 29, 2019
@wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Jul 29, 2019

FYI, this added a dependency from FirebaseCore to AppKit on macOS (for the symbol _NSApplicationDidBecomeActiveNotification).

This causes Firestore on macOS to fail to link with this error:

Undefined symbols for architecture x86_64:
  "_NSApplicationDidBecomeActiveNotification", referenced from:
      -[FIRApp subscribeForAppDidBecomeActiveNotifications] in FirebaseCore(FIRApp.m.o)

I think the fix is to add AppKit as a macOS dependency to the FirebaseCore pod. Does that seem right?

@wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Jul 29, 2019

Assuming the AppKit dependency is legitimate, #3459 is the fix.

@firebase firebase locked and limited conversation to collaborators Oct 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants