Currently, sentry-conventions is parsed at build time into a PHF map of attributes. Additionally, we also hand-define a number of constants for attributes that are explicitly used in relay code. The only check we perform is whether these defined attributes actually exist in sentry-conventions, but we don't check e.g. deprecation status. This means we may continue to refer to attribute names even after they've been deprecated and replaced with new ones.
Ideal solution
To remedy this, I propose the following:
- Add a field for the attribute name to the
AttributeInfo struct.
- Instead of constructing a PHF map of attributes, at compile time, define a constant for each attribute in
sentry-conventions. The constant's name should be an uppercase variant of the name (exact format TBD) and the value should be the AttributeInfo.
- If the attribute is deprecated in
sentry-conventions, a #[deprecated] annotation should be placed on the constant.
- The implementation of
attribute_info should be changed from a PHF map lookup to a match on the name that maps each name to the appropriate constant. Consequently, this function would also need to be generated at compile time.
The effect of this would be that the deprecation of an attribute in sentry-conventions would immediately flag all uses of that attribute in relay with deprecation warnings, thus making sure one has to update the usage sites (either to replace the name or deliberately silence the warning).
Open question: If we replace the lookup by a match, can we keep the current logic for attributes with placeholders? This seems like it might make the implementation quite complicated.
Alternative solution
If removing the PHF map altogether is not feasible, we can also go with a less ambitious plan:
- At compile time, define a constant for each attribute in
sentry-conventions. The constant's name should be an uppercase variant of the name (exact format TBD) and the value should be the name.
- If the attribute is deprecated in
sentry-conventions, a #[deprecated] annotation should be placed on the constant.
- Everything else stays in place.
This gives us most of the benefit of the "ideal solution" (in particular, deprecation warnings) but sidesteps the question of the lookup logic. The downside is that it seems conceptually wasteful to have a constant for each attribute but then have to look up their properties in a map anyway.
Currently,
sentry-conventionsis parsed at build time into a PHF map of attributes. Additionally, we also hand-define a number of constants for attributes that are explicitly used in relay code. The only check we perform is whether these defined attributes actually exist insentry-conventions, but we don't check e.g. deprecation status. This means we may continue to refer to attribute names even after they've been deprecated and replaced with new ones.Ideal solution
To remedy this, I propose the following:
AttributeInfostruct.sentry-conventions. The constant's name should be an uppercase variant of the name (exact format TBD) and the value should be theAttributeInfo.sentry-conventions, a#[deprecated]annotation should be placed on the constant.attribute_infoshould be changed from a PHF map lookup to a match on the name that maps each name to the appropriate constant. Consequently, this function would also need to be generated at compile time.The effect of this would be that the deprecation of an attribute in
sentry-conventionswould immediately flag all uses of that attribute in relay with deprecation warnings, thus making sure one has to update the usage sites (either to replace the name or deliberately silence the warning).Open question: If we replace the lookup by a match, can we keep the current logic for attributes with placeholders? This seems like it might make the implementation quite complicated.
Alternative solution
If removing the PHF map altogether is not feasible, we can also go with a less ambitious plan:
sentry-conventions. The constant's name should be an uppercase variant of the name (exact format TBD) and the value should be the name.sentry-conventions, a#[deprecated]annotation should be placed on the constant.This gives us most of the benefit of the "ideal solution" (in particular, deprecation warnings) but sidesteps the question of the lookup logic. The downside is that it seems conceptually wasteful to have a constant for each attribute but then have to look up their properties in a map anyway.