Standardize support for Firebase products that integrate with Remote Config. #7094
Conversation
| return; | ||
| } | ||
|
|
||
| // This gets dispatched to a serial queue, so this is OK. But even if not, it'll just possibly log | ||
| // more. | ||
| if (self->_armsCache[key] == personalizationId) { |
There was a problem hiding this comment.
does this mean we only want to log each arm once?
There was a problem hiding this comment.
We want to log at least once. We can do de-duping on the back-end.
vic-flair
changed the title
vic-flair
force-pushed
the
vlum.p13n-2
branch
from
9351747
to
e5b90f7
Compare
vic-flair
changed the title
There was a problem hiding this comment.
Seems technically correct to me 👍 I dropped a couple comments re event field naming to improve readability, but since they concern naming, which can be contentious, I flagged them as minor. To avoid miscommunication, I'll defer formal approval to @karenyz, since she's the iOS lead.
| static NSString *const kArmKey = @"_fpid"; | ||
| static NSString *const kArmValue = @"_fpct"; | ||
|
|
||
| static NSString *const kAnalyticsPullEvent = @"personalization_assignment"; |
There was a problem hiding this comment.
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 👍
| kArmValue : values[key].stringValue, | ||
| kPersonalizationIdLogKey : metadata[kPersonalizationId], | ||
| kArmIndexLogKey : metadata[kArmIndex], | ||
| kGroup : metadata[kGroup] |
There was a problem hiding this comment.
Thinking: the design lists five things we want to log:
- Developer friendly identifier of the personalization (either the rc param, or an id customers can map back to rc param), satisfied by the
arm_keyfield - The choice that was made (a string value), satisfied by the
arm_valuefield - The group the user was in (“p13n” or “baseline”), satisfied by the
groupfield - Personalization ID, satisfied by the
personalization_idfield - Arm index, satisfied by the
arm_indexfield
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:@{kChoiceIdLogKey : choiceId}]; |
There was a problem hiding this comment.
Thinking: the design lists one thing we want to log in the internal event:
- Choice ID, satisfied by the
_fpidparam
Thinking: the _fpc event name is correct per the events proposal.
| return; | ||
| } | ||
|
|
||
| // This gets dispatched to a serial queue, so this is OK. But even if not, it'll just possibly |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
"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?
There was a problem hiding this comment.
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."
No description provided.