feat(ourlogs): Adjust 'log' protocol per sdk feedback #4592
Conversation
k-fish
force-pushed
the
feat/ourlogs/adjust-log-item-type
branch
3 times, most recently
from
4dfab55 to
94acbee
Compare
Now that we've mostly finalized the logs protocol with sdks (see develop doc for more info), we want to update Relay to allow the 'log' item type to be sent in this format. Also in this PR is some shimming between the updated protocol / event schema and the kafka consumers. This shimming is temporary until the generic EAP items consumers are ready, at which point we'll have to transform the event-schema Relay accepts into the generic eap item kafka.
k-fish
force-pushed
the
feat/ourlogs/adjust-log-item-type
branch
from
94acbee to
e551796
Compare
Co-authored-by: David Herberth <[email protected]>
k-fish
force-pushed
the
feat/ourlogs/adjust-log-item-type
branch
from
418ea5f to
f133144
Compare
| @@ -46,11 +37,54 @@ pub struct OurLog { | |||
| #[metastructure(pii = "true", trim = false)] | |||
| pub attributes: Annotated<Object<AttributeValue>>, | |||
There was a problem hiding this comment.
Do we want to keep this Otel Like format? Could just be a self describing mapping?
"attributes": {
"boolean.attribute": {
"bool_value": true
},Could just be:
"attributes": {
"boolean.attribute": true,There was a problem hiding this comment.
I asked for sdk folks to discuss in the develop PR, so we'll see.
There was a problem hiding this comment.
tests/integration/test_ourlogs.py
Outdated
| "attributes": { | ||
| "boolean.attribute": {"bool_value": True}, | ||
| "sentry.severity_number": {"int_value": 9}, | ||
| "sentry.severity_text": {"string_value": "info"}, | ||
| }, | ||
| } | ||
| expected_with_missing_fields = { | ||
|
|
||
| del ourlogs[0]["received"] |
There was a problem hiding this comment.
We have a test helper which can fuzzy assert the time, time_within{_delta}:
relay/tests/integration/test_profile_chunks.py
Lines 137 to 149 in bb89697
Then you don't have to delete the time.
|
|
||
| ourlogs = ourlogs_consumer.get_ourlogs() | ||
| assert len(ourlogs) == 2 | ||
| expected_with_all_fields = { | ||
| assert len(ourlogs) == 1 |
There was a problem hiding this comment.
Can just assert ourlogs = [expected] instead of asserting length and first element.
| assert ourlogs[1] == expected_with_missing_fields | ||
|
|
||
| ourlogs_consumer.assert_empty() | ||
| assert ourlogs[0] == expected |
There was a problem hiding this comment.
Same here, assert ourlogs = [expected] and you don't even have to assert the length.
| } | ||
|
|
||
| del ourlogs[0]["received"] |
There was a problem hiding this comment.
Also here can use the time_within_delta helper (as it's already used for the timestamp_nanos)
Co-authored-by: David Herberth <[email protected]>
Summary
Now that we've mostly finalized the logs protocol with sdks (see develop doc for more info), we want to update Relay to allow the 'log' item type to be sent in this format.
SDK's will likely primarily use the
logItemType instead ofotel_logsince we don't need to change timestamp conventions, can send a simplifiedlevel(seeOurLogLeveladded in this PR).Schema changes
We've deprecated some fields in the protocol that only exist for OTEL:
severity_numberandseverity_text, we're coercing these tolevel, but we're keeping the original severity text and number as attributes as OTel allows custom severity text.observed_timestamp_nanosis always set by relay regardless of what is sent because Relay is the 'collector'. We have to leave this as an attribute as well since it's being used by the existing consumer for origin timestamp.timestamp_nanosbecomestimestamp: Timestamptrace_flags, this is unused, and the consumer doesn't even store it in the table. Will decide what to do with this later.Future work
ourlog_merge_otelfunction can be trimmed down since we won't need to fill in deprecated fields to send the same data to the kafka consumer.OurLogprotocol from json received from sdks to a generic EAP "trace items" kafka message that is essentially a couple fields (eg. traceid) + a KVMap forattributes.