Skip to content

Heartbeat logging passthrough for FIS for mobile platforms. #853

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

Merged
merged 4 commits into from
Mar 4, 2022
Merged

Heartbeat logging passthrough for FIS for mobile platforms. #853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c884ada
Heartbeat logging passthrough for FIS for mobile platforms.
AlmostMatt Feb 17, 2022
0c9568a
Remove unnecessary log statement
AlmostMatt Feb 17, 2022
79488c6
added missing semicolon
AlmostMatt Feb 18, 2022
02cd8a7
Call CheckAndClearJniExceptions in LogHeartbeat
AlmostMatt Mar 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions installations/src/android/installations_android.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "installations/src/android/installations_android.h"

#include "app/src/util.h"
#include "app/src/util_android.h"
#include "installations/src/common.h"

namespace firebase {
Expand Down Expand Up @@ -186,6 +187,19 @@ void InstallationsInternal::Cleanup() {
}
}

void InstallationsInternal::LogHeartbeat(const firebase::App& app) {
// Calling the native getter is sufficient to cause a Heartbeat to be logged.
JNIEnv* env = app.GetJNIEnv();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious if there will be some warning and exception thrown out while running on android. If so, probably need to follow this pattern to make sure the jni exception is cleared.

firebase-cpp-sdk/remote_config/src/android/remote_config_android.cc

Line 590 in dcb0b3b

Future<bool> RemoteConfigInternal::Activate() {

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. We probably should do the same for Auth as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a call to CheckAndClearJniExceptions.
I removed the FIREBASE_ASSERT since I had just copied it literally from the above InstallationsInternal, but I dont think we need such an assertion in the LogHeartbeat method since the installations instance is not actually used by LogHeartbeat.

I also checked the Auth change and confirmed that I do already call CheckAndClearJniExceptions in the Auth LogHeartbeat method.
https://github.com/firebase/firebase-cpp-sdk/pull/846/files

I find it a bit odd that there were not any existing calls to CheckAndClearJniExceptions in installations_android.cc, but maybe it is just because it was written longer ago.

jobject platform_app = app.GetPlatformApp();
jclass installations_class = installations::GetClass();
jobject installations_instance_local = env->CallStaticObjectMethod(
installations_class,
installations::GetMethodId(installations::kGetInstance), platform_app);
firebase::util::CheckAndClearJniExceptions(env);
env->DeleteLocalRef(installations_instance_local);
env->DeleteLocalRef(platform_app);
}

Future<std::string> InstallationsInternal::GetId() {
const auto handle =
future_impl_.SafeAlloc<std::string>(kInstallationsFnGetId);
Expand Down
4 changes: 4 additions & 0 deletions installations/src/android/installations_android.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ class InstallationsInternal {
explicit InstallationsInternal(const firebase::App& app);
~InstallationsInternal();

// Platform-specific method that causes a heartbeat to be logged.
// See go/firebase-platform-logging-design for more information.
static void LogHeartbeat(const firebase::App& app);

Future<std::string> GetId();
Future<std::string> GetIdLastResult();

Expand Down
3 changes: 3 additions & 0 deletions installations/src/installations.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ Installations* Installations::GetInstance(App* app) {
// Return the Installations if it already exists.
Installations* existing_installations = FindInstallations(app);
if (existing_installations) {
// Log heartbeat data when instance getters are called.
// See go/firebase-platform-logging-design for more information.
InstallationsInternal::LogHeartbeat(*app);
return existing_installations;
}

Expand Down
4 changes: 4 additions & 0 deletions installations/src/ios/installations_ios.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ class InstallationsInternal {
explicit InstallationsInternal(const firebase::App& app);
~InstallationsInternal();

// Platform-specific method that causes a heartbeat to be logged.
// See go/firebase-platform-logging-design for more information.
static void LogHeartbeat(const firebase::App& app);

Future<std::string> GetId();
Future<std::string> GetIdLastResult();

Expand Down
6 changes: 6 additions & 0 deletions installations/src/ios/installations_ios.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@
// Destructor is necessary for ARC garbage collection.
}

void InstallationsInternal::LogHeartbeat(const firebase::App& app) {
// Calling the native getter is sufficient to cause a Heartbeat to be logged.
FIRApp *platform_app = app.GetPlatformApp();
[FIRInstallations installationsWithApp:platform_app];
}

bool InstallationsInternal::Initialized() const{
return true;
}
Expand Down
2 changes: 2 additions & 0 deletions installations/src/stub/installations_stub.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ InstallationsInternal::InstallationsInternal(const firebase::App& app)

InstallationsInternal::~InstallationsInternal() {}

void InstallationsInternal::LogHeartbeat(const firebase::App& app) {}

bool InstallationsInternal::Initialized() const { return true; }

void InstallationsInternal::Cleanup() {}
Expand Down
4 changes: 4 additions & 0 deletions installations/src/stub/installations_stub.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ class InstallationsInternal {
explicit InstallationsInternal(const firebase::App& app);
~InstallationsInternal();

// Platform-specific method that causes a heartbeat to be logged.
// See go/firebase-platform-logging-design for more information.
static void LogHeartbeat(const firebase::App& app);

Future<std::string> GetId();
Future<std::string> GetIdLastResult();

Expand Down