Capture Endpoint Refactor #6230

yakkomajuri · 2021-10-04T15:20:35Z

Important

This is currently not up for review, but when it is, it should be reviewed with care as it's a refactor of the entry point for our ingestion pipeline!

Changes

Refactoring the capture endpoint as outlined in #6227.

All in all, this:

Ensures we process other events if one event in a batch is invalid
Sends events to a dead letter queue if there's a DB error e.g. Postgres is down (+ metrics)
Adds tests for the capture endpoint on EE deployments (they didn't exist)

It's recommended to merge #6193 first, although it doesn't really matter.

There are certainly other things that could be done, but this PR is already large enough for ingestion code, so would like to keep the scope at this for now.

How did you test this code?

Updated Postgres tests, added a new Postgres test, added EE tests.

yakkomajuri · 2021-10-04T15:21:51Z

posthog/api/test/test_capture.py

@@ -53,7 +57,6 @@ def _to_arguments(self, patch_process_event_with_plugins: Any) -> dict:
            "sent_at": sent_at,
        }

-    @patch("posthog.models.team.TEAM_CACHE", {})


since I was updating a lot of tests, felt like this change could be bundled up here.

We got rid of TEAM_CACHE a long time ago

codecov-commenter · 2021-10-04T15:59:29Z

Codecov Report

❗ No coverage uploaded for pull request base (master@823b3e7). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #6230   +/-   ##
=========================================
  Coverage          ?   91.86%           
=========================================
  Files             ?      400           
  Lines             ?    30294           
  Branches          ?     2566           
=========================================
  Hits              ?    27830           
  Misses            ?     1935           
  Partials          ?      529

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 823b3e7...34e7441. Read the comment docs.

ee/api/test/test_capture.py

posthog/api/capture.py

guidoiaquinti · 2021-10-05T12:35:15Z

posthog/api/test/test_capture.py

-                "You need to set user distinct ID field `distinct_id`.", code="required", attr="distinct_id"
-            ),
-        )
+        # An invalid distinct ID will not return an error code, instead we will capture an exception


Now I see. Is this a change that might need coordination with SDKs, docs and so on?

It shouldn't need coordination with SDKs, docs might be a good addition.

While this sounds like a big change (our most important API behaves differently after this), it's not that bad when you consider that this was actually a bug.

Currently, if you send us 999 valid events, and the last event is an invalid one, we'll send you back a 4xx response. That might lead a client to retry sending these (think platforms like RudderStack or Segment), leading to a lot of duplicates.

Anyway, none of our SDKs rely on these particular errors. But indeed it should be made clear on the docs that if you send us a batch of events we will not tell you if some events in there are invalid.

Here's a start on docs: PostHog/posthog.com#2178

yakkomajuri · 2021-10-05T15:51:03Z

The whole platform team is tagged to review just so people have context. But tagged you @Twixes for quickly batting an eye just because you've worked a lot with ingestion/our API in general and would be important to have this change be well-reviewed

tiina303

didn't review thoroughly yet

posthog/api/capture.py

yakkomajuri · 2021-10-07T12:03:06Z

A lot of tests/improvements incoming

yakkomajuri · 2021-10-07T14:18:19Z

You mean manually testing it? @guidoiaquinti

yakkomajuri · 2021-10-07T15:02:07Z

@guidoiaquinti @tiina303 I made this much easier to review by splitting the DLQ code into another PR with this as its base (#6311). That should be merged before this.

This PR now is very limited in behavior changes and is mostly a refactor with some tests.

The key behavioral change is not sending errors to the client on an invalid event, and instead processing the rest of the batch. That's like a few lines. The rest is moving stuff around and adding more tests.

tiina303

Nice work & thanks for breaking it up & making the PRs smaller. Much easier to review this way =)

posthog/api/utils.py

posthog/api/decide.py

posthog/api/utils.py

posthog/api/capture.py

posthog/api/test/test_capture.py

yakkomajuri · 2021-10-08T10:57:53Z

all addressed, thanks a lot!

Will check that tests pass and resolve the conflicts with #6311 so we can merge that here and get this in early next week (when we can keep a close eye on it)

* Add dead letter queue to capture endpoint This reverts commit 34e7441. * try except kafka produce * format * statsd + log for dlq produce error * fix types * fix * fix * legit fix * ignore type

yakkomajuri · 2021-10-08T15:49:41Z

Ok so we're back to one PR now. @tiina303 maybe give this a final look focusing only on any potential serious bugs/holes. Anything stylistic we can fix elsewhere - the scope is already much larger than what we started with.

tiina303

LGTM, added two nits, but feel free to ignore or consider doing in a follow-up.

posthog/api/capture.py

tiina303 · 2021-10-08T16:51:09Z

posthog/api/utils.py

+        if not is_clickhouse_enabled():
+            error_response = cors_response(


nit: I'd drop this condition (since it's not this function's job to decide if it's used or not, later we might do something different for single capture vs batch for example) & in the usage part check

if not team and not send_to_dead_letter_queue: return error_response

tiina303 · 2021-10-08T16:54:23Z

In the future I'd recommend doing it the other way - land this one & then rebase the other pr on top of master & then land it ... smaller changes ... easier to revert.

WIP refactor capture endpoint + DLQ

1da02e2

yakkomajuri commented Oct 4, 2021

View reviewed changes

timgl temporarily deployed to posthog-pr-6230 October 4, 2021 15:22 Inactive

yakkomajuri added 2 commits October 4, 2021 17:06

Merge branch 'master' into capture-refactor-plus-dlq

88516c5

fix typing

46a4ddb

yakkomajuri temporarily deployed to posthog-pr-6230 October 4, 2021 17:09 Inactive

more testing

95b8213

yakkomajuri temporarily deployed to posthog-pr-6230 October 5, 2021 10:26 Inactive

yakkomajuri commented Oct 5, 2021

View reviewed changes

ee/api/test/test_capture.py Show resolved Hide resolved

add comments

3ed14be

yakkomajuri temporarily deployed to posthog-pr-6230 October 5, 2021 10:30 Inactive

yakkomajuri commented Oct 5, 2021

View reviewed changes

posthog/api/capture.py Outdated Show resolved Hide resolved

format

a058f14

yakkomajuri temporarily deployed to posthog-pr-6230 October 5, 2021 10:39 Inactive

yakkomajuri changed the title ~~WIP Capture Endpoint Refactor + Dead Letter Queue~~ Capture Endpoint Refactor + Dead Letter Queue Oct 5, 2021

yakkomajuri requested review from fuziontech, guidoiaquinti and tiina303 October 5, 2021 11:30

guidoiaquinti reviewed Oct 5, 2021

View reviewed changes

posthog/api/capture.py Show resolved Hide resolved

guidoiaquinti reviewed Oct 5, 2021

View reviewed changes

yakkomajuri requested a review from Twixes October 5, 2021 15:49

yakkomajuri mentioned this pull request Oct 6, 2021

Events dead letter queue CH table #6193

Merged

tiina303 reviewed Oct 6, 2021

View reviewed changes

posthog/api/capture.py Show resolved Hide resolved

posthog/api/capture.py Outdated Show resolved Hide resolved

posthog/api/capture.py Outdated Show resolved Hide resolved

yakkomajuri added 3 commits October 7, 2021 11:07

refactor get_event into smaller modular functions

f72e979

move functions to utils

326aa95

add typing

746c911

yakkomajuri temporarily deployed to posthog-pr-6230 October 7, 2021 11:27 Inactive

yakkomajuri requested a deployment to posthog-pr-6230 October 7, 2021 14:08 Abandoned

remove dlq code

34e7441

yakkomajuri temporarily deployed to posthog-pr-6230 October 7, 2021 14:37 Inactive

yakkomajuri mentioned this pull request Oct 7, 2021

Add dead letter queue to capture endpoint #6311

Merged

yakkomajuri changed the title ~~Capture Endpoint Refactor + Dead Letter Queue~~ Capture Endpoint Refactor Oct 7, 2021

tiina303 approved these changes Oct 7, 2021

View reviewed changes

yakkomajuri added 4 commits October 8, 2021 10:30

determine_team_from_request -> get_team

5c937aa

extract_data_from_request -> get_data

3d5e1d3

minor fixes

2b08a06

extract get_distinct_id, parse_event

8e94e44

yakkomajuri temporarily deployed to posthog-pr-6230 October 8, 2021 10:45 Inactive

add test

a2a42b8

yakkomajuri temporarily deployed to posthog-pr-6230 October 8, 2021 10:54 Inactive

Add dead letter queue to capture endpoint (#6311)

7c60cb3

* Add dead letter queue to capture endpoint This reverts commit 34e7441. * try except kafka produce * format * statsd + log for dlq produce error * fix types * fix * fix * legit fix * ignore type

yakkomajuri temporarily deployed to posthog-pr-6230 October 8, 2021 15:48 Inactive

tiina303 approved these changes Oct 8, 2021

View reviewed changes

merge master

9157b85

yakkomajuri temporarily deployed to posthog-pr-6230 October 11, 2021 09:10 Inactive

handle now timestamp

882ff82

yakkomajuri temporarily deployed to posthog-pr-6230 October 11, 2021 10:02 Inactive

remove test code

cb0172f

yakkomajuri temporarily deployed to posthog-pr-6230 October 11, 2021 10:06 Inactive

yakkomajuri merged commit 3ec5cc0 into master Oct 11, 2021

yakkomajuri deleted the capture-refactor-plus-dlq branch October 11, 2021 10:38

yakkomajuri mentioned this pull request Oct 13, 2021

Sprint 1.30.0 1/2 - Oct 18 to Oct 29 #6360

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Capture Endpoint Refactor #6230

Capture Endpoint Refactor #6230

yakkomajuri commented Oct 4, 2021 •

edited

yakkomajuri Oct 4, 2021

codecov-commenter commented Oct 4, 2021 •

edited

guidoiaquinti Oct 5, 2021

yakkomajuri Oct 5, 2021 •

edited

yakkomajuri Oct 5, 2021

yakkomajuri Oct 5, 2021

yakkomajuri commented Oct 5, 2021

tiina303 left a comment

yakkomajuri commented Oct 7, 2021

yakkomajuri commented Oct 7, 2021

yakkomajuri commented Oct 7, 2021

tiina303 left a comment

yakkomajuri commented Oct 8, 2021

yakkomajuri commented Oct 8, 2021

tiina303 left a comment

tiina303 Oct 8, 2021

tiina303 commented Oct 8, 2021

		if not is_clickhouse_enabled():
		error_response = cors_response(

Capture Endpoint Refactor #6230

Capture Endpoint Refactor #6230

Conversation

yakkomajuri commented Oct 4, 2021 • edited

Important

Changes

How did you test this code?

yakkomajuri Oct 4, 2021

Choose a reason for hiding this comment

codecov-commenter commented Oct 4, 2021 • edited

Codecov Report

guidoiaquinti Oct 5, 2021

Choose a reason for hiding this comment

yakkomajuri Oct 5, 2021 • edited

Choose a reason for hiding this comment

yakkomajuri Oct 5, 2021

Choose a reason for hiding this comment

yakkomajuri Oct 5, 2021

Choose a reason for hiding this comment

yakkomajuri commented Oct 5, 2021

tiina303 left a comment

Choose a reason for hiding this comment

yakkomajuri commented Oct 7, 2021

yakkomajuri commented Oct 7, 2021

yakkomajuri commented Oct 7, 2021

tiina303 left a comment

Choose a reason for hiding this comment

yakkomajuri commented Oct 8, 2021

yakkomajuri commented Oct 8, 2021

tiina303 left a comment

Choose a reason for hiding this comment

tiina303 Oct 8, 2021

Choose a reason for hiding this comment

tiina303 commented Oct 8, 2021

yakkomajuri commented Oct 4, 2021 •

edited

codecov-commenter commented Oct 4, 2021 •

edited

yakkomajuri Oct 5, 2021 •

edited