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

Implement Firebase CoreDiagnostics #3129

Merged
merged 121 commits into from Jul 18, 2019

Conversation

@mikehaney24
Copy link
Collaborator

commented Jun 3, 2019

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.

mikehaney24 added 3 commits May 25, 2019
Implement CoreDiagnostics interop
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.

@googlebot googlebot added the cla: yes label Jun 3, 2019

Update Podfiles that dep on FirebaseCore
So that they also have a dep on FirebaseCoreDiagnosticsInterop
@paulb777
Copy link
Member

left a comment

Needs a pod deintegrate. Otherwise lgtm. Please get at least one of David or Ryan to approve the diagnostic specific functionality.

@mikehaney24

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 3, 2019

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.

Firestore/Example/Podfile Outdated Show resolved Hide resolved
mikehaney24 added 6 commits Jun 4, 2019
@mikehaney24

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 4, 2019

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

@wilhuff
Copy link
Member

left a comment

CMake changes LGTM

@mikehaney24

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 5, 2019

Note: the InAppMessaging OCMock compilation issue is non-transient, I'm able to reliably repro locally and are working on tracking it down.

@paulb777

This comment has been minimized.

Copy link
Member

commented on c08b8c8 Jun 5, 2019

Did this cause the failure?

This comment has been minimized.

Copy link
Collaborator Author

replied Jun 6, 2019

Doubtful, but I want to be consistent.

@davidair davidair requested a review from maksymmalyhin Jun 7, 2019

maksymmalyhin and others added 9 commits Jul 10, 2019
Fix constant name conflic between old and new diagnostics (#3329)
* Rename kFIRDiagnosticsHeartbeatDateFileName -> kFIRCoreDiagnosticsHeartbeatDateFileName to avoid conflicts with FIRDiagnostics.

* Travis: temporary add `mph-master` to run tests.
@paulb777
Copy link
Member

left a comment

Add comments around data collection describing the reasons for collecting the data or a link describing it.

FirebaseCoreDiagnostics.podspec Outdated Show resolved Hide resolved
scripts/pod_lib_lint.rb Show resolved Hide resolved

@mikehaney24 mikehaney24 changed the title Implement CoreDiagnostics interop Implement Firebase CoreDiagnostics Jul 16, 2019

@mikehaney24

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 16, 2019

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

@mikehaney24

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 17, 2019

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?

@paulb777

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

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.

@paulb777
Copy link
Member

left a comment

LGTM to merge on a green travis run

@mikehaney24

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 17, 2019

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?

@paulb777

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

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

@mikehaney24

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 18, 2019

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:

2019-07-17 16:35:27.245498-0700 Firestore_Example_macOS[22146:3039720] *** Assertion failure in gdt_cct_LogResponse GDTCCTDecodeLogResponse(NSData *__strong _Nonnull, NSError *__autoreleasing * _Nullable)(), /Users/haneym/coding/firebase-ios-sdk/GoogleDataTransportCCTSupport/GDTCCTLibrary/GDTCCTNanopbHelpers.m:168
2019-07-17 16:35:27.257992-0700 Firestore_Example_macOS[22146:3039720] [General] An uncaught exception was raised
2019-07-17 16:35:27.258022-0700 Firestore_Example_macOS[22146:3039720] [General] Error in nanopb decoding for bytes: invalid wire_type
2019-07-17 16:35:27.258090-0700 Firestore_Example_macOS[22146:3039720] [General] (
	0   CoreFoundation                      0x00007fff2cf3dcfd __exceptionPreprocess + 256
	1   libobjc.A.dylib                     0x00007fff575e7a17 objc_exception_throw + 48
	2   CoreFoundation                      0x00007fff2cf58a1a +[NSException raise:format:arguments:] + 98
	3   Foundation                          0x00007fff2f2461e5 -[NSAssertionHandler handleFailureInFunction:file:lineNumber:description:] + 166
	4   Firestore_Example_macOS             0x000000010d6d5909 GDTCCTDecodeLogResponse + 601
	5   Firestore_Example_macOS             0x000000010d6d815a __32-[GDTCCTUploader uploadPackage:]_block_invoke_2 + 490
	6   CFNetwork                           0x00007fff2bdd0225 __75-[__NSURLSessionLocal taskForClass:request:uploadFile:bodyData:completion:]_block_invoke + 19
	7   CFNetwork                           0x00007fff2bdcfba9 __49-[__NSCFLocalSessionTask _task_onqueue_didFinish]_block_invoke + 172
	8   Foundation                          0x00007fff2f12455c __NSBLOCKOPERATION_IS_CALLING_OUT_TO_A_BLOCK__ + 7
	9   Foundation                          0x00007fff2f124464 -[NSBlockOperation main] + 68
	10  Foundation                          0x00007fff2f0fa1dd -[__NSOperationInternal _start:] + 685
	11  Foundation                          0x00007fff2f124197 __NSOQSchedule_f + 227
	12  libdispatch.dylib                   0x00007fff58d675f8 _dispatch_call_block_and_release + 12
	13  libdispatch.dylib                   0x00007fff58d6863d _dispatch_client_callout + 8
	14  libdispatch.dylib                   0x00007fff58d6ade6 _dispatch_continuation_pop + 414
	15  libdispatch.dylib                   0x00007fff58d6a4a3 _dispatch_async_redirect_invoke + 703
	16  libdispatch.dylib                   0x00007fff58d763bc _dispatch_root_queue_drain + 324
	17  libdispatch.dylib                   0x00007fff58d76b46 _dispatch_worker_thread2 + 90
	18  libsystem_pthread.dylib             0x00007fff58fa86b3 _pthread_wqthread + 583
	19  libsystem_pthread.dylib             0x00007fff58fa83fd start_wqthread + 13
@paulb777

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

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.

@mikehaney24

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 18, 2019

Travis is green, here we go!

@mikehaney24 mikehaney24 merged commit 783ee5b into master Jul 18, 2019

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mikehaney24 mikehaney24 deleted the mph-master branch Jul 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.