Skip to content

Fix Double Logging of FCM #1834

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 2 commits into from
Aug 5, 2020
Merged

Fix Double Logging of FCM #1834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a1319e
Fix Double Logging of FCM
zwu52 Jul 30, 2020
0baa71b
Revise the package name
zwu52 Jul 31, 2020
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
25 changes: 1 addition & 24 deletions firebase-messaging/src/main/java/com/google/firebase/messaging/FirebaseMessagingService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,10 @@
import androidx.annotation.NonNull;
import androidx.annotation.VisibleForTesting;
import androidx.annotation.WorkerThread;
import com.google.android.datatransport.Encoding;
import com.google.android.datatransport.Transport;
import com.google.android.datatransport.TransportFactory;
import com.google.android.gms.tasks.Task;
import com.google.android.gms.tasks.Tasks;
import com.google.firebase.iid.MessengerIpcClient;
import com.google.firebase.iid.ServiceStarter;
import com.google.firebase.messaging.Constants.FirelogAnalytics;
import com.google.firebase.messaging.Constants.IntentActionKeys;
import com.google.firebase.messaging.Constants.IntentKeys;
import com.google.firebase.messaging.Constants.MessagePayloadKeys;
Expand Down Expand Up @@ -249,26 +245,7 @@ private void passMessageIntentToSdk(Intent intent) {
switch (messageType) {
case MessageTypes.MESSAGE:
if (MessagingAnalytics.shouldUploadScionMetrics(intent)) {
MessagingAnalytics.logNotificationReceived(intent, /* transport= */ null);
}

if (MessagingAnalytics.shouldUploadFirelogAnalytics(intent)) {
TransportFactory transportFactory = FirebaseMessaging.getTransportFactory();

if (transportFactory != null) {
Transport<String> transport =
transportFactory.getTransport(
FirelogAnalytics.FCM_LOG_SOURCE,
String.class,
Encoding.of("json"),
String::getBytes);

MessagingAnalytics.logNotificationReceived(intent, transport);
} else {
Log.e(
TAG,
"TransportFactory is null. Skip exporting message delivery metrics to Big Query");
}
MessagingAnalytics.logNotificationReceived(intent);
}

dispatchMessage(intent);
Expand Down
26 changes: 22 additions & 4 deletions firebase-messaging/src/main/java/com/google/firebase/messaging/MessagingAnalytics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;
import com.google.android.datatransport.Encoding;
import com.google.android.datatransport.Event;
import com.google.android.datatransport.Transport;
import com.google.android.datatransport.TransportFactory;
import com.google.firebase.FirebaseApp;
import com.google.firebase.analytics.connector.AnalyticsConnector;
import com.google.firebase.encoders.DataEncoder;
Expand Down Expand Up @@ -81,11 +83,27 @@ FirelogAnalyticsEventWrapper.class, new FirelogAnalyticsEventWrapperEncoder())
.build();

/** Log that a notification was received by the client app. */
public static void logNotificationReceived(Intent intent, @Nullable Transport<String> transport) {
logToScion(ScionAnalytics.EVENT_NOTIFICATION_RECEIVE, intent);
public static void logNotificationReceived(Intent intent) {

if (transport != null) {
logToFirelog(EventType.MESSAGE_DELIVERED, intent, transport);
if (MessagingAnalytics.shouldUploadScionMetrics(intent)) {
logToScion(ScionAnalytics.EVENT_NOTIFICATION_RECEIVE, intent);
}

if (MessagingAnalytics.shouldUploadFirelogAnalytics(intent)) {
TransportFactory transportFactory = FirebaseMessaging.getTransportFactory();

if (transportFactory != null) {
Transport<String> transport =
transportFactory.getTransport(
FirelogAnalytics.FCM_LOG_SOURCE,
String.class,
Encoding.of("json"),
String::getBytes);
logToFirelog(EventType.MESSAGE_DELIVERED, intent, transport);
} else {
Log.e(
TAG, "TransportFactory is null. Skip exporting message delivery metrics to Big Query");
}
}
}

Expand Down
58 changes: 39 additions & 19 deletions ...ase-messaging/src/test/java/com/google/firebase/messaging/MessagingAnalyticsRoboTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,26 +25,27 @@
import android.os.Bundle;
import androidx.test.core.app.ApplicationProvider;
import com.google.android.datatransport.TransportFactory;
import com.google.firebase.messaging.testing.AnalyticsValidator;
import com.google.firebase.messaging.testing.AnalyticsValidator.LoggedEvent;
import com.google.firebase.FirebaseApp;
import com.google.firebase.FirebaseOptions;
import com.google.firebase.analytics.connector.AnalyticsConnector;
import com.google.firebase.messaging.testing.FakeConnectorComponent;
import com.google.firebase.components.ComponentDiscoveryService;
import com.google.firebase.messaging.AnalyticsTestHelper.Analytics;
import com.google.firebase.messaging.Constants.AnalyticsKeys;
import com.google.firebase.messaging.Constants.FirelogAnalytics;
import com.google.firebase.messaging.Constants.MessageNotificationKeys;
import com.google.firebase.messaging.Constants.MessagePayloadKeys;
import com.google.firebase.messaging.Constants.ScionAnalytics;
import com.google.firebase.messaging.testing.AnalyticsValidator;
import com.google.firebase.messaging.testing.AnalyticsValidator.LoggedEvent;
import com.google.firebase.messaging.testing.FakeConnectorComponent;
import com.google.firebase.messaging.testing.MessagingTestHelper;
import java.util.List;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.robolectric.RobolectricTestRunner;


/** Messaging Analytics tests */
@RunWith(RobolectricTestRunner.class)
public class MessagingAnalyticsRoboTest {
Expand Down Expand Up @@ -117,7 +118,7 @@ public void testNoCrashIfAnalyticsIsMissingAtRuntime() throws Exception {
Intent intent = new Intent();
intent.putExtra(ANALYTICS_COMPOSER_ID, "composer_key");

MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
MessagingAnalytics.logNotificationReceived(intent);
MessagingAnalytics.logNotificationOpen(intent);
MessagingAnalytics.logNotificationDismiss(intent);
// No Exception is thrown = no crash, yeah
Expand Down Expand Up @@ -373,8 +374,9 @@ private Bundle editManifestApplicationMetadata() throws Exception {
public void testComposerUiPopulatesParamMessageId() {
Intent intent = new Intent();
intent.putExtra(ANALYTICS_COMPOSER_ID, "campaign_id");
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");

MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
MessagingAnalytics.logNotificationReceived(intent);

List<LoggedEvent> events = analyticsValidator.getLoggedEvents();
assertThat(events).hasSize(1);
Expand All @@ -393,8 +395,9 @@ public void testComposerUiPopulatesParamMessageId() {
public void testTopicsApiPopulatesParamTopic_straightFromHttpTopicApi() {
Intent intent = new Intent();
intent.putExtra(MessagePayloadKeys.FROM, "/topics/test_topic");
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");

MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
MessagingAnalytics.logNotificationReceived(intent);

List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
assertThat(events).hasSize(1);
Expand All @@ -417,8 +420,9 @@ public void testTopicsApiPopulatesParamTopic_fromComposerUiUsingTopic() {
Intent intent = new Intent();
intent.putExtra(ANALYTICS_COMPOSER_ID, "campaign_id");
intent.putExtra(MessagePayloadKeys.FROM, "/topics/test_topic");
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");

MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
MessagingAnalytics.logNotificationReceived(intent);

List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
assertThat(events).hasSize(1);
Expand All @@ -436,10 +440,12 @@ public void testTopicsApiPopulatesParamTopic_fromComposerUiUsingTopic() {
@Test
public void testTopicsApiPopulatesParamTopic_fromComposerUiWithFromNotATopic() {
Intent intent = new Intent();
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");

intent.putExtra(ANALYTICS_COMPOSER_ID, "campaign_id");
intent.putExtra(MessagePayloadKeys.FROM, "not_a_topic_name");

MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
MessagingAnalytics.logNotificationReceived(intent);

List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
assertThat(events).hasSize(1);
Expand All @@ -455,11 +461,13 @@ public void testTopicsApiPopulatesParamTopic_fromComposerUiWithFromNotATopic() {
@Test
public void analyticsMessageTimestamp() {
Intent intent = new Intent();
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");

intent.putExtra(ANALYTICS_COMPOSER_ID, "campaign_id");
intent.putExtra(ANALYTICS_MESSAGE_TIMESTAMP, "1234");

// Notification with a valid timestamp
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
MessagingAnalytics.logNotificationReceived(intent);

List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
assertThat(events).hasSize(1);
Expand All @@ -473,11 +481,13 @@ public void analyticsMessageTimestamp() {
@Test
public void analyticsMessageTimestamp_invalid() {
Intent intent = new Intent();
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");

intent.putExtra(ANALYTICS_COMPOSER_ID, "campaign_id");
// Notification with a corrupted timestamp
intent.putExtra(ANALYTICS_MESSAGE_TIMESTAMP, "1234_garbage");

MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
MessagingAnalytics.logNotificationReceived(intent);

List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
assertThat(events).hasSize(1);
Expand All @@ -491,9 +501,11 @@ public void analyticsMessageTimestamp_invalid() {
public void analyticsComposerLabel_missing() {
Intent intent = new Intent();
intent.putExtra(ANALYTICS_COMPOSER_ID, "campaign_id");
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");

// ANALYTICS_COMPOSER_LABEL not set

MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
MessagingAnalytics.logNotificationReceived(intent);

List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
assertThat(events).hasSize(1);
Expand All @@ -508,8 +520,9 @@ public void analyticsComposerLabel() {
Intent intent = new Intent();
intent.putExtra(ANALYTICS_COMPOSER_ID, "campaign_id");
intent.putExtra(ANALYTICS_COMPOSER_LABEL, "human composer label");
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");

MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
MessagingAnalytics.logNotificationReceived(intent);

List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
assertThat(events).hasSize(1);
Expand All @@ -523,7 +536,9 @@ public void analyticsComposerLabel() {
@Test
public void analyticsMessageLabel_missing() {
Intent intent = new Intent();
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");

MessagingAnalytics.logNotificationReceived(intent);

List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
assertThat(events).hasSize(1);
Expand All @@ -537,7 +552,9 @@ public void analyticsMessageLabel_missing() {
public void analyticsMessageLabel_present() {
Intent intent = new Intent();
intent.putExtra(ANALYTICS_MESSAGE_LABEL, "developer-provided-label");
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");

MessagingAnalytics.logNotificationReceived(intent);

List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
assertThat(events).hasSize(1);
Expand All @@ -551,8 +568,9 @@ public void analyticsMessageLabel_present() {
@Test
public void notificationLifecycle_eventReceived_dataMessage() {
Intent intent = new Intent();
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");

MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
MessagingAnalytics.logNotificationReceived(intent);

List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
assertThat(events).hasSize(1);
Expand All @@ -565,11 +583,13 @@ public void notificationLifecycle_eventReceived_dataMessage() {
@Test
public void notificationLifecycle_eventReceived_notification() {
Intent intent = new Intent();
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");

intent.putExtra(ANALYTICS_COMPOSER_ID, "campaign_id");
// Set as notification.
intent.putExtra("gcm.n.e", "1");

MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
MessagingAnalytics.logNotificationReceived(intent);

List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
assertThat(events).hasSize(1);
Expand Down Expand Up @@ -645,7 +665,7 @@ public void trackConversions_enabled_eventReceived() {
intent.putExtra(ANALYTICS_TRACK_CONVERSIONS, "1");

// Notification received: NO user-property and NO Event.FIREBASE_CAMPAIGN is logged
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
MessagingAnalytics.logNotificationReceived(intent);

assertThat(analyticsValidator.getLoggedEventNames())
.doesNotContain(ScionAnalytics.EVENT_FIREBASE_CAMPAIGN);
Expand Down Expand Up @@ -726,7 +746,7 @@ public void trackConversions_disabled_eventReceived() {
// Extra: ANALYTICS_TRACK_CONVERSIONS="1" NOT set

// Notification received: NO user-property and NO Event.FIREBASE_CAMPAIGN is logged
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
MessagingAnalytics.logNotificationReceived(intent);

assertThat(analyticsValidator.getLoggedEventNames())
.doesNotContain(ScionAnalytics.EVENT_FIREBASE_CAMPAIGN);
Expand Down Expand Up @@ -844,4 +864,4 @@ public void testGetMessageTypeForFirelog_dataMessage() {
assertThat(MessagingAnalytics.getMessageTypeForFirelog(intent))
.isEqualTo(FirelogAnalytics.MessageType.DISPLAY_NOTIFICATION);
}
}
}