Skip to content

Standardize support for Firebase products that integrate with Remote Config. #7094

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 7 commits into from
Jan 20, 2021
Merged

Standardize support for Firebase products that integrate with Remote Config. #7094

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
91a5f0b
More logging for personalization.
vic-flair Dec 3, 2020
aa69e72
Fix null check in RCNPersonalization.m.
vic-flair Dec 14, 2020
820ad6b
Merge branch 'master' into vlum.p13n-2
vic-flair Jan 12, 2021
e5b90f7
Add CHANGELOG.
vic-flair Jan 12, 2021
e929711
Check cache for choice ID and add more asserts in tests.
vic-flair Jan 15, 2021
41439fc
Rename constants.
vic-flair Jan 16, 2021
89e4aed
Editing some comments.
vic-flair Jan 20, 2021
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
1 change: 1 addition & 0 deletions FirebaseRemoteConfig/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# v7.5.0
- [fixed] Fixed bug that was incorrectly flagging ABT experiment payloads as invalid. (#7184)
- [changed] Standardize support for Firebase products that integrate with Remote Config. (#7094)

# v7.1.0
- [changed] Add support for other Firebase products to integrate with Remote Config. (#6692)
Expand Down
17 changes: 14 additions & 3 deletions FirebaseRemoteConfig/Sources/RCNPersonalization.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,27 @@
NS_ASSUME_NONNULL_BEGIN

static NSString *const kAnalyticsOriginPersonalization = @"fp";
static NSString *const kAnalyticsPullEvent = @"_fpc";
static NSString *const kArmKey = @"_fpid";
static NSString *const kArmValue = @"_fpct";

static NSString *const kAnalyticsPullEvent = @"personalization_assignment";
Copy link

@erikeldridge erikeldridge Jan 16, 2021

Choose a reason for hiding this comment

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

Minor: as I commented on the Android change, it's unclear to me what "pull" means here and below.

Also, given this whole file concerns logging to Analytics, including "Analytics" in some field names, but not others is confusing. I'd vote to just omit it.

The main distinctions from my perspective are internal vs external, and "event" vs "param" (to use Analytics' "feature type" nomenclature from the event proposal).

To summarize, I'd vote to use "internal" and "external" prefixes to differentiate internal from external events, and "event" and "param" suffixes to differentiate event names from param names, eg kInternalEvent, kInternalChoiceIdParam, etc

Thinking (no action req'd): all these field names are consistent across Android and iOS 👍

static NSString *const kArmKey = @"arm_key";
static NSString *const kArmValue = @"arm_value";
static NSString *const kPersonalizationId = @"personalizationId";
static NSString *const kPersonalizationIdKey = @"personalization_id";
static NSString *const kArmIndex = @"armIndex";
static NSString *const kArmIndexKey = @"arm_index";
static NSString *const kGroup = @"group";

static NSString *const kAnalyticsPullEventInternal = @"_fpc";
static NSString *const kChoiceId = @"choiceId";
static NSString *const kChoiceIdKey = @"_fpid";

@interface RCNPersonalization : NSObject

/// Analytics connector
@property(nonatomic, strong) id<FIRAnalyticsInterop> _Nullable analytics;

@property(atomic, strong) NSMutableDictionary *armsCache;

- (instancetype)init NS_UNAVAILABLE;

/// Designated initializer.
Expand Down
26 changes: 23 additions & 3 deletions FirebaseRemoteConfig/Sources/RCNPersonalization.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ - (instancetype)initWithAnalytics:(id<FIRAnalyticsInterop> _Nullable)analytics {
self = [super init];
if (self) {
self->_analytics = analytics;
self->_armsCache = [[NSMutableDictionary alloc] init];
}
return self;
}
Expand All @@ -37,16 +38,35 @@ - (void)logArmActive:(NSString *)key config:(NSDictionary *)config {
}

NSDictionary *metadata = ids[key];
if (!metadata || metadata[kPersonalizationId] == nil) {
if (!metadata) {
return;
}

NSString *personalizationId = metadata[kPersonalizationId];
if (personalizationId == nil) {
return;
}

// This gets dispatched to a serial queue, so this is OK. But even if not, it'll just possibly
Copy link
Contributor

@danasilver danasilver Jan 19, 2021

Choose a reason for hiding this comment

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

It's a little unclear what the "this" refers to. Can you clarify? Does it mean that loggedChoiceIds may not have been updated yet and a duplicate entry may be logged? What makes it serial?

Copy link
Contributor Author

@vic-flair vic-flair Jan 20, 2021

Choose a reason for hiding this comment

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

"This" refers to this function. It has to do with how iOS handles async stuff. A serial queue means that functions are executed serially, which isn't always the case. Do you have any suggestions on how to make it clearer?

Copy link
Contributor

@danasilver danasilver Jan 20, 2021

Choose a reason for hiding this comment

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

What do you think of "Listeners like logArmActive are dispatched to a serial queue, so loggedChoiceIds should contain any previously logged RC Parameter/Choice ID pairs."

// log more.
if (self->_armsCache[key] == personalizationId) {
Copy link
Contributor

@karenyz karenyz Dec 10, 2020

Choose a reason for hiding this comment

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

does this mean we only want to log each arm once?

Copy link
Contributor Author

@vic-flair vic-flair Dec 14, 2020

Choose a reason for hiding this comment

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

We want to log at least once. We can do de-duping on the back-end.

return;
}
self->_armsCache[key] = personalizationId;

[self->_analytics logEventWithOrigin:kAnalyticsOriginPersonalization
name:kAnalyticsPullEvent
parameters:@{
kArmKey : metadata[kPersonalizationId],
kArmValue : values[key].stringValue
kArmKey : key,
kArmValue : values[key].stringValue,
kPersonalizationIdKey : personalizationId,
kArmIndexKey : metadata[kArmIndex],
kGroup : metadata[kGroup]
Copy link

@erikeldridge erikeldridge Jan 16, 2021

Choose a reason for hiding this comment

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

Thinking: the design lists five things we want to log:

  1. Developer friendly identifier of the personalization (either the rc param, or an id customers can map back to rc param), satisfied by the arm_key field
  2. The choice that was made (a string value), satisfied by the arm_value field
  3. The group the user was in (“p13n” or “baseline”), satisfied by the group field
  4. Personalization ID, satisfied by the personalization_id field
  5. Arm index, satisfied by the arm_index field

Thinking: the external event name is arbitrary, we just need to document what it is. personalization_assignment seems self-descriptive to me 👍

Minor: renaming arm_key to something like rc_parameter would be more self-descriptive given it communicates an RC parameter. Likewise, renaming the key arg passed to logArmActive to rcParameter would clarify its purpose.

}];

[self->_analytics logEventWithOrigin:kAnalyticsOriginPersonalization
name:kAnalyticsPullEventInternal
parameters:@{kChoiceIdKey : metadata[kChoiceId]}];
}

@end
67 changes: 42 additions & 25 deletions FirebaseRemoteConfig/Tests/Unit/RCNPersonalizationTest.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,22 @@ - (void)setUp {
initWithData:[@"value3" dataUsingEncoding:NSUTF8StringEncoding]
source:FIRRemoteConfigSourceRemote]
},
RCNFetchResponseKeyPersonalizationMetadata :
@{@"key1" : @{kPersonalizationId : @"id1"}, @"key2" : @{kPersonalizationId : @"id2"}}
RCNFetchResponseKeyPersonalizationMetadata : @{
@"key1" : @{
kPersonalizationId : @"p13n1",
kArmIndex : @0,
kChoiceId : @"id1",
kGroup : @"BASELINE"
},
@"key2" :
@{kPersonalizationId : @"p13n2", kArmIndex : @1, kChoiceId : @"id2", kGroup : @"P13N"}
}
};

_fakeLogs = [[NSMutableArray alloc] init];
_analyticsMock = OCMProtocolMock(@protocol(FIRAnalyticsInterop));
OCMStub([_analyticsMock logEventWithOrigin:kAnalyticsOriginPersonalization
name:kAnalyticsPullEvent
name:[OCMArg isKindOfClass:[NSString class]]
parameters:[OCMArg isKindOfClass:[NSDictionary class]]])
.andDo(^(NSInvocation *invocation) {
__unsafe_unretained NSDictionary *bundle;
Expand Down Expand Up @@ -108,7 +116,7 @@ - (void)testNonPersonalizationKey {

OCMVerify(never(),
[_analyticsMock logEventWithOrigin:kAnalyticsOriginPersonalization
name:kAnalyticsPullEvent
name:[OCMArg isKindOfClass:[NSString class]]
parameters:[OCMArg isKindOfClass:[NSDictionary class]]]);
XCTAssertEqual([_fakeLogs count], 0);
}
Expand All @@ -118,14 +126,14 @@ - (void)testSinglePersonalizationKey {

[_personalization logArmActive:@"key1" config:_configContainer];

OCMVerify(times(1),
OCMVerify(times(2),
[_analyticsMock logEventWithOrigin:kAnalyticsOriginPersonalization
name:kAnalyticsPullEvent
name:[OCMArg isKindOfClass:[NSString class]]
parameters:[OCMArg isKindOfClass:[NSDictionary class]]]);
XCTAssertEqual([_fakeLogs count], 1);
XCTAssertEqual([_fakeLogs count], 2);

NSDictionary *params = @{kArmKey : @"id1", kArmValue : @"value1"};
XCTAssertEqualObjects(_fakeLogs[0], params);
NSDictionary *params = @{kChoiceIdKey : @"id1"};
XCTAssertEqualObjects(_fakeLogs[1], params);
}

- (void)testMultiplePersonalizationKeys {
Expand All @@ -134,35 +142,35 @@ - (void)testMultiplePersonalizationKeys {
[_personalization logArmActive:@"key1" config:_configContainer];
[_personalization logArmActive:@"key2" config:_configContainer];

OCMVerify(times(2),
OCMVerify(times(4),
[_analyticsMock logEventWithOrigin:kAnalyticsOriginPersonalization
name:kAnalyticsPullEvent
name:[OCMArg isKindOfClass:[NSString class]]
parameters:[OCMArg isKindOfClass:[NSDictionary class]]]);
XCTAssertEqual([_fakeLogs count], 2);
XCTAssertEqual([_fakeLogs count], 4);

NSDictionary *params1 = @{kArmKey : @"id1", kArmValue : @"value1"};
XCTAssertEqualObjects(_fakeLogs[0], params1);
NSDictionary *params1 = @{kChoiceIdKey : @"id1"};
XCTAssertEqualObjects(_fakeLogs[1], params1);

NSDictionary *params2 = @{kArmKey : @"id2", kArmValue : @"value2"};
XCTAssertEqualObjects(_fakeLogs[1], params2);
NSDictionary *params2 = @{kChoiceIdKey : @"id2"};
XCTAssertEqualObjects(_fakeLogs[3], params2);
}

- (void)testRemoteConfigIntegration {
[_fakeLogs removeAllObjects];

FIRRemoteConfigFetchAndActivateCompletion fetchAndActivateCompletion =
^void(FIRRemoteConfigFetchAndActivateStatus status, NSError *error) {
OCMVerify(times(2), [self->_analyticsMock
OCMVerify(times(4), [self->_analyticsMock
logEventWithOrigin:kAnalyticsOriginPersonalization
name:kAnalyticsPullEvent
name:[OCMArg isKindOfClass:[NSString class]]
parameters:[OCMArg isKindOfClass:[NSDictionary class]]]);
XCTAssertEqual([self->_fakeLogs count], 2);
XCTAssertEqual([self->_fakeLogs count], 4);

NSDictionary *params1 = @{kArmKey : @"id1", kArmValue : @"value1"};
XCTAssertEqualObjects(self->_fakeLogs[0], params1);
NSDictionary *params1 = @{kChoiceIdKey : @"id1"};
XCTAssertEqualObjects(self->_fakeLogs[1], params1);

NSDictionary *params2 = @{kArmKey : @"id2", kArmValue : @"value2"};
XCTAssertEqualObjects(self->_fakeLogs[1], params2);
NSDictionary *params2 = @{kChoiceIdKey : @"id2"};
XCTAssertEqualObjects(self->_fakeLogs[3], params2);
};

[_configInstance fetchAndActivateWithCompletionHandler:fetchAndActivateCompletion];
Expand Down Expand Up @@ -190,8 +198,17 @@ + (id)mockResponseHandler {
NSDictionary *response = @{
RCNFetchResponseKeyState : RCNFetchResponseKeyStateUpdate,
RCNFetchResponseKeyEntries : @{@"key1" : @"value1", @"key2" : @"value2", @"key3" : @"value3"},
RCNFetchResponseKeyPersonalizationMetadata :
@{@"key1" : @{kPersonalizationId : @"id1"}, @"key2" : @{kPersonalizationId : @"id2"}}
RCNFetchResponseKeyPersonalizationMetadata : @{
@"key1" : @{
kPersonalizationId : @"p13n1",
kArmIndex : @0,
kChoiceId : @"id1",
kGroup : @"BASELINE"
},
@"key2" :
@{kPersonalizationId : @"p13n2", kArmIndex : @1, kChoiceId : @"id2", kGroup : @"P13N"}
}

};
return [OCMArg invokeBlockWithArgs:[NSJSONSerialization dataWithJSONObject:response
options:0
Expand Down