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

ref(metrics): Remove option for controlling metrics summaries calculation #3797

Merged
merged 2 commits into from
Jul 8, 2024
Merged

ref(metrics): Remove option for controlling metrics summaries calculation #3797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ab86b1
ref(metrics): Remove option for controlling metrics summaries calcula…
iambriccardo Jul 8, 2024
ba770cf
Fix
iambriccardo Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions relay-dynamic-config/src/global.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,14 +214,6 @@ pub struct Options {
)]
pub span_extraction_sample_rate: Option<f32>,

/// Overall sampling of metrics summaries computation.
#[serde(
rename = "relay.compute-metrics-summaries.sample-rate",
deserialize_with = "default_on_error",
skip_serializing_if = "is_default"
)]
pub compute_metrics_summaries_sample_rate: Option<f32>,
Comment on lines -217 to -223
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to keep forwarding this option to external relays so they keep computing the summary? Or is this option so new that we don't care?

Copy link
Member

Choose a reason for hiding this comment

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

Discussed offline, this was only enabled for our own org so it can be removed.


/// The maximum duplication factor used to extrapolate distribution metrics from sampled data.
///
/// This applies as long as Relay duplicates distribution values to extrapolate. The default is
Expand Down
28 changes: 3 additions & 25 deletions relay-server/src/metrics_extraction/event.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,20 +52,11 @@ pub fn extract_metrics(
config: CombinedMetricExtractionConfig<'_>,
max_tag_value_size: usize,
span_extraction_sample_rate: Option<f32>,
compute_metrics_summaries_sample_rate: Option<f32>,
) -> Vec<Bucket> {
let mut metrics = generic::extract_metrics(event, config);
// If spans were already extracted for an event, we rely on span processing to extract metrics.
if !spans_extracted && sample(span_extraction_sample_rate.unwrap_or(1.0)) {
let compute_metrics_summaries_sample_rate =
compute_metrics_summaries_sample_rate.unwrap_or(1.0);
extract_span_metrics_for_event(
event,
config,
max_tag_value_size,
&mut metrics,
compute_metrics_summaries_sample_rate,
);
extract_span_metrics_for_event(event, config, max_tag_value_size, &mut metrics);
}

metrics
Expand All @@ -76,17 +67,12 @@ fn extract_span_metrics_for_event(
config: CombinedMetricExtractionConfig<'_>,
max_tag_value_size: usize,
output: &mut Vec<Bucket>,
compute_metrics_summaries_sample_rate: f32,
) {
let compute_metrics_summaries = sample(compute_metrics_summaries_sample_rate);

relay_statsd::metric!(timer(RelayTimers::EventProcessingSpanMetricsExtraction), {
if let Some(transaction_span) = extract_transaction_span(event, max_tag_value_size) {
let (metrics, metrics_summary) =
metrics_summary::extract_and_summarize_metrics(&transaction_span, config);
if compute_metrics_summaries {
metrics_summary.apply_on(&mut event._metrics_summary);
}
metrics_summary.apply_on(&mut event._metrics_summary);
output.extend(metrics);
}

Expand All @@ -95,9 +81,7 @@ fn extract_span_metrics_for_event(
if let Some(span) = annotated_span.value_mut() {
let (metrics, metrics_summary) =
metrics_summary::extract_and_summarize_metrics(span, config);
if compute_metrics_summaries {
metrics_summary.apply_on(&mut span._metrics_summary);
}
metrics_summary.apply_on(&mut span._metrics_summary);
output.extend(metrics);
}
}
Expand Down Expand Up @@ -1211,7 +1195,6 @@ mod tests {
combined_config(features, None).combined(),
200,
None,
None,
)
}

Expand Down Expand Up @@ -1414,7 +1397,6 @@ mod tests {
combined_config([Feature::ExtractCommonSpanMetricsFromEvent], None).combined(),
200,
None,
None,
);
insta::assert_debug_snapshot!((&event.value().unwrap().spans, metrics));
}
Expand Down Expand Up @@ -1472,7 +1454,6 @@ mod tests {
combined_config([Feature::ExtractCommonSpanMetricsFromEvent], None).combined(),
200,
None,
None,
);

// When transaction.op:ui.load and mobile:true, HTTP spans still get both
Expand Down Expand Up @@ -1505,7 +1486,6 @@ mod tests {
combined_config([Feature::ExtractCommonSpanMetricsFromEvent], None).combined(),
200,
None,
None,
);

let usage_metrics = metrics
Expand Down Expand Up @@ -1729,7 +1709,6 @@ mod tests {
combined_config([Feature::ExtractCommonSpanMetricsFromEvent], None).combined(),
200,
None,
None,
);

assert_eq!(metrics.len(), 4);
Expand Down Expand Up @@ -1872,7 +1851,6 @@ mod tests {
config,
200,
None,
None,
);

insta::assert_debug_snapshot!(&event.value().unwrap()._metrics_summary);
Expand Down
1 change: 0 additions & 1 deletion relay-server/src/services/processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1428,7 +1428,6 @@ impl EnvelopeProcessorService {
.aggregator
.max_tag_value_length,
global.options.span_extraction_sample_rate,
global.options.compute_metrics_summaries_sample_rate,
);

state
Expand Down
9 changes: 1 addition & 8 deletions relay-server/src/services/processor/span/processing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,14 +158,7 @@ pub fn process(
span,
CombinedMetricExtractionConfig::new(global_metrics_config, config),
);
if sample(
global_config
.options
.compute_metrics_summaries_sample_rate
.unwrap_or(1.0),
) {
metrics_summary.apply_on(&mut span._metrics_summary)
}
metrics_summary.apply_on(&mut span._metrics_summary);

state
.extracted_metrics
Expand Down
8 changes: 0 additions & 8 deletions tests/integration/test_spans.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2062,10 +2062,6 @@ def test_metrics_summary_with_extracted_spans(
relay_with_processing,
metrics_summaries_consumer,
):
mini_sentry.global_config["options"] = {
"relay.compute-metrics-summaries.sample-rate": 1.0
}

metrics_summaries_consumer = metrics_summaries_consumer()

relay = relay_with_processing()
Expand Down Expand Up @@ -2139,10 +2135,6 @@ def test_metrics_summary_with_standalone_spans(
relay_with_processing,
metrics_summaries_consumer,
):
mini_sentry.global_config["options"] = {
"relay.compute-metrics-summaries.sample-rate": 1.0
}

metrics_summaries_consumer = metrics_summaries_consumer()

relay = relay_with_processing()
Expand Down
Loading