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.

Sign up for GitHub

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

Jump to bottom

feat(spans): Allow activerecord with db.system #2545

Merged
merged 5 commits into from
Sep 27, 2023
Merged

feat(spans): Allow activerecord with db.system #2545

merged 5 commits into from
Sep 27, 2023

Conversation

jjbayer
Copy link
Member

@jjbayer jjbayer commented Sep 27, 2023 •
edited
Loading

We've seen SQL query normalization fail when the queries are prepended with an active record comment, for example

/*some comment `my_function'*/ SELECT 1

Normalization works fine when sqlparser is able to parse the query though. Having a db.system in span.data is a strong indicator that we will be able to parse the query. So we re-allow activerecord spans if they have a db.system set.

In addition, this PR also improves the fallback regex behavior for this kind of comments.

ref: internal issue

@jjbayer jjbayer changed the title Ref/spans simplify filters feat(spans): Allow activerecord with db.system Sep 27, 2023
jjbayer
jjbayer commented Sep 27, 2023
View reviewed changes
relay-dynamic-config/src/defaults.rs
@@ -47,7 +47,6 @@ pub fn add_span_metrics(project_config: &mut ProjectConfig) {
inner: Box::new(RuleCondition::Glob(GlobCondition {
name: span_op_field_name.into(),
value: GlobPatterns::new(vec![
"*active*record*".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.

We now allow metrics from active record spans.

relay-event-normalization/src/normalize/span/description/mod.rs
if sub.contains("clickhouse")
|| sub.contains("mongodb")
|| sub.contains("redis")
|| is_legacy_activerecord(sub, 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.

When active record does not provide a db.system, we cannot parse it, so provide no scrubbed description at all.

relay-server/src/actors/processor.rs
|| op.contains("redis"))
&& !(op == "db.sql.query" && !(description.contains(r#""$"#) || system == "mongodb"))
&& !(op.contains("clickhouse") || op.contains("mongodb") || op.contains("redis"))
&& !(op == "db.sql.query" && (description.contains(r#""$"#) || system == "mongodb"))
Copy link
Member Author

Choose a reason for hiding this comment

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

Drive by fix: There was a double negation here that included db.sql.query only for mongodb.

relay-event-normalization/src/normalize/span/description/sql/mod.rs
(&COMMENTS, "\n"),
(&INLINE_COMMENTS, ""),
(&NORMALIZER_REGEX, "$pre%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.

Comments may contain string delimiters ('), and strings may contain comment delimiters. So no matter how we order the regexes, one can construct a case where scrubbing does not work. Prefer this order for now because we've seen that the previous order destroyed active record descriptions.

@jjbayer jjbayer marked this pull request as ready for review September 27, 2023 08:50
@jjbayer jjbayer requested review from a team, phacops and gggritso September 27, 2023 08:50
olksdr
olksdr approved these changes Sep 27, 2023
View reviewed changes
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.

lgtm!

@jjbayer jjbayer merged commit b9d0722 into master Sep 27, 2023
@jjbayer jjbayer deleted the ref/spans-simplify-filters branch September 27, 2023 10:29
jan-auer added a commit that referenced this pull request Sep 27, 2023
@jan-auer
Merge branch 'master' into ref/rule-condition-builder
b57ac8f 
* master:
  feat(spans): Allow activerecord with db.system (#2545)
  ref(sampling): Replace Evaluator with ControlFlow. (#2544)
  Add main cluster to gocd pop jobs (#2529)
  build(actions): Bump `checkout` action to v4 (#2539)
  fix(beta): Address beta lint warnings (#2541)
  fix(spans): Detect ODBC escape sequence with regex (#2543)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iker-barriocanal iker-barriocanal iker-barriocanal approved these changes

@olksdr olksdr olksdr approved these changes

@phacops phacops Awaiting requested review from phacops

@gggritso gggritso Awaiting requested review from gggritso

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jjbayer @olksdr @iker-barriocanal