feat(spans): Equalize name and description for manual spans#6070
Conversation
| Some(name_for_op_and_attributes(op, &AttributeGetter(attributes))) | ||
| } | ||
|
|
||
| fn name_for_origin_and_description( |
There was a problem hiding this comment.
| fn name_for_origin_and_description( | |
| fn name_from_origin_and_description( |
| }; | ||
|
|
||
| process::scrub(&mut spans, ctx); | ||
| process::backfill_description(&mut spans); |
There was a problem hiding this comment.
Maybe process::normalize_derived or something similar?
| .value() | ||
| .is_some_and(|attrs| attrs.contains_key(SENTRY__DESCRIPTION)) |
There was a problem hiding this comment.
This might need a check which doesn't just use value() but also considers meta, Empty trait might be able to do that for you.
There was a problem hiding this comment.
You mean check if attrs contains SENTRY__DESCRIPTION with a nonempty value?
| if attributes | ||
| .get_value(SENTRY__ORIGIN) | ||
| .and_then(|o| o.as_str()) | ||
| == Some("manual") |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9a7ba1a. Configure here.
Why do you think it is not an improvement? |
I mean, I believe it is in terms of correctness. But it does complicate the code a fair bit. Feels like we may be missing an abstraction for combining |
| if let Some(Annotated(Some(Value::String(db_query)), meta)) = | ||
| attributes.get_annotated_value(DB__QUERY__TEXT) | ||
| { | ||
| return Some(Annotated(Some(db_query.clone()), meta.clone())); | ||
| } | ||
|
|
||
| if let Some(Annotated(Some(Value::String(method)), method_meta)) = | ||
| attributes.get_annotated_value(HTTP__REQUEST__METHOD) | ||
| && let Some(Annotated(Some(Value::String(url)), url_meta)) = | ||
| attributes.get_annotated_value(URL__FULL) | ||
| { | ||
| let description = format!("{method} {url}"); | ||
| let meta = method_meta.clone().merge(url_meta.clone()); | ||
| return Some(Annotated(Some(description), meta)); | ||
| } |
There was a problem hiding this comment.
Instead of actually merging the meta, I think we're fine if we just don't. We've historically never marked data which we added, as that is non-destructive and generally obvious. User/SDK doesn't set it, must be added by the Server.
We could think about adding metadata from which fields we synthesized the description if we wanted, but I am not entirely sure if that's useful.
9a7ba1a to
14ab996
Compare
| if let Some(&Value::String(db_query)) = attributes.get_value(DB__QUERY__TEXT).as_ref() { | ||
| return Some(db_query.clone()); | ||
| } |
There was a problem hiding this comment.
Bug: Moving sentry.description population to after PII scrubbing bypasses custom scrubbing rules for that field, potentially leaking sensitive data from DB queries or URLs.
Severity: HIGH
Suggested Fix
Ensure that derived span descriptions are generated before the PII scrubbing process runs. This would involve calling normalize_derived() before process::scrub(), restoring the previous behavior where custom PII rules targeting sentry.description can effectively remove sensitive information.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: relay-spans/src/description.rs#L27-L29
Potential issue: For non-manual V2 spans, such as database or HTTP spans, the
`sentry.description` attribute is now populated after PII scrubbing has already run.
Previously, this attribute was populated before scrubbing, allowing custom PII rules
targeting `sentry.description` to remove sensitive data like database query text or URL
parameters. Because the attribute is now absent during the scrubbing process, these
custom rules are no longer effective. This results in potentially sensitive information,
which was previously scrubbed, being included in the `sentry.description` field.

This ensures that for spans (V1 or V2) with an origin of
"manual", name and description coincide. For V1 spans, this means copying the description to the name, and the reverse for V2 spans. Other spans behave as before.sentry-conventions.backfill_descriptionthat runs right after PII scrubbing. Unfortunately it can't simply be integrated intonormalize_spanbecause it's not safe to backfill the description before PII scrubbing—the description could end up containing a URL with PII, for example. Also, in the course of this, we move the (limited) functionality that existed for backfilling the description based on attributes out ofwrite_legacy_attributes(where it was somewhat misplaced anyway) and centralize it innormalize_sentry_description.Future work:
sentry-conventionsCloses INGEST-955.