feat(consumer): Add transactions consumer SLO #4442

ayirr7 · 2023-06-29T16:20:04Z

The received field is (most likely) always present in transaction payloads, but we currently don't do anything with it in the relevant MessageProcessor. This PR allows us to grab the timestamp at which Relay received the transaction and pass it into the SLO calculation, to help us get the ingest transactions SLO.

I think, if we are confident that we are always containing the received field, then we should change the Kafka schema to make received a required field. Need to verify this is the case though.

codecov · 2023-07-04T03:06:21Z

Codecov Report

Patch coverage: 78.76% and project coverage change: +0.03% 🎉

Comparison is base (6518d74) 90.34% compared to head (83ccf6e) 90.38%.
Report is 98 commits behind head on master.

❗ Current head 83ccf6e differs from pull request most recent head 842557c. Consider uploading reports for the commit 842557c to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4442      +/-   ##
==========================================
+ Coverage   90.34%   90.38%   +0.03%     
==========================================
  Files         804      823      +19     
  Lines       39576    40585    +1009     
  Branches      245      289      +44     
==========================================
+ Hits        35755    36681     +926     
- Misses       3789     3861      +72     
- Partials       32       43      +11

Files Changed	Coverage Δ
scripts/check-migrations.py	`0.00% <ø> (ø)`
setup.py	`0.00% <0.00%> (ø)`
snuba/admin/auth_roles.py	`93.22% <ø> (ø)`
snuba/admin/clickhouse/system_queries.py	`70.37% <ø> (ø)`
snuba/admin/static/api_client.tsx	`2.03% <0.00%> (-0.11%)`	⬇️
snuba/admin/static/clickhouse_queries/index.tsx	`30.00% <ø> (+10.64%)`	⬆️
snuba/admin/static/snql_to_sql/index.tsx	`11.90% <ø> (ø)`
snuba/admin/static/table.tsx	`81.81% <ø> (ø)`
snuba/cli/consumer.py	`0.00% <0.00%> (ø)`
snuba/cli/devserver.py	`0.00% <0.00%> (ø)`
... and 106 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lynnagara · 2023-07-07T00:08:08Z

I think you can go ahead and change received in the transactions schema to required. It should fail Sentry / Snuba CI if there are tests where the payload is missing it. In prod, it should be there since Relay is processing events/transactions the same - but even if it's not, it's just a warning.

ayirr7 · 2023-07-07T23:09:41Z

It should fail Sentry / Snuba CI if there are tests where the payload is missing it

There was a test where the sample payload didn't include the received field, and I had to add it for the test to pass. @lynnagara

lynnagara · 2023-07-11T17:50:25Z

snuba/datasets/processors/transactions_processor.py

@@ -472,4 +474,11 @@ def process_message(
 # the following operation modifies the event_dict and is therefore *not* order-independent
 self._process_contexts_and_user(processed, event_dict)

- return InsertBatch([processed], None)
+ if event_dict["data"]["received"] is not None:


In the schema it's defined as a number, and not nullable, so why do we need this check here?

My thought process was that -- this is meant to capture the case where the received field is not passed through (since it's not required in the schema yet). Or, is it implied that the field is required if the field isn't nullable in the schema?

from schema point of view, if it's required and non-nullable, you probably don't need to check it. @lynnagara do you know what will happen if a payload is not schema compatible, will we drop the payload? Do you know what are the processes to ensure the payloads matched the schema?

right now, you'll just get a sentry warning if the schema doesn't match but nothing bad will happen. i'd merge the schema change first to validate that if you're unsure if it's always present.

sounds good. Once the payload is always schema compatible, then we probably don't need this extra check.

hubertsentry · 2023-07-25T19:22:06Z

snuba/datasets/processors/transactions_processor.py

+ assert raw_received is not None
+ received = datetime.utcfromtimestamp(raw_received)
+ return InsertBatch([processed], received)
+ except Exception:


I would only check if it's KeyError, Exception can be wild range of exceptions

except KeyError:

Also, maybe looking into https://docs.python.org/3/tutorial/errors.html#exception-chaining

lynnagara · 2023-07-26T17:25:55Z

snuba/datasets/processors/transactions_processor.py

+ received = datetime.utcfromtimestamp(raw_received)
+ return InsertBatch([processed], received)
+ except KeyError:
+ raise KeyError("Missing received timestamp field in transaction")


Wasn't the point of the try/except to fall back to the old behavior in case we hit the KeyError rather than re-raising it?

Just to be clear: today, what happens if there is a payload missing the received field is that the MessageProcessor will just drop it - is this correct?

So I guess we can fall back to the old behavior (so that we don't drop the message), but we also want to see the error show up in Sentry so we can know if there were any payloads missing the field. What's the right way of getting that error to show up?

I think the point was to log to sentry without throwing the exception, i.e just manually call logger.exception

Alright I can do that then, thanks

lynnagara · 2023-07-26T23:30:02Z

snuba/datasets/processors/transactions_processor.py

+ received = datetime.utcfromtimestamp(raw_received)
+ return InsertBatch([processed], received)
+ except KeyError as err:
+ logger.exception("Missing required field received in payload", err)


Suggested change

logger.exception("Missing required field received in payload", err)

logger.exception(e)

lynnagara · 2023-07-26T23:32:56Z

snuba/datasets/processors/transactions_processor.py

- return InsertBatch([processed], None)
+ try:
+ raw_received = _collapse_uint32(int(event_dict["data"]["received"]))
+ assert raw_received is not None


If received is not there, won't you get an AssertionError that isn't being caught?

Right, but wouldn't the first line (which throws KeyError) be thrown and caught first? So we go into the except clause?

But I guess the AssertionError could be even easier, since it'd be guaranteed that it's being thrown at that line

* add transactions SLO * pass received * fix received typing to datetime * fix test * add received to test event * do try/except * fix typing * except KeyError * add logging * fix try except

ayirr7 added 5 commits June 29, 2023 09:19

add transactions SLO

a9fd9e3

pass received

2cdce7d

fix received typing to datetime

1543898

fix test

d07d4f0

add received to test event

e542463

ayirr7 marked this pull request as ready for review July 7, 2023 00:04

ayirr7 requested a review from a team as a code owner July 7, 2023 00:04

lynnagara reviewed Jul 11, 2023

View reviewed changes

ayirr7 added 2 commits July 25, 2023 13:56

do try/except

264e841

fix typing

182441f

hubertsentry reviewed Jul 25, 2023

View reviewed changes

ayirr7 requested a review from lynnagara July 25, 2023 20:14

except KeyError

bf41325

lynnagara reviewed Jul 26, 2023

View reviewed changes

add logging

5144969

lynnagara reviewed Jul 26, 2023

View reviewed changes

fix try except

842557c

lynnagara approved these changes Jul 27, 2023

View reviewed changes

ayirr7 merged commit ea2e56b into master Jul 28, 2023
24 checks passed

ayirr7 deleted the add-transactions-SLO branch July 28, 2023 19:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(consumer): Add transactions consumer SLO #4442

feat(consumer): Add transactions consumer SLO #4442

ayirr7 commented Jun 29, 2023 •

edited

Loading

codecov bot commented Jul 4, 2023 •

edited

Loading

lynnagara commented Jul 7, 2023

ayirr7 commented Jul 7, 2023

lynnagara Jul 11, 2023

ayirr7 Jul 17, 2023 •

edited

Loading

hubertsentry Jul 17, 2023

lynnagara Jul 17, 2023 •

edited

Loading

hubertsentry Jul 17, 2023

hubertsentry Jul 25, 2023 •

edited

Loading

lynnagara Jul 26, 2023 •

edited

Loading

ayirr7 Jul 26, 2023 •

edited

Loading

lynnagara Jul 26, 2023

ayirr7 Jul 26, 2023

lynnagara Jul 26, 2023

lynnagara Jul 26, 2023

ayirr7 Jul 26, 2023 •

edited

Loading

ayirr7 Jul 26, 2023

	logger.exception("Missing required field received in payload", err)
	logger.exception(e)

feat(consumer): Add transactions consumer SLO #4442

feat(consumer): Add transactions consumer SLO #4442

Conversation

ayirr7 commented Jun 29, 2023 • edited Loading

codecov bot commented Jul 4, 2023 • edited Loading

Codecov Report

lynnagara commented Jul 7, 2023

ayirr7 commented Jul 7, 2023

Choose a reason for hiding this comment

ayirr7 Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lynnagara Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hubertsentry Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

lynnagara Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

ayirr7 Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayirr7 Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayirr7 commented Jun 29, 2023 •

edited

Loading

codecov bot commented Jul 4, 2023 •

edited

Loading

ayirr7 Jul 17, 2023 •

edited

Loading

lynnagara Jul 17, 2023 •

edited

Loading

hubertsentry Jul 25, 2023 •

edited

Loading

lynnagara Jul 26, 2023 •

edited

Loading

ayirr7 Jul 26, 2023 •

edited

Loading

ayirr7 Jul 26, 2023 •

edited

Loading