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.

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

feat(spans): Filter spans based on module #2511

Merged
merged 21 commits into from
Sep 14, 2023

Conversation

phacops
Copy link
Contributor

@phacops phacops commented Sep 13, 2023

We're trying to filter spans based on the op field as we prepare to ingest spans for GA. Due to the high cardinality of groupings, we would like to be selective on what we ingest.

At first, we'd want to ingest spans for our DB and Browser modules. If the feature is enabled for the project, we'd add the necessary conditions to match spans corresponding to op we're interesting in.

If no module is enabled, we ingest all spans.

@phacops phacops requested a review from a team September 13, 2023 19:54
Comment on lines 68 to 71
"ui.*".into(),
"browser.*".into(),
"http.*".into(),
"grouping".into(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AbhiPrasad Can you validate these are the right values for the browser module? Maybe it should be without dots?

Copy link
Member

Choose a reason for hiding this comment

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

The values should as per https://develop.sentry.dev/sdk/performance/span-operations/, there are some example span ops in there as well.

Comment on lines 2282 to 2292
let span_op_db_module_denylist: HashSet<String> = HashSet::from([
"db.clickhouse".into(),
"db.redis".into(),
"db.sql.query".into(),
]);
let span_op_browser_module_prefix_allowlist: Vec<String> = vec![
"browser".into(),
"http".into(),
"resource".into(),
"ui".into(),
];
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these don't change during execution, what do you think about making them static and wrapping them in OnceCell? See another example in Relay.

value: GlobPatterns::new(vec![
"browser*".into(),
"http*".into(),
"resource".into(),
Copy link
Member

Choose a reason for hiding this comment

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

resource can also be resource.script, and resource.css so I think we have to glob here as well.

Copy link
Member

@gggritso gggritso left a comment

Choose a reason for hiding this comment

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

👀 nice!

value: Value::Array(vec![
"db.clickhouse".into(),
"db.redis".into(),
"db.sql.query".into(),
Copy link
Member

Choose a reason for hiding this comment

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

@AbhiPrasad what do you think about "db.sql.query"? I feel like we should allow this by default, and opt out the one problematic project that came up so far

Copy link
Member

Choose a reason for hiding this comment

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

ah yeah I think we should allow it - good catch!

@phacops
Copy link
Contributor Author

phacops commented Sep 14, 2023

I ended up simplifying into 2 groups: GA and all modules.

We'll keep the filtering in Relay for the GA case and accepts all spans for projects in the allowlist.

@phacops phacops requested a review from jan-auer September 14, 2023 16:14
relay-dynamic-config/src/defaults.rs Outdated Show resolved Hide resolved
relay-dynamic-config/src/feature.rs Outdated Show resolved Hide resolved
relay-server/src/actors/processor.rs Outdated Show resolved Hide resolved
@phacops phacops enabled auto-merge (squash) September 14, 2023 20:15
@phacops phacops merged commit cb88f12 into master Sep 14, 2023
@phacops phacops deleted the pierre/spans-add-condition-on-module-tag branch September 14, 2023 20:59
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.

5 participants