Integrate Firestore with Firebase platform logging #6507

var-const · 2020-09-21T02:54:35Z

The sample user agent string I'm getting in XCode is apple-platform/ios apple-sdk/17E8258 fire-fst/1.17.1 fire-ios/6.10.2 swift/true xcode/11E503a.

google-oss-bot · 2020-09-21T02:56:00Z

	1 Warning
⚠️	Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

wilhuff · 2020-09-21T17:04:55Z

Firestore/core/src/util/config.h

@@ -1,57 +0,0 @@
-/*


This file shouldn't be deleted (unless there's a replacement?).

Note that config.h was previously generated, but this was changed for compatibility with Swift Package Manager which does not allow for build-time configuration scripts.

Sorry, originally I deleted the wrong config.h file. Thanks for explaining!

wilhuff · 2020-09-21T17:06:28Z

Firestore/core/src/firebase/firestore/util/config.h

@@ -0,0 +1,33 @@
+/*


This was the old location of the file (before we shortened all the paths). This should be removed.

wilhuff

.

wilhuff · 2020-09-21T17:09:52Z

Firestore/core/src/remote/firebase_platform_logging_apple.h

+
+class FirebasePlatformLoggingApple : public FirebasePlatformLogging {
+ public:
+  explicit FirebasePlatformLoggingApple(FIRApp* app);


Objective-C types should have their nullability defined, either by wrapping in NS_ASSUME_NONNNULL_BEGIN/END or by annotating the pointers directly (FIRApp* _Nonnull app) .

wilhuff · 2020-09-21T17:10:22Z

Firestore/core/src/remote/firebase_platform_logging_apple.mm

+#import "FirebaseCore/Sources/Private/FIRAppInternal.h"
+#import "FirebaseCore/Sources/Private/FIRHeartbeatInfo.h"
+#import "FirebaseCore/Sources/Private/FIROptionsInternal.h"
+//#import "FirebaseCore/Sources/Public/FirebaseCore/FIRApp.h"


Please remove commented-out code.

Done, sorry.

wilhuff · 2020-09-21T17:21:41Z

Firestore/core/src/remote/firebase_platform_logging.h

+ * Wraps the platform-dependent functionality associated with Firebase platform
+ * logging.
+ */
+class FirebasePlatformLogging {


In the Android implementation, we have a notion of a GrpcMetadataProvider which does everything required to set the headers. Doing something similar here could make this a much narrower interface.

On the other hand, an interface like this one makes this information independent of the gRPC implementation which has some merit. Do you have any thoughts on how to make these platforms work similarly? This doesn't seem like something that has to be different on the different platforms.

When I started working on this, I indeed wanted to avoid a dependency on gRPC. However, this resulted in an interface that requires a lot of handholding from the client code (it must make sure to call all the IsFooAvailable functions, for example), so it seems that a dependency is a small price to pay for a more self-sufficient class.

Mimicking Android would be a bigger change -- the Android interface is pretty abstract, and it seems like it should provide all the extra metadata (including Cloud headers). If you feel strongly about this, I can do this in a follow-up.

The Android implementation is also imperfect. My original thought there was that separate concerns could supply separate metadata providers. That is, Firebase headers from one provider, Cloud headers from another, Auth perhaps from yet another. From that point of view, I think what you have here fits that goal quite well. Whether or not you see any benefit is another matter.

My original thought there was that separate concerns could supply separate metadata providers.

That's pretty much how I see it as well.

wilhuff · 2020-09-21T17:24:14Z

Firestore/core/test/unit/remote/grpc_stream_tester.h

@@ -248,6 +249,7 @@ class GrpcStreamTester {

  grpc::CompletionQueue grpc_queue_;
  FakeGrpcQueue fake_grpc_queue_;
+  std::unique_ptr<FirebasePlatformLogging> firebase_platform_logging_;


FirebasePlatformLogging could be forward declared in this header.

wilhuff · 2020-09-21T17:24:49Z

Firestore/core/src/remote/grpc_connection.h

@@ -25,6 +25,7 @@
 #include "Firestore/core/src/auth/token.h"
 #include "Firestore/core/src/core/database_info.h"
 #include "Firestore/core/src/remote/connectivity_monitor.h"
+#include "Firestore/core/src/remote/firebase_platform_logging.h"


Use a forward declaration? Here and in all other headers.

Done (in all headers, I think).

Firestore/core/src/api/firestore.h

wilhuff · 2020-09-21T17:33:41Z

Firestore/core/src/remote/firebase_platform_logging.h

+ * Wraps the platform-dependent functionality associated with Firebase platform
+ * logging.
+ */
+class FirebasePlatformLogging {


Consider renaming this to a regular noun rather than a gerund (i.e. "logger" vs "logging").

My intention was to go with the exact name used for the feature; getting this approach to its logical conclusion would result in the awkward FirebasePlatformLoggingLogger. I'm not against using FirebasePlatformLogger if you think not following the feature name 100% isn't an issue.

In a forest, the person who cuts the trees is a "logger", while the larger practice is "logging". They're already grammatically related without resorting to calling the lumberjack a "tree logging logger".

My general skepticism with the "firebase platform logging" name comes from the fact that the term names the larger effort including something client-side that sends data, server-side collection, logs that are written, and the warehousing scripts to bucket the data. Since this is just a part of that larger effort, it seems misleading to name this smaller part with the name of whole thing.

To be fully descriptive you might name this thing "firebase platform logging metadata provider", since this doesn't really log data so much as attach headers to a request (aka provide request metadata). I think that's probably more of a mouthful than is warranted, given this thing's narrow scope.

If you agree with my idea above about (eventually) partitioning the header-providing bits along the lines of Firebase/Cloud/Auth/etc, possibly it's just sufficient to call this a FirebaseMetadataProvider.

Thoughts?

I treat the feature name as an opaque identifier, so I would see it as similar to "LCD display/PIN number/etc.". However, I like the name FirebaseMetadataProvider, so I might as well just go with that.

I fully agree with partitioning metadata providers into Cloud/Firebase/etc.

wilhuff

Structurally LGTM, though with several additional bits of optional feedback.

wilhuff · 2020-09-22T23:01:01Z

Firestore/core/src/remote/firebase_platform_logging.h

+ * Wraps the platform-dependent functionality associated with Firebase platform
+ * logging.
+ */
+class FirebasePlatformLogging {


The Android implementation is also imperfect. My original thought there was that separate concerns could supply separate metadata providers. That is, Firebase headers from one provider, Cloud headers from another, Auth perhaps from yet another. From that point of view, I think what you have here fits that goal quite well. Whether or not you see any benefit is another matter.

wilhuff · 2020-09-22T23:08:27Z

Firestore/core/src/remote/firebase_platform_logging.h

+ * Wraps the platform-dependent functionality associated with Firebase platform
+ * logging.
+ */
+class FirebasePlatformLogging {


In a forest, the person who cuts the trees is a "logger", while the larger practice is "logging". They're already grammatically related without resorting to calling the lumberjack a "tree logging logger".

My general skepticism with the "firebase platform logging" name comes from the fact that the term names the larger effort including something client-side that sends data, server-side collection, logs that are written, and the warehousing scripts to bucket the data. Since this is just a part of that larger effort, it seems misleading to name this smaller part with the name of whole thing.

To be fully descriptive you might name this thing "firebase platform logging metadata provider", since this doesn't really log data so much as attach headers to a request (aka provide request metadata). I think that's probably more of a mouthful than is warranted, given this thing's narrow scope.

If you agree with my idea above about (eventually) partitioning the header-providing bits along the lines of Firebase/Cloud/Auth/etc, possibly it's just sufficient to call this a FirebaseMetadataProvider.

Thoughts?

wilhuff · 2020-09-22T23:12:20Z

Firestore/core/src/remote/firebase_platform_logging_apple.mm

+  }
+}
+
+std::string FirebasePlatformLoggingApple::GetUserAgent() const {


Completely optional: with the exception of GetGmpAppId, none of these access any state in this instance. They could just be free functions in the anonymous namespace. Even GetGmpAppId could be phrased that way (by taking the FIRApp* as a parameter).

Alternatively, you could make these two static rather than const members.

Good call, done.

wilhuff · 2020-09-22T23:14:32Z

Firestore/core/src/remote/grpc_connection.h

@@ -109,6 +112,8 @@ class GrpcConnection {

  void RegisterConnectivityMonitor();

+  void AddFirebasePlatformLoggingHeader(grpc::ClientContext& context) const;


This declaration does not seem to have a definition (but perhaps I missed it)? It also seems not to have any uses.

Fixed, sorry.

wilhuff · 2020-09-22T23:31:25Z

Firestore/core/src/remote/firebase_platform_logging_noop.h

+namespace firestore {
+namespace remote {
+
+class FirebasePlatformLoggingNoOp : public FirebasePlatformLogging {


Optional: if the base class were to have a default no-op implementation, this class would be unnecessary.

At that point, anyone that wanted an implementation could also just absl::make_unique<FirebasePlatformLogging>() for themselves, also removing the need for CreateNoOpFirebasePlatformLogging.

Note that I have no strong preference about whether or not there's an explicit no-op type. I can see the case for it but this could also be simpler without it.

Secondarily, I find the disagreement in word ordering between FirebasePlatformLoggingNoOp and CreateNoOpFirebasePlatformLogging somewhat jarring. I suggest picking just one. Customarily the ordering for refinements of a base type seems to be refinement-base (as in the Create function) so I favor that ordering, but also don't feel strongly. See the differences searching google3 for \w+ThreadPool\b lang:c++ vs \bThreadPool\w+ lang:c++: the former are refinements of thread pool while the latter are mostly either related types (like ThreadPoolOptions) or classes where ThreadPool is the refinement of some other base type (like ThreadPoolTaskSystem).

Made the word order consistent.

I guess I have a strong preference towards the base-refinement ordering because it creates natural grouping, though there is a good argument to be made regarding precedent. I'd certainly prefer not to tackle it in this PR.

I'm slightly in favor of having an explicit no-op class because it's not supposed to be used normally. I can move it under test/ in a follow-up if you prefer.

wilhuff

LGTM

var-const · 2020-09-23T22:43:22Z

@VinayGuthal @vkryachko @maksymmalyhin Vinay, Vlad, Maksym -- can you please take a look to make sure the integration is correct? The most relevant parts are in the firebase_metadata_provider* files and in grpc_connection.cc (where the headers actually get sent).

maksymmalyhin

LGTM

maksymmalyhin · 2020-09-24T13:47:18Z

Firestore/core/src/remote/firebase_metadata_provider_apple.mm

+}
+
+std::string GetHeartbeat() {
+  return std::to_string([FIRHeartbeatInfo heartbeatCodeForTag:@"fire-fst"]);


FYI, currently [FIRHeartbeatInfo heartbeatCodeForTag:] method performs file operations to read and update data in the heartbeat storage. Ideally this call should not happen on the main thread. Also it may be worth to check if the current implementation provides performance good enough for Firestore use case.

Please let me know if you have any questions or concerns.

Firestore runs all networking and and I/O operations on a dedicated queue. This can only ever be called from that queue so this won't have a chance to run on the main thread.

vkryachko · 2020-09-24T13:49:31Z

Firestore/core/src/core/firestore_client.cc

-  std::shared_ptr<FirestoreClient> shared_client(
-      new FirestoreClient(database_info, std::move(credentials_provider),
-                          std::move(user_executor), std::move(worker_queue)));
+  std::shared_ptr<FirestoreClient> shared_client(new FirestoreClient(


curious: any reason not to use make_shared() here?

make_shared requires a public constructor but we're forcing callers to use FirestoreClient::Create and making the constructor private.

makes sense, thanks for explaining

vkryachko · 2020-09-24T13:54:07Z

Firestore/core/src/remote/firebase_metadata_provider_apple.mm

+  context.AddMetadata(kXFirebaseClientHeader, GetUserAgent());
+  context.AddMetadata(kXFirebaseClientLogTypeHeader, GetHeartbeat());


I don't think the headers are sent unconditionally on android:
https://github.com/firebase/firebase-android-sdk/blob/6e286da45f952142d91482219f0ad1e019c2b06d/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/FirebaseClientGrpcMetadataProvider.java#L55

cc @VinayGuthal to confirm

Great point, thanks. Before I do the change, one thing I need to clarify is whether the GMP App ID should or shouldn't be sent if heartbeat is 0. @VinayGuthal

I think sending the user agent, heartbeat type and GMP App ID may be omitted when the heartbeat type is FIRHeartbeatInfoCodeNone if the headers are meant to be used for the Platform logging pipeline only as it won't affect instance count estimation. Though I see a potential value in sending the user agent anyway as it may be used for analysis potential SDK issues.

hmm the concern at the time was that we might be causing a latency everytime firestore is used and hence the headers were not sent unconditionally. As @maksymmalyhin mentioned it won't affect instance count estimation.

Thanks for the explanations. Changed to not send anything if the heartbeat is 0 (how often does it happen, by the way?), PTAL.

the heartbeat is 0 all the time except for the first time in the day.

vkryachko · 2020-09-24T13:55:44Z

Firestore/core/src/remote/firebase_metadata_provider_apple.mm

+  if (!gmp_app_id.empty()) {
+    context.AddMetadata(kXFirebaseGmpIdHeader, gmp_app_id);
+  }


android does not send the gmp app id either. @VinayGuthal, curious why that is the case

@VinayGuthal Do you know why Android doesn't send the GMP App ID? This is something we're currently planning to add.

hmm looks like we do collect for firebase-iid. To estimate the number of devices using firebase we wouldn't need the GMP_APP_ID however. However the platform log proto has GMP_APP_ID as a optional string which can be useful for other metrics. I can change the android implementation to include the GMP_APP_ID as well.

maksymmalyhin · 2020-09-24T20:55:06Z

Firestore/core/src/remote/firebase_metadata_provider_apple.mm

@@ -36,8 +36,8 @@
  return MakeString([FIRApp firebaseUserAgent]);
 }

-std::string GetHeartbeat() {
-  return std::to_string([FIRHeartbeatInfo heartbeatCodeForTag:@"fire-fst"]);
+int GetHeartbeat() {


I wonder why casting it to int instead of using the FIRHeartbeatInfoCode enum? I think we are losing readability in this case.

That's a good point, done.

maksymmalyhin · 2020-09-24T21:04:44Z

Firestore/core/src/remote/firebase_metadata_provider_apple.mm

@@ -52,8 +52,13 @@

 void FirebaseMetadataProviderApple::UpdateMetadata(
    grpc::ClientContext& context) {
+  int heartbeat = GetHeartbeat();
+  if (heartbeat == 0) {
+    return;


optional: the method name looks generic enough for other thing unrelated to platform logs to be added there. This early exist may be very easy to miss, so the additional metadata may be unintentionally dropped. I would either wrap platform logs metadata into if(heartbeat != FIRHeartbeatInfoCodeNone) or introduce a separate method with a name like addPlatformLoggingMetadata() with the early exist which will be called from UpdateMetadata(). WDYT?

This class is intended specifically for the Firebase platform logging-related functionality. We may abstract away other things (e.g. Cloud headers) in a similar class, but it would be a separate one.

Thank you for clarification. IMO, the class and method names look a bit too generic in this case, but I don't feel too strong about it.

maksymmalyhin

LGTM

The sample user agent string I'm getting in XCode is `apple-platform/ios apple-sdk/17E8258 fire-fst/1.17.1 fire-ios/6.10.2 swift/true xcode/11E503a`.

var-const added 6 commits September 17, 2020 19:34

Initial commit

e74a626

GMP App ID

a324574

Refactoring

25a5be0

Accidental

b0db7a4

Remove temporary

956043a

Add comments

4508d1a

var-const requested a review from wilhuff September 21, 2020 02:54

googlebot added the cla: yes label Sep 21, 2020

var-const assigned wilhuff Sep 21, 2020

google-oss-bot added the api: firestore label Sep 21, 2020

wilhuff reviewed Sep 21, 2020

View reviewed changes

wilhuff assigned var-const and unassigned wilhuff Sep 22, 2020

var-const added 3 commits September 22, 2020 17:31

Feedback

75954ee

Feedback 2

a759d20

style.sh

a675dd5

var-const assigned wilhuff and unassigned var-const Sep 22, 2020

Forgotten file

13ae527

wilhuff reviewed Sep 22, 2020

View reviewed changes

wilhuff assigned var-const and unassigned wilhuff Sep 22, 2020

Feedback

0b1d6b5

wilhuff approved these changes Sep 23, 2020

View reviewed changes

Remove flag checking

58b2e26

var-const requested review from VinayGuthal and vkryachko September 23, 2020 22:41

var-const requested a review from maksymmalyhin September 23, 2020 22:41

var-const assigned VinayGuthal, vkryachko and maksymmalyhin and unassigned var-const Sep 23, 2020

maksymmalyhin approved these changes Sep 24, 2020

View reviewed changes

vkryachko reviewed Sep 24, 2020

View reviewed changes

Feedback

f9c51d0

maksymmalyhin approved these changes Sep 24, 2020

View reviewed changes

Feedback

0c966e7

maksymmalyhin approved these changes Sep 24, 2020

View reviewed changes

VinayGuthal approved these changes Sep 24, 2020

View reviewed changes

var-const added 2 commits September 24, 2020 20:06

Fix warning

ef0d609

Fix CMake build

9638842

var-const merged commit 2311c36 into master Sep 25, 2020

christibbs pushed a commit that referenced this pull request Sep 28, 2020

Integrate Firestore with Firebase platform logging (#6507)

c43e818

The sample user agent string I'm getting in XCode is `apple-platform/ios apple-sdk/17E8258 fire-fst/1.17.1 fire-ios/6.10.2 swift/true xcode/11E503a`.

firebase locked and limited conversation to collaborators Oct 26, 2020

paulb777 deleted the varconst/register-library branch January 10, 2021 00:46

		@@ -109,6 +112,8 @@ class GrpcConnection {

		void RegisterConnectivityMonitor();

		void AddFirebasePlatformLoggingHeader(grpc::ClientContext& context) const;

		context.AddMetadata(kXFirebaseClientHeader, GetUserAgent());
		context.AddMetadata(kXFirebaseClientLogTypeHeader, GetHeartbeat());

Integrate Firestore with Firebase platform logging #6507

Integrate Firestore with Firebase platform logging #6507

Uh oh!

Conversation

var-const commented Sep 21, 2020

Uh oh!

google-oss-bot commented Sep 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

var-const commented Sep 23, 2020

Uh oh!

maksymmalyhin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment