Skip to content
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.

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

[Connect] Add analytics client and spec #9780

Merged
merged 21 commits into from
Jan 9, 2025
Merged

Conversation

simond-stripe
Copy link
Collaborator

@simond-stripe simond-stripe commented Dec 13, 2024

Summary

Creates the analytics client and analytics shapes from the spec.

This PR adds the analytics to the WebViewContainer as early as possible so that it can be used for all event tracking later on, including initialization errors.

I also manually regressed the Payouts and Account Onboarding views in both XML and programmatic configuration, plus checked that props were correctly passed in the Onboarding case to make sure I didn't break anything with initialization.

MXMOBILE-2514

Motivation

This lays the groundwork for analytics emissions in the Connect SDK.

Testing

  • Added tests
  • Modified tests
  • Manually verified

I manually created the analytics client in a view and saw that it could fire an event with all the proper metadata, including the event_metadata nested json in addition to the metadata present at the root-level.

Screenshot 2025-01-07 at 10 22 30 AM

Copy link
Contributor

github-actions bot commented Dec 13, 2024

Diffuse output:

OLD: identity-example-release-base.apk (signature: V1, V2)
NEW: identity-example-release-pr.apk (signature: V1, V2)

          │          compressed          │         uncompressed         
          ├───────────┬───────────┬──────┼───────────┬───────────┬──────
 APK      │ old       │ new       │ diff │ old       │ new       │ diff 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
      dex │     2 MiB │     2 MiB │  0 B │   4.1 MiB │   4.1 MiB │  0 B 
     arsc │     1 MiB │     1 MiB │  0 B │     1 MiB │     1 MiB │  0 B 
 manifest │   2.3 KiB │   2.3 KiB │  0 B │     8 KiB │     8 KiB │  0 B 
      res │ 301.8 KiB │ 301.8 KiB │  0 B │ 455.5 KiB │ 455.5 KiB │  0 B 
   native │   6.2 MiB │   6.2 MiB │  0 B │  15.8 MiB │  15.8 MiB │  0 B 
    asset │   7.1 KiB │   7.1 KiB │  0 B │   6.9 KiB │   6.9 KiB │  0 B 
    other │  90.2 KiB │  90.2 KiB │ -1 B │ 170.3 KiB │ 170.3 KiB │  0 B 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
    total │   9.6 MiB │   9.6 MiB │ -1 B │  21.5 MiB │  21.5 MiB │  0 B 

 DEX     │ old   │ new   │ diff      
─────────┼───────┼───────┼───────────
   files │     1 │     1 │ 0         
 strings │ 19971 │ 19971 │ 0 (+0 -0) 
   types │  6191 │  6191 │ 0 (+0 -0) 
 classes │  4982 │  4982 │ 0 (+0 -0) 
 methods │ 29772 │ 29772 │ 0 (+0 -0) 
  fields │ 17542 │ 17542 │ 0 (+0 -0) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  164 │  164 │  0   
 entries │ 3622 │ 3622 │  0
APK
   compressed    │  uncompressed   │                                           
──────────┬──────┼──────────┬──────┤                                           
 size     │ diff │ size     │ diff │ path                                      
──────────┼──────┼──────────┼──────┼───────────────────────────────────────────
 28.4 KiB │ -2 B │ 62.9 KiB │  0 B │ ∆ META-INF/CERT.SF                        
    272 B │ +1 B │    120 B │  0 B │ ∆ META-INF/version-control-info.textproto 
──────────┼──────┼──────────┼──────┼───────────────────────────────────────────
 28.7 KiB │ -1 B │   63 KiB │  0 B │ (total)

@simond-stripe simond-stripe changed the title Add analytics client and spec [Connect] Add analytics client and spec Dec 13, 2024
@simond-stripe simond-stripe force-pushed the simond/connect-analytics branch from 8671bad to 9e55604 Compare December 20, 2024 22:13
@simond-stripe simond-stripe marked this pull request as ready for review January 7, 2025 17:10
@simond-stripe simond-stripe requested a review from a team as a code owner January 7, 2025 17:10
/**
* Analytics event for the Connect SDK. One subclass per unique analytics event is expected.
*/
internal sealed class ConnectAnalyticsEvent(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@lng-stripe lng-stripe left a comment

Choose a reason for hiding this comment

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

Nice! Minor things and would like to start improving our testing story.

/**
* Log an analytic [event].
*/
fun track(event: ConnectAnalyticsEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently unused. Can you add sending at least ComponentCreated to prove it works?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I already did this (see here) but backed out the change once I proved it works because I want all the emissions to be added in #9873.

/**
* Log an analytic [event].
*/
fun track(event: ConnectAnalyticsEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add unit tests for this? At least verify the params are being added.

Comment on lines 18 to 20
* Service for logging [AnalyticsRequestV2] for the Connect SDK.
* This service is very simple. Consumers should prefer [ComponentAnalyticsService] instead,
* which uses this service internally.
Copy link
Contributor

Choose a reason for hiding this comment

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

This docstring is inaccurate.

Suggested change
* Service for logging [AnalyticsRequestV2] for the Connect SDK.
* This service is very simple. Consumers should prefer [ComponentAnalyticsService] instead,
* which uses this service internally.
* Analytics service configured for Connect SDK.

Copy link
Collaborator Author

@simond-stripe simond-stripe Jan 9, 2025

Choose a reason for hiding this comment

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

What's inaccurate about this docstring? Is it specifically mentioning AnalyticsRequestV2, since that's the type we log but not the type the service exposes?

I think it's still useful to recommend consumers to ComponentAnalyticsService (which is what should be used in other parts of the SDK anyways), @lng-stripe let me know what you think of the updated docstring.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I might've misread it. I thought it was saying ConnectAnalyticsService uses ComponentAnalyticsService internally, but it's saying the opposite (which is true).

Still, I'm not a fan of referencing downstream dependencies since it's not clean and adds maintenance cost.

* This service is very simple. Consumers should prefer [ComponentAnalyticsService] instead,
* which uses this service internally.
*/
internal class ConnectAnalyticsService(application: Application) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally this would be abstracted for testability. Imagine unit testing EmbeddedComponentManager.onActivityCreate(), which we should be doing (either now or after the public API is finalized).

@simond-stripe
Copy link
Collaborator Author

@lng-stripe this is ready for re-review

Copy link
Contributor

@lng-stripe lng-stripe left a comment

Choose a reason for hiding this comment

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

👍 👍

@simond-stripe simond-stripe merged commit 924207c into master Jan 9, 2025
13 checks passed
@simond-stripe simond-stripe deleted the simond/connect-analytics branch January 9, 2025 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants