fix(protocol): Disallow - in measurement and breakdown names
#1571
Conversation
relay-common/src/utils.rs
Outdated
| } | ||
| } | ||
| false | ||
| } |
There was a problem hiding this comment.
I had to move this function from relay-metrics to relay-common so it can be used from relay-general.
relay-common/src/utils.rs
Outdated
| } | ||
| } | ||
| false | ||
| } |
There was a problem hiding this comment.
I had to move this function from relay-metrics to relay-common so it can be used from relay-general. Note that this version no longer allows /.
There was a problem hiding this comment.
Can disallowing the usage of / cause issues in any of the places where it's currently used?
There was a problem hiding this comment.
The only other usage is here, and I refactored that part to split by / before validating, so all cases should be covered.
relay-metrics/src/protocol.rs
Outdated
| let (raw_namespace, name) = name_and_namespace | ||
| .split_once('/') | ||
| .unwrap_or(("custom", string)); | ||
| let (raw_namespace, name, unit, value) = parse_name_unit_value(name_value_str, ty)?; |
There was a problem hiding this comment.
I had to refactor these parser functions because is_valid_metric_name no longer allows / (because it is also called from normalization). Instead of validate-then-split, we know do split-then-validate.
relay-common/src/utils.rs
Outdated
| } | ||
| } | ||
| false | ||
| } |
There was a problem hiding this comment.
Can disallowing the usage of / cause issues in any of the places where it's currently used?
relay-general/src/store/normalize.rs
Outdated
| "my_custom-measurement_1": {"value": 123}, | ||
| "my_custom_measurement_2": {"value": 789} |
There was a problem hiding this comment.
nit: considering the parsing fails if the name is not correct and we've modified the accepted domain, I'd add a third measurement with a . (valid), and maybe one with a / (invalid).
There was a problem hiding this comment.
This was great advice! By adding some more cases I realized that there is already protocol validation in protocol.rs, which actually adds errors for invalid names, so I reverted all changes in this PR and just fixed the validation there.
jjbayer
force-pushed
the
fix/normalize-enforce-mri
branch
from
d9971f5
to
c649218
Compare
jjbayer
changed the title
- in measurement and breakdown names
|
Sorry I missed this. But this looks good to me 👍 |
Names of custom measurements are converted into MRIs without any validation. This means that in metrics extraction, we are able to construct MRIs that are less strict than what the metrics protocol demands. Subsequent attempts to parse the MRI string into a
MetricsResourceIdentifierobject will fail.This popped up in rate limiting, where we have to parse the MRI to know what namespace and name we are dealing with. Because of this bug, custom measurements with invalid names are currently not rate limited at all.
Fixes POP-RELAY-2N8.