Skip to content

Conversation

loewenheim
Copy link
Contributor

@loewenheim loewenheim commented May 26, 2025

This implements the conversion of Span V2 items/item containers in envelopes to Span V1. At the start of span::processing::process, if there is a Span V2 container item, it is removed and replaced with individual items for the converted spans. The process function now returns a Result because it rejects envelopes containing more than one Span V2 container (in line with the design; this may be lifted in future).

ref: RELAY-64

@loewenheim loewenheim requested a review from Dav1dde May 26, 2025 11:10
@loewenheim
Copy link
Contributor Author

After discussion with @Dav1dde it's clear that the conversion/expansion needs to be moved—it needs to happen before inbound filters are applied at the start of process_standalone_spans.

@loewenheim loewenheim force-pushed the sebastian/convert-span-v2 branch from edeb437 to b7bb4ae Compare May 27, 2025 12:03
Comment on lines -1124 to +1338
assert [m for m in metrics if ":spans/" in m["name"]] == expected_span_metrics

span_metrics = [m for m in metrics if ":spans/" in m["name"]]

assert len(span_metrics) == len(expected_span_metrics)
for actual, expected in zip(span_metrics, expected_span_metrics):
assert actual == expected
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any objection to me rewriting the assertion this way? Checking the whole list diff at once will drive you insane.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this file, the envelope_with_spans function now adds two V2 envelopes as well. Consequently, a bunch of consumed spans and metrics change slightly.

@loewenheim loewenheim marked this pull request as ready for review May 27, 2025 13:00
@loewenheim loewenheim requested a review from a team as a code owner May 27, 2025 13:00
@@ -51,13 +51,11 @@ use crate::constants::DEFAULT_EVENT_RETENTION;
use crate::extractors::{PartialMeta, RequestMeta};

mod attachment;
#[cfg(feature = "processing")]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need access to ItemContainer for span V2 parsing even outside of processing, so this feature gate needs to go.

@loewenheim
Copy link
Contributor Author

This says #skip-changelog but it occurs to me that this is the first Span V2 related change that might actually deserve an entry, wdyt?

@Dav1dde
Copy link
Member

Dav1dde commented May 27, 2025

This says #skip-changelog but it occurs to me that this is the first Span V2 related change that might actually deserve an entry, wdyt?

Good idea, as this is effectively the first commit which accepts span v2 envelopes. But maybe call out that this is still highly experimental.

@loewenheim loewenheim changed the title feat(spans): Convert V2 spans in envelopes to V1 spans feat(spans): Accept V2 spans and convert them to V1 spans May 28, 2025
@loewenheim
Copy link
Contributor Author

I'll remove all of the outcome/category logic changes for now, they only confuse the issue.

@loewenheim loewenheim force-pushed the sebastian/convert-span-v2 branch from 5d9d461 to d52b219 Compare May 28, 2025 13:28
@loewenheim loewenheim enabled auto-merge (squash) May 30, 2025 10:05
@loewenheim loewenheim merged commit 1a6a4c4 into master May 30, 2025
28 checks passed
@loewenheim loewenheim deleted the sebastian/convert-span-v2 branch May 30, 2025 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants