Skip to content

Commit f833a38

Browse files
zwu52zwu52
authored
Fix Double Logging of FCM (firebase#1834)
* Fix Double Logging of FCM * Revise the package name
1 parent 924aaa2 commit f833a38

File tree

3 files changed

+62
-47
lines changed

3 files changed

+62
-47
lines changed

‎firebase-messaging/src/main/java/com/google/firebase/messaging/FirebaseMessagingService.java‎

Lines changed: 1 addition & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,10 @@
2424
import androidx.annotation.NonNull;
2525
import androidx.annotation.VisibleForTesting;
2626
import androidx.annotation.WorkerThread;
27-
import com.google.android.datatransport.Encoding;
28-
import com.google.android.datatransport.Transport;
29-
import com.google.android.datatransport.TransportFactory;
3027
import com.google.android.gms.tasks.Task;
3128
import com.google.android.gms.tasks.Tasks;
3229
import com.google.firebase.iid.MessengerIpcClient;
3330
import com.google.firebase.iid.ServiceStarter;
34-
import com.google.firebase.messaging.Constants.FirelogAnalytics;
3531
import com.google.firebase.messaging.Constants.IntentActionKeys;
3632
import com.google.firebase.messaging.Constants.IntentKeys;
3733
import com.google.firebase.messaging.Constants.MessagePayloadKeys;
@@ -249,26 +245,7 @@ private void passMessageIntentToSdk(Intent intent) {
249245
switch (messageType) {
250246
case MessageTypes.MESSAGE:
251247
if (MessagingAnalytics.shouldUploadScionMetrics(intent)) {
252-
MessagingAnalytics.logNotificationReceived(intent, /* transport= */ null);
253-
}
254-
255-
if (MessagingAnalytics.shouldUploadFirelogAnalytics(intent)) {
256-
TransportFactory transportFactory = FirebaseMessaging.getTransportFactory();
257-
258-
if (transportFactory != null) {
259-
Transport<String> transport =
260-
transportFactory.getTransport(
261-
FirelogAnalytics.FCM_LOG_SOURCE,
262-
String.class,
263-
Encoding.of("json"),
264-
String::getBytes);
265-
266-
MessagingAnalytics.logNotificationReceived(intent, transport);
267-
} else {
268-
Log.e(
269-
TAG,
270-
"TransportFactory is null. Skip exporting message delivery metrics to Big Query");
271-
}
248+
MessagingAnalytics.logNotificationReceived(intent);
272249
}
273250

274251
dispatchMessage(intent);

‎firebase-messaging/src/main/java/com/google/firebase/messaging/MessagingAnalytics.java‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,10 @@
2626
import androidx.annotation.NonNull;
2727
import androidx.annotation.Nullable;
2828
import androidx.annotation.VisibleForTesting;
29+
import com.google.android.datatransport.Encoding;
2930
import com.google.android.datatransport.Event;
3031
import com.google.android.datatransport.Transport;
32+
import com.google.android.datatransport.TransportFactory;
3133
import com.google.firebase.FirebaseApp;
3234
import com.google.firebase.analytics.connector.AnalyticsConnector;
3335
import com.google.firebase.encoders.DataEncoder;
@@ -81,11 +83,27 @@ FirelogAnalyticsEventWrapper.class, new FirelogAnalyticsEventWrapperEncoder())
8183
.build();
8284

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

87-
if (transport != null) {
88-
logToFirelog(EventType.MESSAGE_DELIVERED, intent, transport);
88+
if (MessagingAnalytics.shouldUploadScionMetrics(intent)) {
89+
logToScion(ScionAnalytics.EVENT_NOTIFICATION_RECEIVE, intent);
90+
}
91+
92+
if (MessagingAnalytics.shouldUploadFirelogAnalytics(intent)) {
93+
TransportFactory transportFactory = FirebaseMessaging.getTransportFactory();
94+
95+
if (transportFactory != null) {
96+
Transport<String> transport =
97+
transportFactory.getTransport(
98+
FirelogAnalytics.FCM_LOG_SOURCE,
99+
String.class,
100+
Encoding.of("json"),
101+
String::getBytes);
102+
logToFirelog(EventType.MESSAGE_DELIVERED, intent, transport);
103+
} else {
104+
Log.e(
105+
TAG, "TransportFactory is null. Skip exporting message delivery metrics to Big Query");
106+
}
89107
}
90108
}
91109

‎firebase-messaging/src/test/java/com/google/firebase/messaging/MessagingAnalyticsRoboTest.java‎

Lines changed: 39 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -25,26 +25,27 @@
2525
import android.os.Bundle;
2626
import androidx.test.core.app.ApplicationProvider;
2727
import com.google.android.datatransport.TransportFactory;
28+
import com.google.firebase.messaging.testing.AnalyticsValidator;
29+
import com.google.firebase.messaging.testing.AnalyticsValidator.LoggedEvent;
2830
import com.google.firebase.FirebaseApp;
2931
import com.google.firebase.FirebaseOptions;
3032
import com.google.firebase.analytics.connector.AnalyticsConnector;
33+
import com.google.firebase.messaging.testing.FakeConnectorComponent;
3134
import com.google.firebase.components.ComponentDiscoveryService;
3235
import com.google.firebase.messaging.AnalyticsTestHelper.Analytics;
3336
import com.google.firebase.messaging.Constants.AnalyticsKeys;
3437
import com.google.firebase.messaging.Constants.FirelogAnalytics;
3538
import com.google.firebase.messaging.Constants.MessageNotificationKeys;
3639
import com.google.firebase.messaging.Constants.MessagePayloadKeys;
3740
import com.google.firebase.messaging.Constants.ScionAnalytics;
38-
import com.google.firebase.messaging.testing.AnalyticsValidator;
39-
import com.google.firebase.messaging.testing.AnalyticsValidator.LoggedEvent;
40-
import com.google.firebase.messaging.testing.FakeConnectorComponent;
4141
import com.google.firebase.messaging.testing.MessagingTestHelper;
4242
import java.util.List;
4343
import org.junit.Before;
4444
import org.junit.Test;
4545
import org.junit.runner.RunWith;
4646
import org.robolectric.RobolectricTestRunner;
4747

48+
4849
/** Messaging Analytics tests */
4950
@RunWith(RobolectricTestRunner.class)
5051
public class MessagingAnalyticsRoboTest {
@@ -117,7 +118,7 @@ public void testNoCrashIfAnalyticsIsMissingAtRuntime() throws Exception {
117118
Intent intent = new Intent();
118119
intent.putExtra(ANALYTICS_COMPOSER_ID, "composer_key");
119120

120-
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
121+
MessagingAnalytics.logNotificationReceived(intent);
121122
MessagingAnalytics.logNotificationOpen(intent);
122123
MessagingAnalytics.logNotificationDismiss(intent);
123124
// No Exception is thrown = no crash, yeah
@@ -373,8 +374,9 @@ private Bundle editManifestApplicationMetadata() throws Exception {
373374
public void testComposerUiPopulatesParamMessageId() {
374375
Intent intent = new Intent();
375376
intent.putExtra(ANALYTICS_COMPOSER_ID, "campaign_id");
377+
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");
376378

377-
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
379+
MessagingAnalytics.logNotificationReceived(intent);
378380

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

397-
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
400+
MessagingAnalytics.logNotificationReceived(intent);
398401

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

421-
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
425+
MessagingAnalytics.logNotificationReceived(intent);
422426

423427
List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
424428
assertThat(events).hasSize(1);
@@ -436,10 +440,12 @@ public void testTopicsApiPopulatesParamTopic_fromComposerUiUsingTopic() {
436440
@Test
437441
public void testTopicsApiPopulatesParamTopic_fromComposerUiWithFromNotATopic() {
438442
Intent intent = new Intent();
443+
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");
444+
439445
intent.putExtra(ANALYTICS_COMPOSER_ID, "campaign_id");
440446
intent.putExtra(MessagePayloadKeys.FROM, "not_a_topic_name");
441447

442-
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
448+
MessagingAnalytics.logNotificationReceived(intent);
443449

444450
List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
445451
assertThat(events).hasSize(1);
@@ -455,11 +461,13 @@ public void testTopicsApiPopulatesParamTopic_fromComposerUiWithFromNotATopic() {
455461
@Test
456462
public void analyticsMessageTimestamp() {
457463
Intent intent = new Intent();
464+
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");
465+
458466
intent.putExtra(ANALYTICS_COMPOSER_ID, "campaign_id");
459467
intent.putExtra(ANALYTICS_MESSAGE_TIMESTAMP, "1234");
460468

461469
// Notification with a valid timestamp
462-
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
470+
MessagingAnalytics.logNotificationReceived(intent);
463471

464472
List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
465473
assertThat(events).hasSize(1);
@@ -473,11 +481,13 @@ public void analyticsMessageTimestamp() {
473481
@Test
474482
public void analyticsMessageTimestamp_invalid() {
475483
Intent intent = new Intent();
484+
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");
485+
476486
intent.putExtra(ANALYTICS_COMPOSER_ID, "campaign_id");
477487
// Notification with a corrupted timestamp
478488
intent.putExtra(ANALYTICS_MESSAGE_TIMESTAMP, "1234_garbage");
479489

480-
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
490+
MessagingAnalytics.logNotificationReceived(intent);
481491

482492
List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
483493
assertThat(events).hasSize(1);
@@ -491,9 +501,11 @@ public void analyticsMessageTimestamp_invalid() {
491501
public void analyticsComposerLabel_missing() {
492502
Intent intent = new Intent();
493503
intent.putExtra(ANALYTICS_COMPOSER_ID, "campaign_id");
504+
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");
505+
494506
// ANALYTICS_COMPOSER_LABEL not set
495507

496-
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
508+
MessagingAnalytics.logNotificationReceived(intent);
497509

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

512-
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
525+
MessagingAnalytics.logNotificationReceived(intent);
513526

514527
List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
515528
assertThat(events).hasSize(1);
@@ -523,7 +536,9 @@ public void analyticsComposerLabel() {
523536
@Test
524537
public void analyticsMessageLabel_missing() {
525538
Intent intent = new Intent();
526-
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
539+
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");
540+
541+
MessagingAnalytics.logNotificationReceived(intent);
527542

528543
List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
529544
assertThat(events).hasSize(1);
@@ -537,7 +552,9 @@ public void analyticsMessageLabel_missing() {
537552
public void analyticsMessageLabel_present() {
538553
Intent intent = new Intent();
539554
intent.putExtra(ANALYTICS_MESSAGE_LABEL, "developer-provided-label");
540-
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
555+
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");
556+
557+
MessagingAnalytics.logNotificationReceived(intent);
541558

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

555-
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
573+
MessagingAnalytics.logNotificationReceived(intent);
556574

557575
List<AnalyticsValidator.LoggedEvent> events = analyticsValidator.getLoggedEvents();
558576
assertThat(events).hasSize(1);
@@ -565,11 +583,13 @@ public void notificationLifecycle_eventReceived_dataMessage() {
565583
@Test
566584
public void notificationLifecycle_eventReceived_notification() {
567585
Intent intent = new Intent();
586+
intent.putExtra(Constants.AnalyticsKeys.ENABLED, "1");
587+
568588
intent.putExtra(ANALYTICS_COMPOSER_ID, "campaign_id");
569589
// Set as notification.
570590
intent.putExtra("gcm.n.e", "1");
571591

572-
MessagingAnalytics.logNotificationReceived(intent, /*transport= */ null);
592+
MessagingAnalytics.logNotificationReceived(intent);
573593

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

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

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

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

731751
assertThat(analyticsValidator.getLoggedEventNames())
732752
.doesNotContain(ScionAnalytics.EVENT_FIREBASE_CAMPAIGN);
@@ -844,4 +864,4 @@ public void testGetMessageTypeForFirelog_dataMessage() {
844864
assertThat(MessagingAnalytics.getMessageTypeForFirelog(intent))
845865
.isEqualTo(FirelogAnalytics.MessageType.DISPLAY_NOTIFICATION);
846866
}
847-
}
867+
}

0 commit comments

Comments
 (0)