Skip to content

feat(spans): Align otel attributes with sentry span #3457

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

Merged
merged 28 commits into from
Apr 22, 2024

Conversation

jjbayer
Copy link
Member

@jjbayer jjbayer commented Apr 18, 2024

To simplify conversion in the future, align attribute names in Otel's span attributes with keys in Sentry's span.data. The old keys in span.data remain as legacy alias.

See also getsentry/develop#1230 for the span schema / attribute convention.

ref: #3278

@jjbayer jjbayer force-pushed the feat/spans-otel-test branch from 0dedad1 to 980b6d3 Compare April 19, 2024 09:03
pub replay_id: Annotated<Value>,

#[metastructure(field = "sdk.name")]
#[metastructure(field = "sentry.sdk.name")]
Copy link
Member Author

Choose a reason for hiding this comment

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

No legacy alias need here, this field was only introduced in #3456.

@@ -1357,6 +1357,7 @@ struct SpanKafkaMessage<'a> {
event_id: Option<EventId>,
#[serde(rename(deserialize = "exclusive_time"))]
exclusive_time_ms: f64,
#[serde(default)]
is_segment: bool,
Copy link
Member Author

Choose a reason for hiding this comment

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

When we cannot derive is_segment, is should be set to false.

Comment on lines -7 to -32
("enduser.id", "user.id"),
("http.request.cookies", "request.cookies"),
("http.request.env", "request.env"),
(
"http.request.headers.content-type",
"request.headers.content-type",
),
("http.request.method", "request.method"),
("sentry.environment", "environment"),
("sentry.origin", "origin"),
("sentry.release", "release"),
("sentry.sample_rate", "sample_rate"),
("sentry.sdk.integrations", "sdk.integrations"),
("sentry.sdk.name", "sdk.name"),
("sentry.sdk.packages", "sdk.packages"),
("sentry.sdk.version", "sdk.version"),
("sentry.source", "source"),
("sentry.user.email", "user.email"),
("sentry.user.geo.city", "user.geo.city"),
("sentry.user.geo.country_code", "user.geo.country_code"),
("sentry.user.geo.region", "user.geo.region"),
("sentry.user.ip_address", "user.ip_address"),
("sentry.user.segment", "user.segment"),
("sentry.user.username", "user.username"),
("url.full", "request.url"),
("url.query_string", "request.query_string"),
Copy link
Member Author

Choose a reason for hiding this comment

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

@phacops do you know if any of these mappings was already used for product features? If not, I'd rather remove the mapping and start fresh.

@@ -200,11 +200,11 @@ pub struct SpanData {
pub db_system: Annotated<Value>,

/// The sentry environment.
#[metastructure(field = "environment")]
#[metastructure(field = "sentry.environment", legacy_alias = "environment")]
Copy link
Member Author

Choose a reason for hiding this comment

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

The disadvantage of this rename is that the new key will now be serialized into the payload. But span.data is not (yet) forwarded to sentry directly on standalone spans (we only extract some of it into sentry_tags, but that happens in relay). Spans as part of transactions are materialized into nodestore, but they don't rely on these particular span.data items AFAIK.

Comment on lines -172 to -174
// TODO: This is wrong, a segment could still have a parent in the trace.
let is_segment = parent_span_id.is_none().into();

Copy link
Member Author

Choose a reason for hiding this comment

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

Relying on set_segment_attributes instead.

if let Some(statement) = otel_value_to_string(value) {
description = statement;
}
match attribute.key.as_str() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the large diff, I converted this if to a match expression.

Copy link
Member

Choose a reason for hiding this comment

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

Large diffs that clean up code make me happy.

let otel_span: OtelSpan = serde_json::from_str(json).unwrap();
let span_from_otel = otel_to_sentry_span(otel_span);

// TODO: measurements, metrics_summary
Copy link
Member Author

Choose a reason for hiding this comment

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

This will need to be handled in a follow-up.

@jjbayer jjbayer marked this pull request as ready for review April 19, 2024 09:19
@jjbayer jjbayer requested a review from a team as a code owner April 19, 2024 09:19
if let Some(statement) = otel_value_to_string(value) {
description = statement;
}
match attribute.key.as_str() {
Copy link
Member

Choose a reason for hiding this comment

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

Large diffs that clean up code make me happy.

Base automatically changed from feat/spans-more-fields-2 to master April 22, 2024 08:40
pub replay_id: Annotated<Value>,

/// The sentry SDK (see [`crate::protocol::ClientSdkInfo`]).
#[metastructure(field = "sdk.name")]
#[metastructure(field = "sentry.sdk.name")]
Copy link
Member Author

Choose a reason for hiding this comment

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

No need for an alias here, this field was only introduced in #3456.

@jjbayer jjbayer enabled auto-merge (squash) April 22, 2024 13:34
@jjbayer jjbayer merged commit 69874ae into master Apr 22, 2024
@jjbayer jjbayer deleted the feat/spans-otel-test branch April 22, 2024 13:57
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