feat(metrics): Limit metric name to 150 chars #3628
Conversation
| @@ -464,6 +464,23 @@ mod tests { | |||
| ); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_normalize_name_length() { | |||
| let long_mri = "c:custom/ThisIsACharacterLongStringForTestingPurposesToEnsureThatWeHaveEnoughCharactersToWorkWithAndToCheckIfOurFunctionProperlyHandlesSlicingAndNormalizationWithoutErrors"; | |||
There was a problem hiding this comment.
makes sense to also have a test on how it behaves when we have replacement characters in the name which get replaced with a single underscore
| Cow::Borrowed(value) => Cow::Borrowed(&value[..CUSTOM_METRIC_NAME_MAX_SIZE]), | ||
| Cow::Owned(mut value) => { | ||
| value.truncate(CUSTOM_METRIC_NAME_MAX_SIZE); |
There was a problem hiding this comment.
Note that both &value[..CUSTOM_METRIC_NAME_MAX_SIZE] and String::truncate would panic if it wasn't for normalize_re and the .len() check. I don't have a better idea right now, but it's something to keep in mind if we ever refactor this function.
Maybe we can turn the early return of the length check into a if normalized.len() > CUSTOM_METRIC_NAME_MAX_SIZE { ... } block to clarify that these belong together, and add a debug_assert!(value.is_ascii());
There was a problem hiding this comment.
Yeah, this was considered, in my opinion our best bet is to put enough good tests so that a malicious refactor will be noticed.
relay-base-schema/src/metrics/mri.rs
Outdated
| @@ -474,6 +474,14 @@ mod tests { | |||
| "ThisIsACharacterLongStringForTestingPurposesToEnsureThatWeHaveEnoughCharactersToWorkWithAndToCheckIfOurFunctionProperlyHandlesSlicingAndNormalizationW" | |||
| ); | |||
|
|
|||
| let long_mri_with_replacement = "c:custom/ThisIsÄCharacterLongStringForŤestingPurposesToEnsureThatWeHaveEnoughCharactersToWorkWithAndToCheckIfOurFunctionProperlyHandlesSlicingAndNormalizationWithoutErrors"; | |||
There was a problem hiding this comment.
The test is supposed to show what happens if multiple characters get replaced with a single underscore. Also possibly makes sense to have a separate test case for exactly this behaviour.
This PR limits the metric name to 150 characters, which is added to the limit of 15 characters in the metric unit. Since we have a total of 200 characters supported by the indexer, we now have 35 chars for separators, type and namespace of the MRI.
Closes: #3614