feat(otlp): Infer span description for db spans
#4541
Conversation
| @@ -803,6 +807,14 @@ fn validate(span: &mut Annotated<Span>) -> Result<(), ValidationError> { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| fn infer_span_description(span: &Span) -> Option<String> { | |||
| let category = span.sentry_tags.value()?.category.value()?; | |||
There was a problem hiding this comment.
Should this not use the original place where this tag is extracted from?
There was a problem hiding this comment.
This tag is typically inferred from either the span op or a combination of its attributes, rather than being passed in directly. This inference happens in tag_extraction::extract_tags, with the result being written into sentry_tags and retrieved here.
This code to set the span description could be moved into extract_tags, but it would mean making its span parameter a mutable reference and mutating span in there (setting its description), which feels a bit out of scope of what I'd expect extract_tags to do.
An alternative might be to make category a field on Span, set it earlier in the process, and use that everywhere we want to query category instead of digging into sentry_tags. (It would then be the responsibility of Snuba to write it into the attributes dictionary I suppose).
| fn infer_span_description(span: &Span) -> Option<String> { | ||
| let category = span.sentry_tags.value()?.category.value()?; | ||
| match category.as_str() { | ||
| "db" => span.data.value()?.db_query_text.value()?.to_owned().into(), |
There was a problem hiding this comment.
We already have a promote_span_data_fields function which seems to be a perfect fit for this logic.
There was a problem hiding this comment.
promote_span_data_fields seems to be moving properties out of data and into top level span attributes:
Does it make sense to move this kind of logic, which is conditional on other span fields and also doesn't move data, into the same spot?
The other challenge is that we need to know the category, which isn't calculated yet at the time promote_span_data_fields is run. (See other comments).
| /// E.g. SELECT * FROM wuser_table where username = ?; SET mykey ? | ||
| /// See [OpenTelemetry docs for a list of well-known identifiers](https://opentelemetry.io/docs/specs/semconv/database/database-spans/#common-attributes). | ||
| #[metastructure(field = "db.query.text", legacy_alias = "db.statement")] | ||
| pub db_query_text: Annotated<String>, |
There was a problem hiding this comment.
Does the SQL scrubber need to run over this field, also if the field gets copied into the description, is it being scrubbed?
We should have an integration test for this.
There was a problem hiding this comment.
The span description that we receive isn't scrubbed or otherwise modified. So writing the unmodified db.query.text value into description is expected.
(The exception is PII scrubbing, although description isn't a default field so that relies on user configuration. That happens later in span processing so we should be covered there.)
A scrubbed copy of description is made in extract_tags and stored in sentry_tags' description field. This scrubbed version is used by Sentry's Insights modules. That scrubbed description extraction does not run for the inferred descriptions in this PR, as the scrubbing happens in extract_tags.
That is fine, though: supporting Insights is a later milestone of the span first/OTLP project. It isn't expected that Insights works with OTLP. If it was simple enough to generate the scrubbed descriptions I'd consider it, but the scrubbing also currently depends on op - that has all got to be replaced too before it's usable with OTLP. We'll be changing how that works when we work on that milestone.
I agree about an integration test though, we'll add one.
Co-authored-by: David Herberth <[email protected]>
This PR is part of the OTLP initiative and will allow Relay to determine the
descriptionfield on spans by using the provided metadata. In this case specifically, Relay will use check if the span'scategoryis set todb, in which case it will find the DB query and set that as the span'sdescription.Co-authored-by: Matt Quinn [email protected]`