Closed yunapotamus closed 3 years ago
ImpressionTracker now records impression IDs for currently visible items. This allows us to re-use impression IDs between actions and impressions within the same collection view.
What's the use case for reusing the same impression ID? When removing blocking views and it's actually the same underlying view?
Previously, we did not log impression IDs on Action events because inferred joins would take care of that. We're now reconsidering this decision and instead want to set impression IDs on the client. This change has the following supported changes.
Fill in whatever fields you are confident in. If you are not, we can create a new hint field or leave it blank.
Because of these factors, we removed the log*Action methods for most action types, except Navigate
Cool. This is fine. Aligns more with other frameworks.
oslog changes
That's cool. Ah, so it creates a functor of the input? Interesting.
ImpressionTracker now records impression IDs for currently visible items. This allows us to re-use impression IDs between actions and impressions within the same collection view.
What's the use case for reusing the same impression ID? When removing blocking views and it's actually the same underlying view?
When an action is logged against an item in the list, we assign action.impressionID
to be the same impression ID that was logged for the item. Previously we were not filling out impression ID.
oslog changes
That's cool. Ah, so it creates a functor of the input? Interesting.
Yup, @autoclosure
for Swift does that for function parameters.
Fantastic. For an expert review, let me see if I can get Wendy or Ted to review. Otherwise, from a high level, I approve.
Because of these factors, we removed the log*Action methods for most action types, except Navigate.
What is the plan to support manual and/or arbitrary views and/or actions?
We still support calling metricsLogger.logAction
with any of the existing action types. For actions outside of this list, clients can use ActionType.custom
and a client-supplied name. We also support calling logView
, but when the new view ID changes happen we will discourage clients from directly doing so.
This builds the support needed in the
react-native-metrics
module for CollectionTracker. High-level changes are:ImpressionTracker
now records impression IDs for currently visible items. This allows us to re-use impression IDs between actions and impressions within the same collection view.log*Action
methods fromMetricsLogger
to reduce the complexity of integrating with React Native. Instead, allow clients to log content for every action type.OSLog
convenience methods take@autoclosures
for its arguments so that they're not evaluated at runtime unless actually used.This PR doesn't add CollectionTracker for Swift yet. The work here supports that goal, but we're prioritizing React Native first.
Impression IDs
Previously, we did not log impression IDs on Action events because inferred joins would take care of that. We're now reconsidering this decision and instead want to set impression IDs on the client. This change has the following supported changes.
ImpressionTracker
keep an internal map of visible content to impression ID. We only keep a map of visible content because once content becomes invisible, the current impression ends, and the impression ID should never be re-used.ImpressionTracker
to query impression ID for visible content.MetricsLogger
so that we can read the generated impression ID. More about this on the section below.MetricsLogger Methods
Previously, we had a separate
log*Action
method for each action type. This was so that the interface could specify which action types require content as parameters. This approach would up being error-prone (due to React Native NativeModule support) and now is more complicated because we want to return protobufs from thelog*
methods (more on this below). Because of these factors, we removed thelog*Action
methods for most action types, exceptNavigate
. The decrease in complexity is worth the tradeoff of not having semantics of which methods take content parameters.Because protobufs are Swift structs, they are not usable from Objective C. Therefore, any method in
MetricsLogger
that returns a protobuf can't be used from Objective C. In order to support both returning protobufs and Objective C compatibility, eachlog*
method has two versions: One for Objective C and one for Swift. The Objective C version calls the Swift version but does not return any value. For example:Usage from Swift:
Usage from Objective C:
Notes about the implementation:
MetricsLogger
returns the logged proto.@discardableResult
so we don't get compiler warnings when we don't need the logged proto._objc
to make it clear to Swift developers that they shouldn't use this method. We have to keep these methods in the public interface, so this is a workaround.void
return types.@objc
annotation, give the methods friendlier names so that Objective C code calls use those names. (As far as Objective C is concerned, the_objc
version of the method looks like- (void)logImpression:...
in the interface.)OSLog
This change prevents the string arguments passed to logging from being evaluated unless the logging statement is actually executed. So if you have an expression like:
In the previous version,
myComplicatedFunction
would be invoked regardless of whether the logging configuration actually logs a message atinfo
level. By adding@autoclosure
to the logging parameters, this prevents the expression for the parameter (myComplicatedFunction
in this case) from being evaluated ifinfo
level logging does not occur.Caveats
This change does not yet implement the view ID changes. Views are logged with the existing mechanism.