[Connect] Add analytic events (part 1/2) #4238

mludowise-stripe · 2024-11-07T22:58:14Z

Summary

Adds analytic events matching spec:
https://docs.google.com/document/d/1nzHGofoclzij4nP5n5qgSpYLia-93heStXvt7vOfrkQ/edit?pli=1&tab=t.0#bookmark=id.hbdcbks7wn2n

Also adds catch-all client_error to log unexpected client-side errors with their domain and code.

Additional minor changes:

Only return an HTTPStatusError when the failed URL corresponds to the component page URL. Otherwise we could erroneously return an error if an image asset on the page 404s.
Updated the debug analytics log statement to pretty-print JSON

Motivation

https://jira.corp.stripe.com/browse/MXMOBILE-2491

Testing

Adding unit test coverage in #4239 since this PR is already quite large.

Manual testing

Demonstrates:

component.created
component.viewed
component.web.page_loaded
component.web.component_loaded

Screen.Recording.2024-11-07.at.2.56.04.PM.mov

Demonstrates the following warnings / errors by invoking them from the Safari console:

component.web.error.unexpected_load_error_type
component.web.warn.unrecognized_setter_function
component.web.error.deserialize_message

Screen.Recording.2024-11-07.at.3.18.09.PM.mov

Demonstrates the following by invoking the authenticated web view + redirect from the Safari console:

component.authenticated_web.opened
component.authenticated_web.canceled
component.authenticated_web.redirected
component.authenticated_web.error

Screen.Recording.2024-11-07.at.3.21.37.PM_1080.mov

Changelog

n/a

github-actions · 2024-11-07T23:14:52Z

🚨 New dead code detected in this PR:

ConnectWebViewController.swift:12 warning: Enum 'ConnectWebViewControllerError' is unused

Please remove the dead code before merging.

If this is intentional, you can bypass this check by adding the label skip dead code check to this PR.

ℹ️ If this comment appears to be left in error, double check that the flagged code is actually used and/or make sure your branch is up-to-date with master.

emerge-tools · 2024-11-08T00:16:43Z

⚠️ 1 new unused protocol, 1 build decreased size, 5 builds had no size change

Name	Version	Download	Change	Install	Change	Approval
StripeSize com.stripe.StripeSize	1.0 (1)	2.1 MB	⬇️ 5 B	6.9 MB	-	N/A
StripeApplePaySize com.stripe.StripeApplePaySize	1.0 (1)	448.9 kB	⬇️ 49 B (-0.01%)	1.6 MB	-	N/A
StripeFinancialConnectionsSize com.stripe.StripeFinancialConnectionsSize	1.0 (1)	1.3 MB	⬆️ 7 B	4.4 MB	-	N/A
StripePaymentsSize com.stripe.StripePaymentsSize	1.0 (1)	1.2 MB	⬇️ 1 B	4.1 MB	-	N/A
StripePaymentsUISize com.stripe.StripePaymentsUISize	1.0 (1)	1.9 MB	⬇️ 9 B	6.3 MB	-	N/A
StripePaymentSheetSize com.stripe.StripePaymentSheetSize	1.0 (1)	3.7 MB	⬇️ 5.9 kB (-0.16%)	10.8 MB	⬇️ 8.8 kB (-0.08%)	N/A

StripeSize 1.0 (1)
com.stripe.StripeSize

No changes to report

StripeApplePaySize 1.0 (1)
com.stripe.StripeApplePaySize

No changes to report

StripeFinancialConnectionsSize 1.0 (1)
com.stripe.StripeFinancialConnectionsSize

No changes to report

StripePaymentsSize 1.0 (1)
com.stripe.StripePaymentsSize

No changes to report

StripePaymentsUISize 1.0 (1)
com.stripe.StripePaymentsUISize

No changes to report

StripePaymentSheetSize 1.0 (1)
com.stripe.StripePaymentSheetSize

⚖️ Compare build
⏱️ Analyze build performance

Total install size change: ⬇️ 8.8 kB (-0.08%)
Total download size change: ⬇️ 5.9 kB (-0.16%)

Largest size changes

Item	Install Size Change
🗑 StripePaymentSheet.PaymentSheet.paymentSheetViewControllerDidSele...	⬇️ -5.4 kB
StripePaymentSheet.PayWithLinkController.payWithLinkWebController...	⬇️ -2.8 kB
StripePaymentSheet.PaymentSheetVerticalViewController.didRemove(v...	⬆️ 904 B
StripePaymentSheet.PaymentSheet.FlowController.confirm(from,compl...	⬇️ -804 B
StripePaymentSheet.PaymentSheet.payWithLinkWebControllerDidComple...	⬇️ -664 B

View Treemap

🛸 Powered by Emerge Tools

mludowise-stripe · 2024-11-07T23:54:51Z

StripeCore/StripeCore/Source/Analytics/AnalyticsClientV2.swift

+        let jsonString = String(
+            data: try! JSONSerialization.data(
+                withJSONObject: payload,
+                options: [.sortedKeys, .prettyPrinted]
+            ),
+            encoding: .utf8
+        )!
+        NSLog("LOG ANALYTICS: \(jsonString)")


Printing alphabetized & indented JSON to the console makes it easier to debug. Proposed in slack.

mludowise-stripe · 2024-11-07T23:56:35Z

StripeConnect/StripeConnect/Source/Internal/Analytics/ComponentAnalyticsClient.swift

+                        file: StaticString = #file,
+                        line: UInt = #line) {
+        client.log(
+            eventName: "client_error",


👀 Open to a different name – it's meant to be a catch-all for any unexpected client-side error that we can setup alerts for.

mludowise-stripe · 2024-11-08T19:53:06Z

...eConnect/Source/Internal/Webview/MessageHandlers/Helpers/ScriptMessageHandlerWithReply.swift


+            let response = try value.jsonObject(with: .connectEncoder)
            return (response, nil)


Forwards the original encoding error description to the web layer instead of swallowing it. The web layer can add error logging for this to help us root cause serialization issues.

mludowise-stripe · 2024-11-08T20:46:45Z

🚨 New dead code detected in this PR:
ConnectWebViewController.swift:12 warning: Enum 'ConnectWebViewControllerError' is unused
Please remove the dead code before merging.

If this is intentional, you can bypass this check by adding the label skip dead code check to this PR.

ℹ️ If this comment appears to be left in error, double check that the flagged code is actually used and/or make sure your branch is up-to-date with master.

This is used from a delegate method which isn't detected by the dead code check. Skipping check.

mludowise-stripe · 2024-11-08T20:49:08Z

Stripe.xcworkspace/contents.xcworkspacedata

-   <FileRef
-      location = "group:HTTPStatusError.swift">
-   </FileRef>


This was an invalid file reference that wasn't associated with any project -- cleaned it up.

mludowise-stripe · 2024-11-08T20:50:30Z

StripeConnect/StripeConnect/Source/Internal/Webview/ConnectComponentWebViewController.swift

        if let response = navigationResponse.response as? HTTPURLResponse,
+           response.url?.absoluteStringRemovingParams == StripeConnectConstants.connectJSBaseURL.absoluteString,


Added this check to ensure we only call didFailLoad if the failure happens on the root component page. This method is called for any HTTP request originating from the page, including loading resources like image assets. If an image asset fails to load, we don't want to send a failure callback.

mludowise-stripe · 2024-11-08T20:55:56Z

StripeConnect/StripeConnect/Source/EmbeddedComponentManager.swift

+    var analyticsClientFactory: ComponentAnalyticsClientFactory = {
+        ComponentAnalyticsClient(client: AnalyticsClientV2.sharedConnect,
+                                 commonFields: $0)
+    }


Using a factory pattern for test injection.

Initially I tried adding an AnalyticsClientV2 instance and was planning to use MockAnalyticsClientV2 in tests. But when I started writing tests in #4239, I realized that MockAnalyticsClientV2 would force us to test against the raw analytics payload and we'd be better served by our own Mock (MockComponentAnalyticsClient) so we could test on the ComponentAnalyticEvent types rather than a raw payload.

nschris-stripe · 2024-11-12T15:32:29Z

StripeConnect/StripeConnect/Source/Internal/Webview/ConnectComponentWebViewController.swift

+    override func webView(_ webView: WKWebView, didFinish navigation: WKNavigation!) {
+        super.webView(webView, didFinish: navigation)
+
+        analyticsClient.logComponentWebPageLoaded(loadEnd: .now)


Do we not want to log url here as well?

Analytic events

5da51c2

mludowise-stripe added 3 commits November 7, 2024 15:27

rm dead code

83e319e

docstrings

7928c03

comment

117546c

mludowise-stripe changed the title ~~[Connect] Add analytic events~~ [Connect] Add analytic events (part 1/2) Nov 8, 2024

mludowise-stripe added 4 commits November 7, 2024 16:52

fix tests

60b7bdd

revert accidental change

52dc5d6

MessageSenderError

6f7f586

Use MockComponentAnalyticsClient

d8e31ef

mludowise-stripe commented Nov 8, 2024

View reviewed changes

mludowise-stripe marked this pull request as ready for review November 8, 2024 20:45

mludowise-stripe requested review from a team as code owners November 8, 2024 20:45

mludowise-stripe added the skip dead code check label Nov 8, 2024

mludowise-stripe assigned nschris-stripe Nov 8, 2024

mludowise-stripe commented Nov 8, 2024

View reviewed changes

mludowise-stripe mentioned this pull request Nov 8, 2024

[Connect] Analytics tests (part 2/2) #4239

Draft

nschris-stripe reviewed Nov 12, 2024

View reviewed changes

nschris-stripe approved these changes Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Connect] Add analytic events (part 1/2) #4238

[Connect] Add analytic events (part 1/2) #4238

mludowise-stripe commented Nov 7, 2024 •

edited

Loading

github-actions bot commented Nov 7, 2024 •

edited

Loading

emerge-tools bot commented Nov 8, 2024 •

edited

Loading

mludowise-stripe Nov 7, 2024

mludowise-stripe Nov 7, 2024 •

edited

Loading

mludowise-stripe Nov 8, 2024

mludowise-stripe commented Nov 8, 2024

mludowise-stripe Nov 8, 2024

mludowise-stripe Nov 8, 2024

mludowise-stripe Nov 8, 2024 •

edited

Loading

nschris-stripe Nov 12, 2024


		let response = try value.jsonObject(with: .connectEncoder)
		return (response, nil)

		if let response = navigationResponse.response as? HTTPURLResponse,
		response.url?.absoluteStringRemovingParams == StripeConnectConstants.connectJSBaseURL.absoluteString,

[Connect] Add analytic events (part 1/2) #4238

Are you sure you want to change the base?

[Connect] Add analytic events (part 1/2) #4238

Conversation

mludowise-stripe commented Nov 7, 2024 • edited Loading

Summary

Motivation

Testing

Manual testing

Changelog

github-actions bot commented Nov 7, 2024 • edited Loading

emerge-tools bot commented Nov 8, 2024 • edited Loading

⚠️ 1 new unused protocol, 1 build decreased size, 5 builds had no size change

StripeSize 1.0 (1)com.stripe.StripeSize

StripeApplePaySize 1.0 (1)com.stripe.StripeApplePaySize

StripeFinancialConnectionsSize 1.0 (1)com.stripe.StripeFinancialConnectionsSize

StripePaymentsSize 1.0 (1)com.stripe.StripePaymentsSize

StripePaymentsUISize 1.0 (1)com.stripe.StripePaymentsUISize

StripePaymentSheetSize 1.0 (1)com.stripe.StripePaymentSheetSize

mludowise-stripe Nov 7, 2024

Choose a reason for hiding this comment

mludowise-stripe Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

mludowise-stripe Nov 8, 2024

Choose a reason for hiding this comment

mludowise-stripe commented Nov 8, 2024

mludowise-stripe Nov 8, 2024

Choose a reason for hiding this comment

mludowise-stripe Nov 8, 2024

Choose a reason for hiding this comment

mludowise-stripe Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

nschris-stripe Nov 12, 2024

Choose a reason for hiding this comment

mludowise-stripe commented Nov 7, 2024 •

edited

Loading

github-actions bot commented Nov 7, 2024 •

edited

Loading

emerge-tools bot commented Nov 8, 2024 •

edited

Loading

StripeSize 1.0 (1)
com.stripe.StripeSize

StripeApplePaySize 1.0 (1)
com.stripe.StripeApplePaySize

StripeFinancialConnectionsSize 1.0 (1)
com.stripe.StripeFinancialConnectionsSize

StripePaymentsSize 1.0 (1)
com.stripe.StripePaymentsSize

StripePaymentsUISize 1.0 (1)
com.stripe.StripePaymentsUISize

StripePaymentSheetSize 1.0 (1)
com.stripe.StripePaymentSheetSize

mludowise-stripe Nov 7, 2024 •

edited

Loading

mludowise-stripe Nov 8, 2024 •

edited

Loading