-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Implement Firebase CoreDiagnostics #3129
Conversation
For use by FirebaseCore. Please pay special attention to the changes in FIRApp, I've replaced passing FIRApp in the notification with a data object that will eventually replace the same information that FIRDiagnostics extracted.
So that they also have a dep on FirebaseCoreDiagnosticsInterop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a pod deintegrate
. Otherwise lgtm. Please get at least one of David or Ryan to approve the diagnostic specific functionality.
Thanks for the pre-emptive review--I was trying to figure out why the Firestore checks are failing and was thinking I'd try to break up this giant PR into more digestible pieces once I get travis green. |
I am not planning on committing this just yet, I'm going to build at least a 2-stack of PRs that contain all the changes to move CoreDiagnostics here. There's also in-progress cl/248621323 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake changes LGTM
Note: the InAppMessaging OCMock compilation issue is non-transient, I'm able to reliably repro locally and are working on tracking it down. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple questions from me.
Firestore/core/src/firebase/firestore/remote/connectivity_monitor_noop.cc
Outdated
Show resolved
Hide resolved
* Rename kFIRDiagnosticsHeartbeatDateFileName -> kFIRCoreDiagnosticsHeartbeatDateFileName to avoid conflicts with FIRDiagnostics. * Travis: temporary add `mph-master` to run tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comments around data collection describing the reasons for collecting the data or a link describing it.
I think I'll need Ryan or David to provide that. @ryanwilson @davidair |
Is this resolved by @morganchen12 's documentation copy? |
Not yet. But we can consider this PR unblocked. We may want to add a link from the source to that copy in a later PR still. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM to merge on a green travis run
The same two Firestore tests are failing. Do you want to take a look to see if they should block this PR? |
It's ok for the asan tests to fail but not the macOS ones. It looked like a flake so I just restarted it. cc; @wilhuff |
It took me a while, but I was able to repro locally, and I figured out the cause of the failure:
|
Great! It's nice when CI catches real issues. |
Travis is green, here we go! |
For use by FirebaseCore.
Please pay special attention to the changes in FIRApp, I've replaced passing FIRApp in the notification with a data object that will eventually replace the same information that FIRDiagnostics extracted.