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

Proposal: Remove Autocapture functionality from Paths #5895

Merged
merged 5 commits into from
Sep 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 1 addition & 20 deletions ee/clickhouse/queries/paths/path_event_query.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
from ee.clickhouse.models.property import get_property_string_expr
from ee.clickhouse.queries.event_query import ClickhouseEventQuery
from posthog.constants import (
AUTOCAPTURE_EVENT,
FUNNEL_PATH_AFTER_STEP,
FUNNEL_PATH_BEFORE_STEP,
FUNNEL_PATH_BETWEEN_STEPS,
Expand Down Expand Up @@ -55,12 +54,7 @@ def get_query(self) -> Tuple[str, Dict[str, Any]]:
if self._should_query_url()
else "if(0, '', "
)
event_conditional += (
f"if({self.EVENT_TABLE_ALIAS}.event = '{AUTOCAPTURE_EVENT}', concat('autocapture:', {self.EVENT_TABLE_ALIAS}.elements_chain), "
if self._should_query_elements_chain()
else "if(0, '', "
)
event_conditional += f"{self.EVENT_TABLE_ALIAS}.event))) AS path_item"
event_conditional += f"{self.EVENT_TABLE_ALIAS}.event)) AS path_item"

_fields.append(event_conditional)

Expand Down Expand Up @@ -113,9 +107,6 @@ def _get_event_query(self) -> Tuple[str, Dict[str, Any]]:
if self._filter.include_screenviews:
or_conditions.append(f"event = '{SCREEN_EVENT}'")

if self._filter.include_autocaptures:
or_conditions.append(f"event = '{AUTOCAPTURE_EVENT}'")

if self._filter.include_all_custom_events:
or_conditions.append(f"NOT event LIKE '$%%'")

Expand Down Expand Up @@ -154,13 +145,3 @@ def _should_query_screen(self) -> bool:
return True

return False

def _should_query_elements_chain(self) -> bool:
if (
self._filter.target_events == [] and self._filter.custom_events == []
) and AUTOCAPTURE_EVENT not in self._filter.exclude_events:
return True
elif self._filter.include_autocaptures:
return True

return False
2 changes: 0 additions & 2 deletions ee/clickhouse/queries/paths/paths.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,13 @@ def __init__(self, filter: PathFilter, team: Team, funnel_filter: Optional[Filte
"events": [], # purely a speed optimization, don't need this for filtering
"event_in_session_limit": self._filter.step_limit or EVENT_IN_SESSION_LIMIT_DEFAULT,
"session_time_threshold": SESSION_TIME_THRESHOLD_DEFAULT,
"autocapture_match": "%autocapture:%",
}
self._funnel_filter = funnel_filter

if self._filter.include_all_custom_events and self._filter.custom_events:
raise ValidationError("Cannot include all custom events and specific custom events in the same query")

# TODO: don't allow including $pageview and excluding $pageview at the same time
# TODO: Filter on specific autocapture / page URLs

def run(self, *args, **kwargs):

Expand Down
27 changes: 9 additions & 18 deletions ee/clickhouse/queries/test/test_paths.py
Original file line number Diff line number Diff line change
Expand Up @@ -1106,30 +1106,26 @@ def test_paths_start_and_end(self):
)

def test_properties_queried_using_path_filter(self):
def should_query_list(filter) -> Tuple[bool, bool, bool]:
def should_query_list(filter) -> Tuple[bool, bool]:
path_query = PathEventQuery(filter, self.team.id)
return (
path_query._should_query_url(),
path_query._should_query_screen(),
path_query._should_query_elements_chain(),
)
return (path_query._should_query_url(), path_query._should_query_screen())

filter = PathFilter()
self.assertEqual(should_query_list(filter), (True, True, True))
self.assertEqual(should_query_list(filter), (True, True))

filter = PathFilter({"include_event_types": ["$pageview"]})
self.assertEqual(should_query_list(filter), (True, False, False))
self.assertEqual(should_query_list(filter), (True, False))

filter = PathFilter({"include_event_types": ["$screen"]})
self.assertEqual(should_query_list(filter), (False, True, False))
self.assertEqual(should_query_list(filter), (False, True))

filter = filter.with_data({"include_event_types": [], "include_custom_events": ["/custom1", "/custom2"]})
self.assertEqual(should_query_list(filter), (False, False, False))
self.assertEqual(should_query_list(filter), (False, False))

filter = filter.with_data(
{"include_event_types": ["$pageview", "$screen", "custom_event"], "include_custom_events": []}
)
self.assertEqual(should_query_list(filter), (True, True, False))
self.assertEqual(should_query_list(filter), (True, True))

filter = filter.with_data(
{
Expand All @@ -1138,14 +1134,9 @@ def should_query_list(filter) -> Tuple[bool, bool, bool]:
"exclude_events": ["/custom1"],
}
)
self.assertEqual(should_query_list(filter), (True, True, False))
self.assertEqual(should_query_list(filter), (True, True))

filter = filter.with_data(
{"include_event_types": [], "include_custom_events": [], "exclude_events": ["$pageview"],}
)
self.assertEqual(should_query_list(filter), (False, True, True))

filter = filter.with_data(
{"include_event_types": [], "include_custom_events": [], "exclude_events": ["$autocapture"],}
)
self.assertEqual(should_query_list(filter), (True, True, False))
self.assertEqual(should_query_list(filter), (False, True))
12 changes: 1 addition & 11 deletions ee/clickhouse/sql/paths/path.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,16 +137,7 @@


PATH_ARRAY_QUERY = """
SELECT if(target_event LIKE %(autocapture_match)s, concat(arrayElement(splitByString('autocapture:', assumeNotNull(source_event)), 1), final_source_element), source_event) final_source_event,
if(target_event LIKE %(autocapture_match)s, concat(arrayElement(splitByString('autocapture:', assumeNotNull(target_event)), 1), final_target_element), target_event) final_target_event,
event_count,
average_conversion_time,
if(target_event LIKE %(autocapture_match)s, arrayElement(splitByString('autocapture:', assumeNotNull(source_event)), 2), NULL) source_event_elements_chain,
concat('<', extract(source_event_elements_chain, '^(.*?)[.|:]'), '> ', extract(source_event_elements_chain, 'text="(.*?)"')) final_source_element,
if(target_event LIKE %(autocapture_match)s, arrayElement(splitByString('autocapture:', assumeNotNull(target_event)), 2), NULL) target_event_elements_chain,
concat('<', extract(target_event_elements_chain, '^(.*?)[.|:]'), '> ', extract(target_event_elements_chain, 'text="(.*?)"')) final_target_element
FROM (
SELECT last_path_key as source_event,
SELECT last_path_key as source_event,
path_key as target_event,
COUNT(*) AS event_count,
avg(conversion_time) AS average_conversion_time
Expand Down Expand Up @@ -201,6 +192,5 @@
source_event,
target_event
LIMIT 20
)

"""
2 changes: 0 additions & 2 deletions frontend/src/scenes/insights/InsightTabs/PathTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ export function PathTab({ annotationsToCreate }: BaseTabProps): JSX.Element {
<Col>starting at</Col>
<Col>
<PropertyValue
endpoint={filter.path_type === PathType.AutoCapture ? 'api/paths/elements' : undefined}
outerOptions={
filter.path_type === PathType.CustomEvent
? customEventNames.map((name) => ({
Expand All @@ -72,7 +71,6 @@ export function PathTab({ annotationsToCreate }: BaseTabProps): JSX.Element {
value={filter.start_point}
placeholder={'Select start element'}
autoFocus={false}
allowCustom={filter.path_type !== PathType.AutoCapture}
/>
</Col>
</Row>
Expand Down
2 changes: 0 additions & 2 deletions frontend/src/scenes/paths/pathsLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,12 @@ import { dashboardsModel } from '~/models/dashboardsModel'
export const pathOptionsToLabels = {
[PathType.PageView]: 'Page views (Web)',
[PathType.Screen]: 'Screen views (Mobile)',
[PathType.AutoCapture]: 'Autocaptured events',
[PathType.CustomEvent]: 'Custom events',
}

export const pathOptionsToProperty = {
[PathType.PageView]: '$current_url',
[PathType.Screen]: '$screen_name',
[PathType.AutoCapture]: 'autocaptured_event',
[PathType.CustomEvent]: 'custom_event',
}

Expand Down
1 change: 0 additions & 1 deletion frontend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,6 @@ export enum ViewType {

export enum PathType {
PageView = '$pageview',
AutoCapture = '$autocapture',
Screen = '$screen',
CustomEvent = 'custom_event',
}
Expand Down
13 changes: 1 addition & 12 deletions posthog/models/filters/mixins/paths.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
from typing import Dict, List, Literal, Optional, Tuple, cast

from posthog.constants import (
AUTOCAPTURE_EVENT,
CUSTOM_EVENT,
END_POINT,
FUNNEL_PATHS,
Expand All @@ -17,7 +16,7 @@
from posthog.models.filters.mixins.common import BaseParamMixin
from posthog.models.filters.mixins.utils import cached_property, include_dict, process_bool

PathType = Literal["$pageview", "$autocapture", "$screen", "custom_event"]
PathType = Literal["$pageview", "$screen", "custom_event"]

FunnelPathsType = Literal["funnel_path_before_step", "funnel_path_between_steps", "funnel_path_after_step"]

Expand Down Expand Up @@ -57,8 +56,6 @@ class PropTypeDerivedMixin(PathTypeMixin):
def prop_type(self) -> str:
if self.path_type == SCREEN_EVENT:
return "properties->> '$screen_name'"
elif self.path_type == AUTOCAPTURE_EVENT:
return "tag_name_source"
elif self.path_type == CUSTOM_EVENT:
return "event"
else:
Expand All @@ -70,8 +67,6 @@ class ComparatorDerivedMixin(PropTypeDerivedMixin):
def comparator(self) -> str:
if self.path_type == SCREEN_EVENT:
return "{} =".format(self.prop_type)
elif self.path_type == AUTOCAPTURE_EVENT:
return "group_id ="
elif self.path_type == CUSTOM_EVENT:
return "event ="
else:
Expand All @@ -83,8 +78,6 @@ class TargetEventDerivedMixin(PropTypeDerivedMixin):
def target_event(self) -> Tuple[Optional[PathType], Dict[str, str]]:
if self.path_type == SCREEN_EVENT:
return cast(PathType, SCREEN_EVENT), {"event": SCREEN_EVENT}
elif self.path_type == AUTOCAPTURE_EVENT:
return cast(PathType, AUTOCAPTURE_EVENT), {"event": AUTOCAPTURE_EVENT}
elif self.path_type == CUSTOM_EVENT:
return None, {}
else:
Expand Down Expand Up @@ -112,10 +105,6 @@ def include_pageviews(self) -> bool:
def include_screenviews(self) -> bool:
return SCREEN_EVENT in self.target_events

@property
def include_autocaptures(self) -> bool:
return AUTOCAPTURE_EVENT in self.target_events

@property
def include_all_custom_events(self) -> bool:
return CUSTOM_EVENT in self.target_events
Expand Down
3 changes: 0 additions & 3 deletions posthog/queries/paths.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,6 @@ def calculate_paths(self, filter: PathFilter, team: Team):

sessions_sql, sessions_sql_params = sessions.query.sql_with_params()

if event == "$autocapture":
sessions_sql = self._add_elements(query_string=sessions_sql)

events_notated = "\
SELECT *, CASE WHEN EXTRACT('EPOCH' FROM (timestamp - previous_timestamp)) >= (60 * 30) OR previous_timestamp IS NULL THEN 1 ELSE 0 END AS new_session\
FROM ({}) AS inner_sessions\
Expand Down
87 changes: 0 additions & 87 deletions posthog/queries/test/test_paths.py
Original file line number Diff line number Diff line change
Expand Up @@ -228,93 +228,6 @@ def test_screen_paths(self):
self.assertEqual(response[3]["target"], "3_/about")
self.assertEqual(response[3]["value"], 1)

def test_autocapture_paths(self):
person_factory(team_id=self.team.pk, distinct_ids=["person_1"])

event_factory(
event="$autocapture",
team=self.team,
distinct_id="person_1",
elements=[
Element(tag_name="a", text="hello", href="/a-url", nth_child=1, nth_of_type=0),
Element(tag_name="button", nth_child=0, nth_of_type=0),
Element(tag_name="div", nth_child=0, nth_of_type=0),
Element(tag_name="div", nth_child=0, nth_of_type=0, attr_id="nested",),
],
)

event_factory(
event="$autocapture",
team=self.team,
distinct_id="person_1",
elements=[
Element(tag_name="a", text="goodbye", nth_child=2, nth_of_type=0, attr_id="someId",),
Element(tag_name="div", nth_child=0, nth_of_type=0),
# make sure elements don't get double counted if they're part of the same event
Element(href="/a-url-2", nth_child=0, nth_of_type=0),
],
)

person_factory(team_id=self.team.pk, distinct_ids=["person_2"])
event_factory(
event="$autocapture",
team=self.team,
distinct_id="person_2",
elements=[
Element(tag_name="a", text="hello1", href="/a-url", nth_child=1, nth_of_type=0,),
Element(tag_name="button", nth_child=0, nth_of_type=0),
Element(tag_name="div", nth_child=0, nth_of_type=0),
Element(tag_name="div", nth_child=0, nth_of_type=0, attr_id="nested",),
],
)

event_factory(
event="$autocapture",
team=self.team,
distinct_id="person_2",
elements=[
Element(tag_name="a", text="goodbye1", nth_child=2, nth_of_type=0, attr_id="someId",),
Element(tag_name="div", nth_child=0, nth_of_type=0),
# make sure elements don't get double counted if they're part of the same event
Element(href="/a-url-2", nth_child=0, nth_of_type=0),
],
)

event_factory(
event="$autocapture",
team=self.team,
distinct_id="person_2",
elements=[
Element(tag_name="div", text="goodbye1", nth_child=2, nth_of_type=0, attr_id="someId",),
Element(tag_name="div", nth_child=0, nth_of_type=0),
# make sure elements don't get double counted if they're part of the same event
Element(href="/a-url-2", nth_child=0, nth_of_type=0),
],
)
filter = PathFilter(data={"path_type": "$autocapture"})

response = paths(team=self.team, filter=filter).run(team=self.team, filter=filter)

self.assertEqual(response[0]["source"], "1_<a> hello")
self.assertEqual(response[0]["target"], "2_<a> goodbye")
self.assertEqual(response[0]["value"], 1)

self.assertEqual(response[1]["source"], "1_<a> hello1")
self.assertEqual(response[1]["target"], "2_<a> goodbye1")
self.assertEqual(response[1]["value"], 1)

self.assertEqual(response[2]["source"], "2_<a> goodbye1")
self.assertEqual(response[2]["target"], "3_<div> goodbye1")
self.assertEqual(response[2]["value"], 1)

elements = self.client.get("/api/paths/elements/").json()
self.assertEqual(elements[0]["name"], "<a> goodbye") # first since captured twice
self.assertEqual(elements[1]["name"], "<a> goodbye1")
self.assertEqual(elements[2]["name"], "<a> hello")
self.assertEqual(elements[3]["name"], "<a> hello1")
self.assertEqual(elements[4]["name"], "<div> goodbye1")
self.assertEqual(len(elements), 5)

def test_paths_properties_filter(self):
person_factory(team_id=self.team.pk, distinct_ids=["person_1"])
event_factory(
Expand Down