ref(spans): JSON Kafka message with metadata #2556

jjbayer · 2023-09-29T14:38:29Z

This PR makes the following changes to the relay -> sentry kafka schema for spans:

[breaking change] Encode the payload as JSON, not msgpack. Msgpack should only be used for payloads that contain (partial) binary data.
Add the fields organization_id and retention_days so the consumer does not have to look them up.
[through ref(spans): Write sentry tags to dedicated field #2555] New field span.sentry_tags can be used instead of span.data. It now contains the same keys as expected by snuba. The values are guaranteed to be strings.
Some extra validation to make sure we do not send corrupted data.

Sentry counterpart: getsentry/sentry#57284

relay-server/src/actors/store.rs

phacops

Can you add profile_id as well? Only filled if the profile context contains a profile_id key. Should be filled for every spans generated by the transaction (and the segment as well).

relay-server/src/actors/store.rs

phacops

I noticed some values missing from before or which could be added to help:

description
start_timestamp_ms

Is group in the sentry_tags guaranteed to be a good value (convertible to an hex value)?

And regarding tags and sentry_tags, can we make sure we don't add a None value?

jjbayer · 2023-10-03T14:01:01Z

relay-server/src/actors/processor.rs

+        exclusive_time
+            .value()
+            .ok_or(anyhow::anyhow!("missing exclusive_time"))?;
+
+        if let Some(sentry_tags) = sentry_tags.value_mut() {
+            sentry_tags.retain(|key, value| match value.value() {
+                Some(s) => {
+                    if key == "group" {
+                        // Only allow 16-char hex strings in group.
+                        s.len() == 16 && s.chars().all(|c| c.is_ascii_hexdigit())
+                    } else {
+                        true
+                    }
+                }
+                // Drop empty string values.
+                None => false,
+            });
+        }
+        if let Some(tags) = tags.value_mut() {
+            tags.retain(|_, value| !value.is_empty())
+        }


@phacops this should give some additional guarantees about the data.

If tags is not None but it's empty, is it forwarded as an empty object?

Probably. The consumer should be able to handle that though.

olksdr

Please, fix the changelog. Otherwise lgtm

olksdr · 2023-10-03T14:12:41Z

CHANGELOG.md

- Exclude more spans fron metrics extraction. ([#2522](https://github.com/getsentry/relay/pull/2522), [#2525](https://github.com/getsentry/relay/pull/2525), [#2545](https://github.com/getsentry/relay/pull/2545))
+pull/2522), [#2525](https://github.com/getsentry/relay/pull/2525), [#2545](https://github.com/getsentry/relay/pull/2545))


Something strange here, looks like it got broken, maybe on master merge?

👍 fixed now

iker-barriocanal

Is a new release of the sentry kafka schemas library required before deploying this?

iker-barriocanal · 2023-10-03T14:21:32Z

relay-server/src/actors/processor.rs

+                        // Only allow 16-char hex strings in group.
+                        s.len() == 16 && s.chars().all(|c| c.is_ascii_hexdigit())


Q: why not longer/shorter strings?

Yes, I think this is a mistake. We are converting from an hex string into a u64 but the length of the hex string can be 8 I believe.

The group hash is a 64-bit hash, i.e. 8 bytes, but it takes two hex characters to encode a byte. So the group hash must always be 16 characters long. I'm fine with relaxing the condition to <= 16 though.

relay/relay-event-normalization/src/normalize/span/tag_extraction.rs

Lines 339 to 341 in e27eff0

let mut span_group = format!("{:?}", md5::compute(scrubbed_desc));

span_group.truncate(16);

span_tags.insert(SpanTagKey::Group, span_group);

String::len returns bytes, so 16 bytes * 2 chars / byte = 32 chars?

16 is the length of the hexidecimal representation, e.g. 0123456789abcdef. But it still only encodes 8 bytes of data: 01 23 45 67 89 ab cd ef.

relay-server/src/actors/store.rs

iker-barriocanal · 2023-10-03T14:29:53Z

tests/integration/fixtures/processing.py

+    def get_span(self):
+        message = self.poll()


Should we use the timeout here? Calling the spans_consumer with a timeout and it not being used may generate some confusion.

See

relay/tests/integration/fixtures/processing.py

Lines 160 to 162 in 415e5cb

def poll(self, timeout=None):

if timeout is None:

timeout = self.timeout

phacops

Is there still a possibility for status and op to be in sentry_tags and at the span top-level and have a different value? Would be good to clean this up and have only 1 value, wherever it's easier to read from.

phacops · 2023-10-03T16:47:18Z

relay-server/src/actors/processor.rs

+                        // Only allow 16-char hex strings in group.
+                        s.len() == 16 && s.chars().all(|c| c.is_ascii_hexdigit())


Yes, I think this is a mistake. We are converting from an hex string into a u64 but the length of the hex string can be 8 I believe.

phacops · 2023-10-03T16:52:27Z

Is a new release of the sentry kafka schemas library required before deploying this?

No, we haven't written one yet for ingest-spans. When this schema is stabilized (after this PR), I'll write one and start using it in the Sentry consumer.

phacops · 2023-10-03T19:56:59Z

Can you also validate span.status_code is a valid integer even if we're going to send it as a string?

jjbayer · 2023-10-04T11:31:48Z

@phacops

Can you also validate span.status_code is a valid integer even if we're going to send it as a string?

Done.

Is there still a possibility for status and op to be in sentry_tags and at the span top-level and have a different value? Would be good to clean this up and have only 1 value, wherever it's easier to read from.

Removed status from sentry_tags now because it's an exact copy of the top-level status. Keeping op for now because it does a lowercase transform:

relay/relay-event-normalization/src/normalize/span/tag_extraction.rs

Lines 235 to 238 in b45dce8

    
           if let Some(unsanitized_span_op) = span.op.value() { 
        
               let span_op = unsanitized_span_op.to_owned().to_lowercase(); 
        
               span_tags.insert(SpanTagKey::SpanOp, span_op.to_owned());

jjbayer added 7 commits September 28, 2023 13:54

wip

93ad857

schema

86d6730

wip

4f68528

ref(spans): Write sentry tags to dedicated field

fc7e8fd

ref(spans): Make sentry_tags a string mapping

65b2bf8

Merge branch 'ref/span-sentry-tags' into ref/spans-kafka-schema

cabdf31

update test

e1ee592

jjbayer commented Sep 29, 2023

View reviewed changes

relay-server/src/actors/store.rs Show resolved Hide resolved

jjbayer mentioned this pull request Sep 29, 2023

ref(spans): Write sentry tags to dedicated field #2555

Merged

phacops reviewed Sep 29, 2023

View reviewed changes

relay-server/src/actors/store.rs Outdated Show resolved Hide resolved

relay-server/src/actors/store.rs Outdated Show resolved Hide resolved

relay-server/src/actors/store.rs Outdated Show resolved Hide resolved

jjbayer added 3 commits October 2, 2023 14:00

double-write

ff377a3

Merge remote-tracking branch 'origin/master' into ref/span-sentry-tags

1b3b5e0

remove unused function

63b91ed

phacops reviewed Oct 2, 2023

View reviewed changes

fix: Also for transaction span

0c9c141

phacops mentioned this pull request Oct 2, 2023

feat(spans): Handle span schema v1 from Relay getsentry/sentry#57284

Merged

jjbayer added 7 commits October 3, 2023 10:11

ref: Use short tag names

cadab70

tests

bcc3aa4

Merge branch 'ref/span-sentry-tags' into ref/spans-kafka-schema

1f11d03

ref: Validate on extraction

1e08222

changelog

74b281d

simplify

a7429d6

Wrap raw value

a7256ae

jjbayer changed the title ~~ref(spans): Kafka spans schema V1~~ ref(spans): JSON Kafka message with metadata Oct 3, 2023

jjbayer added 4 commits October 3, 2023 14:23

Remove outdated comment, changelog

da8785e

Merge branch 'ref/span-sentry-tags' into ref/spans-kafka-schema

78bef44

fix non-processing build

4962ab8

extra validation

e27eff0

jjbayer commented Oct 3, 2023

View reviewed changes

jjbayer marked this pull request as ready for review October 3, 2023 14:01

jjbayer requested review from a team and phacops October 3, 2023 14:01

olksdr approved these changes Oct 3, 2023

View reviewed changes

iker-barriocanal approved these changes Oct 3, 2023

View reviewed changes

phacops reviewed Oct 3, 2023

View reviewed changes

Base automatically changed from ref/span-sentry-tags to master October 4, 2023 10:17

jjbayer added 4 commits October 4, 2023 13:07

fix

fa61e6f

validate status code

6e027d2

Merge remote-tracking branch 'origin/master' into ref/spans-kafka-schema

b45dce8

ref: Remove span.status from sentry_tags

26bd175

phacops approved these changes Oct 4, 2023

View reviewed changes

lint

6d66741

jjbayer merged commit 600efba into master Oct 4, 2023

jjbayer deleted the ref/spans-kafka-schema branch October 4, 2023 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(spans): JSON Kafka message with metadata #2556

ref(spans): JSON Kafka message with metadata #2556

jjbayer commented Sep 29, 2023 •

edited

Loading

phacops left a comment

phacops left a comment •

edited

Loading

jjbayer Oct 3, 2023

iker-barriocanal Oct 3, 2023

jjbayer Oct 4, 2023

olksdr left a comment

olksdr Oct 3, 2023

jjbayer Oct 4, 2023

iker-barriocanal left a comment

iker-barriocanal Oct 3, 2023

phacops Oct 3, 2023

jjbayer Oct 4, 2023

iker-barriocanal Oct 4, 2023

jjbayer Oct 4, 2023

iker-barriocanal Oct 3, 2023

jjbayer Oct 4, 2023

phacops left a comment

phacops Oct 3, 2023

phacops commented Oct 3, 2023

phacops commented Oct 3, 2023

jjbayer commented Oct 4, 2023

		- Exclude more spans fron metrics extraction. ([#2522](https://github.com/getsentry/relay/pull/2522), [#2525](https://github.com/getsentry/relay/pull/2525), [#2545](https://github.com/getsentry/relay/pull/2545))
		pull/2522), [#2525](https://github.com/getsentry/relay/pull/2525), [#2545](https://github.com/getsentry/relay/pull/2545))

		// Only allow 16-char hex strings in group.
		s.len() == 16 && s.chars().all(\|c\| c.is_ascii_hexdigit())

	let mut span_group = format!("{:?}", md5::compute(scrubbed_desc));
	span_group.truncate(16);
	span_tags.insert(SpanTagKey::Group, span_group);

	def poll(self, timeout=None):
	if timeout is None:
	timeout = self.timeout

ref(spans): JSON Kafka message with metadata #2556

ref(spans): JSON Kafka message with metadata #2556

Conversation

jjbayer commented Sep 29, 2023 • edited Loading

phacops left a comment

Choose a reason for hiding this comment

phacops left a comment • 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

olksdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iker-barriocanal left a 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

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

phacops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phacops commented Oct 3, 2023

phacops commented Oct 3, 2023

jjbayer commented Oct 4, 2023

jjbayer commented Sep 29, 2023 •

edited

Loading

phacops left a comment •

edited

Loading