Add Buffering, Configurable Endpoint, and Expensive Geometric Appearance Calculation #3
Conversation
…nd configurable (for local env)
| - `analyticsRoot: String`: The root keypath for analytics tokens. For example, in Penn Mobile, all analytics values will look like `pennmobile.{FEATURE}.{SUBFEATURE}`. In that case, the `analyticsRoot = "pennmobile"`. | ||
| - I decided to not make analytics optional. This is because I wanted the analytics requests to be simple with very little room for failure. If analytics were optional, I would want all analytics functions to throw errors instead of silently failing, which would lead to additional complexity when calling these functions. | ||
| - `clientId: String` and `redirectUrl: String`. These are issued by Platform. | ||
| - `configuration: LabsPlatform.Configuration`. See below. |
There was a problem hiding this comment.
Have we ever considered just using DocC instead at this point?
There was a problem hiding this comment.
I did DocC commenting for a lot of stuff that's public facing, README was easier to maintain though.
| } | ||
|
|
||
| extension View { | ||
| @available(*, deprecated, message: "Switch to analytics(_:logViewAppearances:)") |
There was a problem hiding this comment.
Are we using this anywhere? I can't seem to find any uses in Penn Mobile, so if we aren't then we may as well remove this entirely
There was a problem hiding this comment.
It's not it just felt cool to deprecate my own code
| var body: some View { | ||
| Group { | ||
| if logViewAppearances { | ||
| if case .enabled = logViewAppearances { |
There was a problem hiding this comment.
Can we use a switch statement here instead
| var body: some View { | ||
| Group { |
There was a problem hiding this comment.
I think if you annotate the body with @ViewBuilder you might not need the Group here
| } | ||
|
|
||
| public enum ViewAppearanceLoggingMode { | ||
| case disabled, enabled, enabledExpensive |
There was a problem hiding this comment.
We should probably make enabledExpensive more explicit, so maybe something like case disabled, lifecycleBased, screenPositionBased
| } | ||
|
|
||
| func addTimedOperation(_ operation: AnalyticsTimedOperation) { | ||
| func addTimedOperation(_ operation: AnalyticsTimedOperation, removeOnDuplicateName: Bool = true) { |
There was a problem hiding this comment.
I would try to see if there's a way to make this nonisolated so that background threads dealing with analytics don't need to wait on the main thread. In theory this is a theoretical concern but will probably be marginally better for performance down the road
There was a problem hiding this comment.
Yeah blocking the main thread is a concern, especially with as many values as we're about to send over. Will look into.
| guard let (data, response) = try? await URLSession.shared.data(for: request), | ||
| let httpResponse = response as? HTTPURLResponse, | ||
|
|
||
| guard let (data, response) = try? await URLSession.shared.data(for: request), let httpResponse = response as? HTTPURLResponse, |
There was a problem hiding this comment.
We should probably do something if there's an error, no? Not going to block on this but just something to keep in mind
There was a problem hiding this comment.
The error is handled in the queue, where it is added back to the queue in the event of failure, so it'll keep trying to send the value until it hits expiry.
|
Also for reference this should probably have been three separate PRs |
Expensive Geometric Appearance Calculation
For Portal-related appearance calculation, using
onAppearandonDisappearis actually not accurate enough. An option has been added to support a more expensive (computationally) method of calculating when a view enters the screen (by literally measuring when it crosses the viewport)Buffering
Due to this more expensive appearance calculation, we now have to consider that a person spam scrolling on their screen might trigger the appearance methods very frequently. We now have added a 5-second buffer (which can be modified) to ensure that a single token can not be recorded more than once every 5 seconds.
Configurable Endpoint
The endpoint to which Analytics values are sent can now be fully modified to support local testing. I was using a personal project, where I was sending values to
0.0.0.0:80using a local server, and it works great! Using the docker-compose environment for analytics allowed me to also see values hitting the redis cache in real time.