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

feat(autocapture): auto capture element interactions #224

Merged
merged 31 commits into from
Nov 8, 2024

Conversation

ioannisj
Copy link
Contributor

@ioannisj ioannisj commented Oct 23, 2024

💡 Motivation and Context

Autocapture feature for iOS (UIKit)

FE: PostHog/posthog#25763, PostHog/posthog#25996
Docs: PostHog/posthog.com#9783

Capture
Elements

Elements

  • UIButton
  • UIMenu (and UIAction)
  • UIPageControl
  • UISearchBar
  • UISegmentedControl
  • UISlider
  • UISwitch
  • UIStepper
  • UITextField
  • UITextView
  • UIAlertController (alert)
  • UIAlertController (action sheet)
  • UIToolbar
  • UINavigationBar
  • UIDatePicker
  • UIColorPickerViewController
  • UIFontPicker
  • UIImagePickerController
  • UIPickerView
  • UIScrollView (UITableView, UICollectionView)

Gestures

  • UITapGestureRecognizer
  • UISwipeGestureRecognizer
  • UIPanGestureRecognizer
  • UILongPressGestureRecognizer
  • UIPinchGestureRecognizer
  • UIRotationGestureRecognizer
  • UIScreenEdgePanGestureRecognizer

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

@ioannisj
Copy link
Contributor Author

I wonder if at least some parts can be unit-tested?

Yeah, I can add some on config, sanitization and potentially swizzling. For the rest, not sure it's going to be effective without diving into UI tests?

@marandaneto
Copy link
Member

I wonder if at least some parts can be unit-tested?

Yeah, I can add some on config, sanitization and potentially swizzling. For the rest, not sure it's going to be effective without diving into UI tests?

I'd not focus on UI tests now, just the bits that are possible via unit tests but still asserting/testing the feature.

@ioannisj ioannisj marked this pull request as ready for review October 31, 2024 11:12
@ioannisj
Copy link
Contributor Author

ioannisj commented Nov 1, 2024

Working on the docs made me realize that this new configuration could be confusing to the users.

We already have captureApplicationLifecycleEvents and captureScreenViews which could both be considered an Autocapture feature.

Currently, the config looks like this:

config.autocapture = true // $autocapture support
config.autocaptureConfig = true // configuration for autocapture element interactions
config.captureApplicationLifecycleEvents = true // Application Opened etc
config.captureScreenViews = true // $screen support

Ideally, all of these configs should be nested in autocaptureConfig and controlled via the autocapture option. I don't want to introduce a breaking change especially for an experimental feature, so I propose the following

config.captureApplicationLifecycleEvents = true // Application Open etc
config.captureScreenViews = true // $screen support
config.captureInteractions = true // $autocapture support

Once this is battle tested we can think about nesting or grouping the configurations.

What do you guys think? @marandaneto @pauldambra

@marandaneto
Copy link
Member

Working on the docs made me realize that this new configuration could be confusing to the users.

We already have captureApplicationLifecycleEvents and captureScreenViews which could both be considered an Autocapture feature.

Currently, the config looks like this:

config.autocapture = true // $autocapture support
config.autocaptureConfig = true // configuration for autocapture element interactions
config.captureApplicationLifecycleEvents = true // Application Opened etc
config.captureScreenViews = true // $screen support

Ideally, all of these configs should be nested in autocaptureConfig and controlled via the autocapture option. I don't want to introduce a breaking change especially for an experimental feature, so I propose the following

config.captureApplicationLifecycleEvents = true // Application Open etc
config.captureScreenViews = true // $screen support
config.captureInteractions = true // $autocapture support

Once this is battle tested we can think about nesting or grouping the configurations.

What do you guys think? @marandaneto @pauldambra

While I agree with the confusion, JS is the same, see https://posthog.com/docs/product-analytics/autocapture
autocapture != capture_pageview, they are different things and the same applies here.
Maybe we should stop calling the app life cycle events, screen views, etc as autocapture and define autocapture as its documented in the docs as only auto capture control events such as clicks, taps, etc.
In this case, I just would fix the headers in the docs as "automatically captured" for those screen views, app lifecycle events, etc and "autocapture" for the auto capture feature.

config.captureApplicationLifecycleEvents = true // Application Open etc
config.captureScreenViews = true // $screen support
config.autocapture = true // $autocapture support
config.autocaptureConfig {
...
}

how does that sound? it was probably my mistake to call such events "autocapture" as well because it was unclear to me that autocapture was only about the $autocapture events that are only captured for the control events.

@ioannisj
Copy link
Contributor Author

ioannisj commented Nov 4, 2024

Yeah, I see that JS distinguishes in a similar way between capture_pageview and autocapture configuration options.

However, in the intro section of the docs, under Autocapture, pageview and screen are included. I think this is what throws me off a bit for both. It's only until you get to Disabling autocapture section that it becomes a bit clear that autocapture = false does not turn off $pageview and $pageleave events

Autocapture

PostHog can capture frontend events automatically using autocapture. This captures events like pageview, screen, click, change of input, or submission associated with a button, form, input, select, or textarea.

I think I'll go with your suggestion for now, keep this as-is, and rework the docs a bit. But in general I think we need to clear this up in both libs. If we don't want to combine all events under "Autocapture", then maybe it's better if we start calling "Application Open", "Page view", "Page leave" "Screen" etc as Default events instead

@marandaneto
Copy link
Member

However, in the intro section of the docs, under Autocapture, pageview and screen are included. I think this is what throws me off a bit for both.

@ioannisj it's a good point, I also noticed that when reading it again.
Double-check with the prod analytics team about that please, if page views and screens are not "autocapture", then we should fix the docs https://posthog.com/docs/product-analytics/autocapture or consider them as "autocapture" as well on mobile SDKs which is what I did before, but we need to be more clear on the docs about the different configurations.

# Conflicts:
#	CHANGELOG.md
#	PostHogTests/PostHogSDKTest.swift
@ioannisj
Copy link
Contributor Author

ioannisj commented Nov 5, 2024

@marandaneto updated docs. See links in body

@ioannisj ioannisj merged commit b7c3b67 into main Nov 8, 2024
6 checks passed
@ioannisj ioannisj deleted the feat/auto-capture-element-interactions branch November 8, 2024 14:58
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