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(server): Denylist for metrics #2954

Merged
merged 24 commits into from
Jan 22, 2024
Merged

feat(server): Denylist for metrics #2954

merged 24 commits into from
Jan 22, 2024

Conversation

TBS1996
Copy link
Contributor

@TBS1996 TBS1996 commented Jan 17, 2024

New addition to projectconfig that allow users to filter out certain metrics based on their name using glob patterns.

#2796

@TBS1996 TBS1996 self-assigned this Jan 17, 2024
@TBS1996 TBS1996 marked this pull request as ready for review January 17, 2024 13:15
@TBS1996 TBS1996 requested a review from a team as a code owner January 17, 2024 13:15
Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

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

Just a quick check of things, I need in my PR as well

relay-dynamic-config/src/project.rs Outdated Show resolved Hide resolved
relay-dynamic-config/src/project.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved

impl Metrics {
/// Filters metrics based on the metric name.
pub fn filter_metrics(&self, metrics: &mut Vec<Bucket>) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this function into a logic module to keep the config types plain. For now, processor.rs would be the ideal place for this, until all metrics related processing functionality moves into a dedicated module.

This means you can also remove the dependency on relay_metrics from this crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just moved this logic into the apply_filter_metrics function in project.rs, how do you think about that?

relay-dynamic-config/src/metrics.rs Outdated Show resolved Hide resolved
relay-dynamic-config/src/project.rs Outdated Show resolved Hide resolved
relay-dynamic-config/src/project.rs Outdated Show resolved Hide resolved
relay-dynamic-config/src/project.rs Outdated Show resolved Hide resolved
relay-dynamic-config/src/metrics.rs Show resolved Hide resolved
relay-server/src/services/project.rs Show resolved Hide resolved
#[derive(Debug, Default, Clone, Serialize, Deserialize)]
#[serde(transparent)]
pub struct Metrics {
/// Patterns of names of metrics that we want to filter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Patterns of names of metrics that we want to filter.
/// The list of patterns which used to filter names of the received metrics.

Maybe something like this will sound better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, isnt it a bit misleading, like we want to say we filter metrics based on their name, not that we filter their names?

how about /// List of patterns for blocking metrics based on their name. ?

@TBS1996 TBS1996 requested review from olksdr and Dav1dde January 19, 2024 13:22
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Approved once #[serde(transparent)] is removed

relay-dynamic-config/src/metrics.rs Outdated Show resolved Hide resolved
Co-authored-by: Jan Michael Auer <mail@jauer.org>
}

impl Metrics {
/// Returns `true` if it contains any names patterns to filter metric names.
Copy link
Member

Choose a reason for hiding this comment

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

This comment is outdated, is_empty does more now

relay-dynamic-config/src/metrics.rs Show resolved Hide resolved
@@ -88,6 +89,9 @@ pub struct ProjectConfig {
/// relays that might still need them.
#[serde(skip_serializing_if = "Option::is_none")]
pub span_description_rules: Option<Vec<SpanDescriptionRule>>,
/// Configuration for filtering metrics.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Configuration for filtering metrics.
/// Configuration for metrics.

@TBS1996 TBS1996 requested a review from Dav1dde January 22, 2024 11:38
Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

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

This seems like a footgun that the there is no explicit filter for namespaces at least, but this was already discussed and marked as a future improvement.

@Dav1dde Dav1dde self-requested a review January 22, 2024 12:11
Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

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

Does this get rid of Request Changes now?

@TBS1996 TBS1996 merged commit 009504e into master Jan 22, 2024
20 checks passed
@TBS1996 TBS1996 deleted the denymet branch January 22, 2024 12:23
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.

4 participants