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

Enable Paths Grouping on backend #6052

Merged
merged 5 commits into from
Sep 22, 2021
Merged

Enable Paths Grouping on backend #6052

merged 5 commits into from
Sep 22, 2021

Conversation

neilkakkar
Copy link
Collaborator

@neilkakkar neilkakkar commented Sep 21, 2021

Changes

Part of #4364

Goes after #6035

Decisions made:

  1. Custom match regexes: * matches everything (.* regex). We don't allow custom regexes. We'll probably have to experiment to figure out what are the best operators to expose, but this seemed good enough for now.
  2. The aggregation happens as early as possible: i.e. coming out from PathEventQuery. Doing things like this, start & end points respect aggregations as well, and felt cleaner than doing it in the middle, since that would involve keeping timestamps in order as well. (when 2 events become duplicates because of grouping, these effectively become duplicate events that we dedupe)

Introducing the path_groupings API parameter, which takes a list of strings to match. For example, see test:

def test_path_groupings(self):

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious
  • New/changed UI is decent on smartphones (viewport width around 360px)

@timgl timgl temporarily deployed to posthog-pr-6052 September 21, 2021 12:09 Inactive
@timgl timgl temporarily deployed to posthog-pr-6052 September 21, 2021 12:27 Inactive
@neilkakkar neilkakkar marked this pull request as ready for review September 21, 2021 16:27
Copy link
Member

@EDsCODE EDsCODE left a comment

Choose a reason for hiding this comment

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

Same as other comments. Mostly looks good. The PR description could use more detailing as to what parameters we're now able to use

Also, what's the difference between % and * you were describing above?

@timgl timgl temporarily deployed to posthog-pr-6052 September 22, 2021 12:05 Inactive
@timgl timgl temporarily deployed to posthog-pr-6052 September 22, 2021 12:22 Inactive
@neilkakkar
Copy link
Collaborator Author

Also, what's the difference between % and * you were describing above?

Based on: #4364 (comment) , decided to get rid of second operator.

The difference was: One did .*, like a true wildcard match, while the other, %, matched parts of URLs: [^?&=/]+

Copy link
Member

@EDsCODE EDsCODE left a comment

Choose a reason for hiding this comment

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

Looks good

@neilkakkar neilkakkar merged commit 152dfae into master Sep 22, 2021
@neilkakkar neilkakkar deleted the path_grouping branch September 22, 2021 15:41
pauldambra pushed a commit that referenced this pull request Sep 23, 2021
* enable paths grouping on backend

* write API test for path groupings

* remove % only use *
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.

None yet

3 participants