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(metrics): Add ability to compute metrics summary and attach it to events #3769

Merged
merged 43 commits into from
Jul 4, 2024

Conversation

iambriccardo
Copy link
Member

@iambriccardo iambriccardo commented Jun 27, 2024

This PR introduces functionality to compute metric summaries from a slice of Buckets, adhering to the semantics currently used in our SDKs.

The motivation for this change stems from our decision to discontinue metric emission from SDKs, instead extracting them in Relay through specified configurations. Without this update, the current Relay implementation would compromise the product experience, as it cannot extract metric summaries from event-derived metrics. This PR addresses and resolves this limitation.

Currently, metrics summaries are computed only for metrics belonging to the custom namespace.

Closes: getsentry/sentry#73152

Comment on lines 26 to 34
/// Merges a [`MetricsSummary`] into itself.
///
/// The merge operation concatenates tags of the same metric even if they are identical for the
/// sake of simplicity. This doesn't cause issues for us, under the assumption that the two
/// summaries do not have overlapping metric names (which is the case for us since summaries
/// computed by the SDKs have metric names that are different from the ones we generate in
/// Relay).
///
/// Do not use this function to merge any arbitrary [`MetricsSummary`]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.

I am not happy about this comment, open for suggestions.

Comment on lines 200 to 202
if buckets.is_empty() {
return None;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Small optimization to avoid creating an aggregator with nothing inside.

@iambriccardo iambriccardo marked this pull request as ready for review June 28, 2024 07:20
@iambriccardo iambriccardo requested a review from a team as a code owner June 28, 2024 07:20
}

/// Computes the [`MetricsSummary`] from a slice of [`Bucket`]s.
pub fn compute(buckets: &[Bucket]) -> Option<MetricsSummary> {
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 add a form of option or feature flag that controls whether this code should actually run. A global flag would be absolutely sufficient, though it would be useful to have the ability to control this per organization/project. Choose based on what's more straight forward to integrate.

.filter(|b| matches!(b.name.namespace(), MetricNamespace::Custom));

let aggregator = MetricsSummaryAggregator::from_buckets(filtered_buckets);
if aggregator.is_empty() {
Copy link
Member Author

@iambriccardo iambriccardo Jul 1, 2024

Choose a reason for hiding this comment

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

We do the check in the aggregator directly so that we avoid collecting the filtered buckets into a vector just for the sake of determining the size before calling from_buckets.

relay-server/src/metrics_extraction/event.rs Outdated Show resolved Hide resolved
relay-server/src/metrics_extraction/event.rs Outdated Show resolved Hide resolved
relay-server/src/metrics_extraction/event.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
relay-server/src/metrics_extraction/metrics_summary.rs Outdated Show resolved Hide resolved
relay-server/src/metrics_extraction/metrics_summary.rs Outdated Show resolved Hide resolved
relay-server/src/metrics_extraction/metrics_summary.rs Outdated Show resolved Hide resolved
relay-server/src/metrics_extraction/metrics_summary.rs Outdated Show resolved Hide resolved
relay-server/src/metrics_extraction/metrics_summary.rs Outdated Show resolved Hide resolved
relay-server/src/metrics_extraction/metrics_summary.rs Outdated Show resolved Hide resolved
@iambriccardo iambriccardo merged commit 2392ad7 into master Jul 4, 2024
23 checks passed
@iambriccardo iambriccardo deleted the riccardo/feat/build-summaries branch July 4, 2024 08:03
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.

emit metrics summaries from relay when we have extraction rules
5 participants