fix(envelope) Add support for implicitly sized envelope items #1229
Conversation
RaduW
force-pushed
the
fix/implicit-size-envelope-items
branch
from
21dbd24
to
fa9e2c1
Compare
There was a problem hiding this comment.
@RaduW would you mind explaining in the description the motivation for this feature? The "what" is clear, but not the "why" we should add this feature. I know it is (an optional) part of the protocol, but why are we adding it to the Python SDK now?
| else: | ||
| # if no length was specified we need to read up to the end of line | ||
| payload = f.readline().rstrip() | ||
| if headers.get("type") in ("event", "transaction", "metric_buckets"): |
There was a problem hiding this comment.
"metric_buckets" is this addition intentional? If so, please let's document it at least in the PR description as it seems unrelated to the title.
There was a problem hiding this comment.
Yes, sorry sneaked it in since I bumped into the problem while testing metrics.
"metric_buckets" is the new item type that contains metrics... will detail in description.
sentry_sdk/envelope.py
Outdated
| f.readline() | ||
| else: | ||
| # if no length was specified we need to read up to the end of line | ||
| payload = f.readline().rstrip() |
There was a problem hiding this comment.
Aware that we didn't check the value of length before (we probably should), I think here we should be more defensive against reading too much data into memory. There should be an upper limit, because we know that above that envelopes won't make it through and can potentially bring apps down.
sentry_sdk/envelope.py
Outdated
| payload = f.read(length) | ||
| if headers.get("type") in ("event", "transaction"): | ||
| rv = cls(headers=headers, payload=PayloadRef(json=parse_json(payload))) | ||
| headers = json.loads(line) |
There was a problem hiding this comment.
parse_json seems to be a Python 2/3 compatibility helper, why replace its use with json.loads?
There was a problem hiding this comment.
hmm, don't even remember doing it (I guess, just from habit), will revert to parse_json.
tests/test_envelope.py
Outdated
| '{"event_id":"9ec79c33ec9942ab8353589fcb2e04dc"}\n' | ||
| + '{"type":"type1"}\n1234\n' | ||
| + '{"type":"type2"}\nabcd\n' | ||
| + '{"type":"type3"}\n\n' |
There was a problem hiding this comment.
Could we also add a case for when a \n is omitted as in "Envelope with 2 empty attachments, last newline omitted" in https://develop.sentry.dev/sdk/envelopes/
There was a problem hiding this comment.
I can do this, but this should be added to the explicitly specified length, test.
If we do aggre to support implicitly specified lengths I will extend the tests to cover all the cases I can think of.
This PR adds support for implicitly sized envelope items, that is envelope items that do not contain the size in the
item header and the payload is inferred by the first
\ncharacter.The support for implicitly sized envelopes is added so that the parsing of envelopes is compliant with the Envelope specification. Envelope Items
Additionally this PR adds the type
metric_bucketsas a recognised JSON item type ( it contains metrics).