fix: elements api fixes #31067
fix: elements api fixes #31067
Conversation
| @@ -141,7 +141,10 @@ export const heatmapToolbarMenuLogic = kea<heatmapToolbarMenuLogicType>([ | |||
| date_to: values.commonFilters.date_to, | |||
| } | |||
|
|
|||
| defaultUrl = `/api/element/stats/${encodeParams({ ...params, paginate_response: true }, '?')}` | |||
| defaultUrl = `/api/element/stats/${encodeParams( | |||
| { ...params, paginate_response: true, sampling_factor: 0.1 }, | |||
There was a problem hiding this comment.
sampling was still in beta at posthog when we added sampling to elements API, let's allow the client to control it (but default to on)
checking in housewatch, switch to sampling factor isn't significantly faster or slower and is at least a consistent approach
posthog/api/element.py
Outdated
| raise ValidationError("offset must be an integer") | ||
|
|
||
| events_filter = self._events_filter(request) | ||
| timer = ServerTimingsGathered() |
There was a problem hiding this comment.
i "know" this is slow
let's prove it
posthog/api/element.py
Outdated
| date_params.update(date_to_params) | ||
|
|
||
| try: | ||
| limit = int(request.query_params.get("limit", 10_000)) |
There was a problem hiding this comment.
raising the page limit doesn't seem to slow the query down massively so 🤷
| # when multiple includes are sent expects them as separate parameters | ||
| # e.g. ?include=a&include=b | ||
| events_to_include = request.query_params.getlist("include", []) | ||
| def _events_filter(self, request) -> tuple[Literal["$autocapture", "$rageclick", "$dead_click"], ...]: |
There was a problem hiding this comment.
simplify and add dead click support
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR enhances the elements API with performance optimizations and improved timing metrics.
- Added configurable sampling factor (0.1) to element stats API calls in heatmapToolbarMenuLogic to reduce backend processing load
- Added support for
$dead_clickevent type alongside existing$autocaptureand$rageclickevents - Implemented a reusable
ServerTimingsGatheredcontext manager for tracking and reporting server-side timing metrics - Modified SQL query to correctly extrapolate element counts when sampling is used (
count() * %(sampling_factor)s) - Increased default element limit from 250 to 10,000 for more comprehensive data retrieval
5 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
posthog/models/element/sql.py
Outdated
| @@ -1,8 +1,8 @@ | |||
| GET_ELEMENTS = """ | |||
| SELECT | |||
| elements_chain, count() as count, event as event_type | |||
| elements_chain, count() * %(sampling_factor)s as count, event as event_type | |||
There was a problem hiding this comment.
we should apparently always have been adjusting this count 😱
https://clickhouse.com/docs/sql-reference/statements/select/sample#sample-k
| @@ -48,7 +48,6 @@ const EDITABLE_INSTANCE_SETTINGS = [ | |||
| 'RATE_LIMITING_ALLOW_LIST_TEAMS', | |||
| 'SENTRY_AUTH_TOKEN', | |||
| 'SENTRY_ORGANIZATION', | |||
| 'HEATMAP_SAMPLE_N', | |||
There was a problem hiding this comment.
no need to support self hosted config here any more
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
Size Change: +1.82 kB (+0.01%) Total Size: 13.2 MB
|
pauldambra
requested review from
a team,
Copilot,
sortafreel and
veryayskiy
and removed request for
a team
consider me nerd-sniped
has a set of changes from revisiting this after a number of years