feat(chain): add BatchParser #134
Conversation
| class FakeCodec: | ||
| def decode(self, payload: bytes, _: Any) -> Mapping[str, int]: | ||
| if payload == b'{"a": 1}': | ||
| return {"a": 1} | ||
| else: | ||
| return {"b": 2} |
There was a problem hiding this comment.
instead of returning a fake codec here i would suggest to use Codec class by kafka schemas directly, just to exercise more code. if there is a breaking change in sentry-kafka-schemas (intentional or not), we're more likely to catch it with this test
same for FakeMessage, it seems feasible to me to create a real message?
There was a problem hiding this comment.
used real classes
| monkeypatch.setattr( | ||
| "sentry_streams.pipeline.msg_parser._get_codec_from_msg", | ||
| lambda _: JsonCodec(json_schema={}), | ||
| ) |
There was a problem hiding this comment.
Why do you need to monkeypatch? Please consider using a real schema like ingest-metrics.
While, in unit tests, you want to keep your test restricted in scope and fast, mocking the schema does not buy you anything. There is no dependency on external systems.
There was a problem hiding this comment.
if we remove the schema or even just alter it, we'll have to change this. i personally regret using real schemas/datasets in some unit tests in snuba, as i changed/removed some schema and suddenly have to change random irrelevant tests that fully rely on it.
There was a problem hiding this comment.
i can hardcode a real schema instead of importing it, then the tests won't be affected if the schema changes. But that doesn't make a difference than using a fake schema imo since they're both hardcoded
There was a problem hiding this comment.
done ^
There was a problem hiding this comment.
if we remove the schema or even just alter it, we'll have to change this. i personally regret using real schemas/datasets in some unit tests in snuba, as i changed/removed some schema and suddenly have to change random irrelevant tests that fully rely on it.
Don't worry sentry-kafka-schemas will not be there for long, we need to decouple that repo from this one (even the licenses are incompatible). So feel free to use a real schema.
There was a problem hiding this comment.
@victoria-yining-huang my issue is having to hijack the behavior of the parser by monkeypatching the codec, not the fact that the schema is the real one or not. Any schema is fine, I'd try to avoid monkeypatching because it makes the test cover less code.
There was a problem hiding this comment.
i used real example and schema imported from sentry_kafka_schema lib for the test now.
There was a problem hiding this comment.
removed monkeypatch
There was a problem hiding this comment.
added test cases for no shema or wrong shema. This covers _get_codec_from_msg function
victoria-yining-huang
force-pushed
the
vic/add_sbc_parser
branch
from
d674ad7 to
e0337ac
Compare
add all draft remove prints frmt move exception block frmt typing typing input output type typing add generic tout push for riya type fixes remove comment add unit test add unit test use real classes dont cast fix module path hardcode a real codec use real things from sentry kafka schema
victoria-yining-huang
force-pushed
the
vic/add_sbc_parser
branch
from
453763d to
6bda28f
Compare
|
@evanh addressed the question "Why does the BatchParser output another batch" in weekly meeting. The design choice for batching is batch in batch out, batches stay intact throughout every single step re "Should it use a FlatMap instead?" I know currently FlatMap is not fully implemented yet ( streams/sentry_streams/sentry_streams/adapters/arroyo/rust_arroyo.py Lines 238 to 242 in 68500b1 |
No The Batch parser is just syntactic sugar over a Batch -> Custom map. |
this
BatchParseris to be used after aBatchstep, not to be confused with theParserstep which is to be used before aBatchstep.also updated the example in
Batching.pyto use thisBatchParser, tested successfully locally