Skip to content
New issue

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

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

Already on GitHub? # to your account

fix(spans): Use db.system for SQL parsing #2536

Merged
merged 2 commits into from
Sep 26, 2023
Merged

Conversation

jjbayer
Copy link
Member

@jjbayer jjbayer commented Sep 26, 2023

The SQL parser used for span description normalization and grouping accessed the wrong field to pick the SQL dialect. This bug did not durface in unit tests, because I passed the system string into those tests directly.

@@ -34,7 +34,7 @@ pub(crate) fn scrub_span_description(span: &mut Span, rules: &Vec<SpanDescriptio
("db", _) => sql::scrub_queries(
span.data
.value()
.and_then(|d| d.get("system"))
.and_then(|d| d.get("db.system"))
Copy link
Member Author

Choose a reason for hiding this comment

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

.and_then(|d| d.get("description.scrubbed"))
.and_then(|v| v.value())
.and_then(|v| v.as_str());
assert_eq!(scrubbed, Some("SELECT %s"));
Copy link
Member Author

Choose a reason for hiding this comment

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

Without the fix, this would be SELECT not an identifier, because the default parser would assume that double quotes delimit an identifier.

@jjbayer jjbayer marked this pull request as ready for review September 26, 2023 08:14
@jjbayer jjbayer requested a review from a team September 26, 2023 08:14
Copy link
Contributor

@olksdr olksdr left a comment

Choose a reason for hiding this comment

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

🚢 it!

@jjbayer jjbayer merged commit c0ba1f5 into master Sep 26, 2023
20 checks passed
@jjbayer jjbayer deleted the fix/spans-db-system branch September 26, 2023 09:28
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants