Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(relay): Add span links to event schema #4486

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Feb 5, 2025

This PR adds span link definitions as spec'd out in RFC #141 to the relay-event-schema crate. As described in the envelope item payload section, span links can be added by SDKs in

  • event.contexts.trace.links for links of the root span/transaction
  • event.spans[i].links for links of child spans

This PR also adds integration tests for events with span links.

@reviewers, this is my first PR to Relay, so please let me know if I'm missing something or you want to see something else tested. Happy to make changes!

Open Questions

This PR does not include any kind of validation logic for passed attributes or links in general. Frankly, I'm a bit unsure what our approach for validation is in Relay.

  • Given that Relay today already forwards links prior to this PR, should it drop events with malformed links? or remove such links from the event? I don't have strong opinions but would simply follow best practices or recommendations.
  • As for validating link attributes: These can be arbitrary key/value pairs, just like span attributes. The only attribute we expect to be of a certain type (string) would be the sentry.link.type attribute. Is this something we can/should enforce in Relay or handle further down the line?

Happy to make further changes for this but it probably makes sense to tackle them in a follow-up PR.

ref #4443

Comment on lines +744 to +746
/// Span link attributes, similar to span attributes/data
#[metastructure(pii = "maybe", trim = false)]
pub attributes: Annotated<Object<Value>>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this the correct way to let Relay know that this field can contain PII and should be scrubbed?

Copy link
Member

Choose a reason for hiding this comment

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

To apply default scrubbing rules (not just when the user explicitly configures it through advanced rules), set pii = "true".

@Lms24 Lms24 marked this pull request as ready for review February 5, 2025 14:51
@Lms24 Lms24 requested a review from a team as a code owner February 5, 2025 14:51
@Lms24 Lms24 requested a review from jjbayer February 5, 2025 14:52
@@ -10,6 +10,7 @@
- Add flags context to event schema. ([#4458](https://github.com/getsentry/relay/pull/4458))
- Add support for view hierarchy attachment scrubbing. ([#4452](https://github.com/getsentry/relay/pull/4452))
- Allow configuration of Relay's log format via an environment variable. ([#4484](https://github.com/getsentry/relay/pull/4484))
- Add span links to event schema ([#4486](https://github.com/getsentry/relay/pull/4486))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Add span links to event schema ([#4486](https://github.com/getsentry/relay/pull/4486))
- Add span links to event schema. ([#4486](https://github.com/getsentry/relay/pull/4486))

@@ -138,6 +138,10 @@ pub struct TraceContext {
#[metastructure(pii = "maybe", skip_serialization = "null")]
pub data: Annotated<SpanData>,

/// Links to other spans from the trace's root span.
#[metastructure(pii = "maybe", skip_serialization = "null")]
Copy link
Member

Choose a reason for hiding this comment

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

pii = "maybe" means it will only be scrubbed with Advanced Data Scrubbing Rules. Are there any link attributes that we expect to contain PII?

Comment on lines +744 to +746
/// Span link attributes, similar to span attributes/data
#[metastructure(pii = "maybe", trim = false)]
pub attributes: Annotated<Object<Value>>,
Copy link
Member

Choose a reason for hiding this comment

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

To apply default scrubbing rules (not just when the user explicitly configures it through advanced rules), set pii = "true".


/// Span link attributes, similar to span attributes/data
#[metastructure(pii = "maybe", trim = false)]
pub attributes: Annotated<Object<Value>>,
Copy link
Member

Choose a reason for hiding this comment

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

If you want to validate the sentry.link.type attribute, you can define attributes as a type SpanLinkAttributes that contains

struct SpanLinkAttributes {
    #[metastructure(field = "sentry.link.type", ...)]
    pub link_type: Annotated<LinkType>, // `LinkType` being an enum
    #[metastructure(additional_properties, retain = true, ...)]
    pub other: Object<Value>,
}

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