Skip to content

Commit

Permalink
Merge branch 'master' into fix/accept-unknown-items
Browse files Browse the repository at this point in the history 
* master:
  fix(metrics): Enforce metric name length limit (#1238)
  • Loading branch information
@jan-auer
jan-auer committed Apr 28, 2022
2 parents 19817fe + 10874b5 commit 0a3228e
Show file tree
Hide file tree
Showing 2 changed files with 163 additions and 12 deletions.
4 changes: 3 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@
- Add units in built-in measurements. ([#1229](https://github.com/getsentry/relay/pull/1229))

**Bug Fixes**:

- fix(metrics): Enforce metric name length limit. ([#1238](https://github.com/getsentry/relay/pull/1238))
- Accept and forward unknown Envelope items. In processing mode, drop items individually rather than rejecting the entire request. This allows SDKs to send new data in combined Envelopes in the future. ([#1246](https://github.com/getsentry/relay/pull/1246))

**Internal**:

* Add sampling + tagging by event platform and transaction op. Some (unused) tagging rules from 22.4.0 have been renamed. ([#1231](https://github.com/getsentry/relay/pull/1231))
- Add sampling + tagging by event platform and transaction op. Some (unused) tagging rules from 22.4.0 have been renamed. ([#1231](https://github.com/getsentry/relay/pull/1231))
- Refactor aggregation error, recover from errors more gracefully. ([#1240](https://github.com/getsentry/relay/pull/1240))
- Remove/reject nul-bytes from metric strings. ([#1235](https://github.com/getsentry/relay/pull/1235))
- Remove the unused "internal" data category. ([#1245](https://github.com/getsentry/relay/pull/1245))
Expand Down
171 changes: 160 additions & 11 deletions relay-metrics/src/aggregation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -715,7 +715,7 @@ impl Bucket {
}

/// Any error that may occur during aggregation.
#[derive(Debug, Fail)]
#[derive(Debug, Fail, PartialEq)]
#[fail(display = "failed to aggregate metrics: {}", kind)]
pub struct AggregateMetricsError {
kind: AggregateMetricsErrorKind,
Expand All @@ -727,7 +727,7 @@ impl From<AggregateMetricsErrorKind> for AggregateMetricsError {
}
}

#[derive(Debug, Fail)]
#[derive(Debug, Fail, PartialEq)]
#[allow(clippy::enum_variant_names)]
enum AggregateMetricsErrorKind {
/// A metric bucket had invalid characters in the metric name.
Expand All @@ -739,6 +739,9 @@ enum AggregateMetricsErrorKind {
/// Internal error: Attempted to merge two metric buckets of different types.
#[fail(display = "found incompatible metric types")]
InvalidTypes,
/// A metric bucket had a too long string (metric name or a tag key/value).
#[fail(display = "found invalid string")]
InvalidStringLength,
}

#[derive(Clone, Debug, PartialEq, Eq, Hash)]
Expand Down Expand Up @@ -807,6 +810,21 @@ pub struct AggregatorConfig {
///
/// Defaults to 1 minute.
pub max_secs_in_future: u64,

/// The length the name of a metric is allowed to be.
///
/// Defaults to `200` bytes.
pub max_name_length: usize,

/// The length the tag key is allowed to be.
///
/// Defaults to `200` bytes.
pub max_tag_key_length: usize,

/// The length the tag value is allowed to be.
///
/// Defaults to `200` bytes.
pub max_tag_value_length: usize,
}

impl AggregatorConfig {
Expand Down Expand Up @@ -903,6 +921,9 @@ impl Default for AggregatorConfig {
debounce_delay: 10,
max_secs_in_past: 5 * 24 * 60 * 60, // 5 days, as for sessions
max_secs_in_future: 60, // 1 minute
max_name_length: 200,
max_tag_key_length: 200,
max_tag_value_length: 200,
}
}
}
Expand Down Expand Up @@ -1066,10 +1087,45 @@ impl Aggregator {
}
}

/// Remove invalid characters from tags and metric names.
/// Validates the metric name and its tags are correct.
///
/// Returns `Err` if the metric should be dropped.
fn validate_bucket_key(mut key: BucketKey) -> Result<BucketKey, AggregateMetricsError> {
fn validate_bucket_key(
mut key: BucketKey,
aggregator_config: &AggregatorConfig,
) -> Result<BucketKey, AggregateMetricsError> {
key = Self::validate_metric_name(key, aggregator_config)?;
key = Self::validate_metric_tags(key, aggregator_config);
Ok(key)
}

/// Removes invalid characters from metric names.
///
/// Returns `Err` if the metric must be dropped.
fn validate_metric_name(
key: BucketKey,
aggregator_config: &AggregatorConfig,
) -> Result<BucketKey, AggregateMetricsError> {
let metric_name_length = key.metric_name.len();
if metric_name_length > aggregator_config.max_name_length {
relay_log::configure_scope(|scope| {
scope.set_extra(
"bucket.project_key",
key.project_key.as_str().to_owned().into(),
);
scope.set_extra("bucket.metric_name", key.metric_name.into());
scope.set_extra(
"bucket.metric_name.length",
metric_name_length.to_string().into(),
);
scope.set_extra(
"aggregator_config.max_name_length",
aggregator_config.max_name_length.to_string().into(),
);
});
return Err(AggregateMetricsErrorKind::InvalidStringLength.into());
}

if !protocol::is_valid_mri(&key.metric_name) {
relay_log::debug!("invalid metric name {:?}", key.metric_name);
relay_log::configure_scope(|scope| {
Expand All @@ -1081,8 +1137,40 @@ impl Aggregator {
});
return Err(AggregateMetricsErrorKind::InvalidCharacters.into());
}
Ok(key)
}

/// Removes tags with invalid characters in the key, and validates tag values.
///
/// Tag values are validated with `protocol::validate_tag_value`.
fn validate_metric_tags(mut key: BucketKey, aggregator_config: &AggregatorConfig) -> BucketKey {
let proj_key = key.project_key.as_str();
key.tags.retain(|tag_key, tag_value| {
if tag_key.len() > aggregator_config.max_tag_key_length {
relay_log::configure_scope(|scope| {
scope.set_extra("bucket.project_key", proj_key.to_owned().into());
scope.set_extra("bucket.metric.tag_key", tag_key.to_owned().into());
scope.set_extra(
"aggregator_config.max_tag_key_length",
aggregator_config.max_tag_key_length.to_string().into(),
);
});
relay_log::debug!("Invalid metric tag key");
return false;
}
if tag_value.len() > aggregator_config.max_tag_value_length {
relay_log::configure_scope(|scope| {
scope.set_extra("bucket.project_key", proj_key.to_owned().into());
scope.set_extra("bucket.metric.tag_value", tag_value.to_owned().into());
scope.set_extra(
"aggregator_config.max_tag_value_length",
aggregator_config.max_tag_value_length.to_string().into(),
);
});
relay_log::debug!("Invalid metric tag value");
return false;
}

key.tags.retain(|tag_key, _| {
if protocol::is_valid_tag_key(tag_key) {
true
} else {
Expand All @@ -1093,8 +1181,7 @@ impl Aggregator {
for (_, tag_value) in key.tags.iter_mut() {
protocol::validate_tag_value(tag_value);
}

Ok(key)
key
}

/// Merges any mergeable value into the bucket at the given `key`.
Expand All @@ -1108,7 +1195,7 @@ impl Aggregator {
let timestamp = key.timestamp;
let project_key = key.project_key;

let key = Self::validate_bucket_key(key)?;
let key = Self::validate_bucket_key(key, &self.config)?;

match self.buckets.entry(key) {
Entry::Occupied(mut entry) => {
Expand Down Expand Up @@ -1486,6 +1573,9 @@ mod tests {
debounce_delay: 0,
max_secs_in_past: 50 * 365 * 24 * 60 * 60,
max_secs_in_future: 50 * 365 * 24 * 60 * 60,
max_name_length: 200,
max_tag_key_length: 200,
max_tag_value_length: 200,
}
}

Expand Down Expand Up @@ -2140,7 +2230,7 @@ mod tests {
}

#[test]
fn test_validate_bucket_key() {
fn test_validate_bucket_key_chars() {
let project_key = ProjectKey::parse("a94ae32be2584e0bbd7a4cbb95971fee").unwrap();

let bucket_key = BucketKey {
Expand Down Expand Up @@ -2171,8 +2261,10 @@ mod tests {
tags
},
};
let aggregator_config = test_config();

let mut bucket_key = Aggregator::validate_bucket_key(bucket_key).unwrap();
let mut bucket_key =
Aggregator::validate_bucket_key(bucket_key, &aggregator_config).unwrap();

assert_eq!(bucket_key.tags.len(), 1);
assert_eq!(
Expand All @@ -2182,6 +2274,63 @@ mod tests {
assert_eq!(bucket_key.tags.get("another\0garbage"), None);

bucket_key.metric_name = "hergus\0bergus".to_owned();
Aggregator::validate_bucket_key(bucket_key).unwrap_err();
Aggregator::validate_bucket_key(bucket_key, &aggregator_config).unwrap_err();
}

#[test]
fn test_validate_bucket_key_str_lens() {
relay_test::setup();
let project_key = ProjectKey::parse("a94ae32be2584e0bbd7a4cbb95971fee").unwrap();
let aggregator_config = test_config();

let short_metric = BucketKey {
project_key,
timestamp: UnixTimestamp::now(),
metric_name: "c:a_short_metric".to_owned(),
metric_type: MetricType::Counter,
metric_unit: MetricUnit::None,
tags: BTreeMap::new(),
};
assert!(Aggregator::validate_bucket_key(short_metric, &aggregator_config).is_ok());

let long_metric = BucketKey {
project_key,
timestamp: UnixTimestamp::now(),
metric_name: "c:long_name_a_very_long_name_its_super_long_really_but_like_super_long_probably_the_longest_name_youve_seen_and_even_the_longest_name_ever_its_extremly_long_i_cant_tell_how_long_it_is_because_i_dont_have_that_many_fingers_thus_i_cant_count_the_many_characters_this_long_name_is".to_owned(),
metric_type: MetricType::Counter,
metric_unit: MetricUnit::None,
tags: BTreeMap::new(),
};
let validation = Aggregator::validate_bucket_key(long_metric, &aggregator_config);

assert_eq!(
validation.unwrap_err(),
AggregateMetricsError::from(AggregateMetricsErrorKind::InvalidStringLength)
);

let short_metric_long_tag_key = BucketKey {
project_key,
timestamp: UnixTimestamp::now(),
metric_name: "c:a_short_metric_with_long_tag_key".to_owned(),
metric_type: MetricType::Counter,
metric_unit: MetricUnit::None,
tags: BTreeMap::from([("i_run_out_of_creativity_so_here_we_go_Lorem_Ipsum_is_simply_dummy_text_of_the_printing_and_typesetting_industry_Lorem_Ipsum_has_been_the_industrys_standard_dummy_text_ever_since_the_1500s_when_an_unknown_printer_took_a_galley_of_type_and_scrambled_it_to_make_a_type_specimen_book".into(), "tag_value".into())]),
};
let validation =
Aggregator::validate_bucket_key(short_metric_long_tag_key, &aggregator_config).unwrap();
assert_eq!(validation.tags.len(), 0);

let short_metric_long_tag_value = BucketKey {
project_key,
timestamp: UnixTimestamp::now(),
metric_name: "c:a_short_metric_with_long_tag_value".to_owned(),
metric_type: MetricType::Counter,
metric_unit: MetricUnit::None,
tags: BTreeMap::from([("tag_key".into(), "i_run_out_of_creativity_so_here_we_go_Lorem_Ipsum_is_simply_dummy_text_of_the_printing_and_typesetting_industry_Lorem_Ipsum_has_been_the_industrys_standard_dummy_text_ever_since_the_1500s_when_an_unknown_printer_took_a_galley_of_type_and_scrambled_it_to_make_a_type_specimen_book".into())]),
};
let validation =
Aggregator::validate_bucket_key(short_metric_long_tag_value, &aggregator_config)
.unwrap();
assert_eq!(validation.tags.len(), 0);
}
}

0 comments on commit 0a3228e

Please sign in to comment.