fix(transport): Use correct data category for transaction events #826
Conversation
|
My current suspicion is that this bug did not go noticed because transaction events were counted as data_category=default while errors were data_category=error. And if the server returns a data_category=transaction when rate-limiting events we'd simply not honor the rate limit anyway, which also "works". |
Co-authored-by: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
untitaker
force-pushed
the
fix/transport-item-types
branch
from
0c13dd6
to
14bbe82
Compare
| @@ -726,3 +727,37 @@ def test_init_string_types(dsn, sentry_init): | |||
| Hub.current.client.dsn | |||
| == "http://894b7d594095440f8dfea9b300e6f572@localhost:8000/2" | |||
| ) | |||
|
|
|||
|
|
|||
| def test_envelope_types(): | |||
There was a problem hiding this comment.
Can we document what behavior this is supposed to test?
tests/conftest.py
Outdated
| @@ -132,8 +132,14 @@ def check_string_keys(map): | |||
| check_string_keys(event) | |||
| validate_event_schema(event) | |||
|
|
|||
| def check_envelope(envelope): | |||
| # Perhaps one day | |||
There was a problem hiding this comment.
A TODO: ... would be more helpful for a future maintainer (ourselves included).
sentry_sdk/transport.py
Outdated
| @@ -208,7 +204,9 @@ def _send_event( | |||
| self, event # type: Event | |||
| ): | |||
| # type: (...) -> None | |||
| if self._check_disabled(get_event_data_category(event)): | |||
| assert event.get("type") != "transaction" | |||
There was a problem hiding this comment.
Suspicious use of assert here. Feels like over reacting to the bug?
It is also weird to assert what the type is NOT, rather than what it should be, judging from the next line, it should be of "error" type. For example, it would be equally a mistake to send a "session" through this method and have a call _check_disabled("error") for a session.
sentry_sdk/transport.py
Outdated
| """This gets invoked with an envelope when a transaction or session should | ||
| be sent to sentry. |
There was a problem hiding this comment.
Suggestion: document what the method does, not who calls it. The latter requires frequent updates when code evolves and gets of sync easily.
To know who calls the method and when we can use "grep" or other tools.
| """This gets invoked with an envelope when a transaction or session should | |
| be sent to sentry. | |
| """Sends an envelope to Sentry. |
We can also document that capture_event is for legacy error events going straight as JSON into the /store endpoint, while capture_envelope can deal with more payload types.
Co-authored-by: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
While working on #825 I found a bug that caused us to use the "default" item type for transaction events. The bug happened because there were mismatched expectations as to what Item.get_event should return if the content is a transaction event.
Will add an explicit test later.