feat(config): Only propagate known feature flags (#2256)

This PR
* reverts #2040,
* ensures that feature flags are only propagated to downstream Relays if
they are known,
* removes unknown features on deserialization,
* fixes a bug in #2250, which did
not deserialize a new feature flag correctly.

Author: jjbayer

relay-dynamic-config/src/feature.rs

@@ -1,60 +1,79 @@
-use std::borrow::Cow;
+use std::collections::BTreeSet;
 
 use serde::{Deserialize, Serialize};
 
 /// Features exposed by project config.
-#[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash)]
+#[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
 pub enum Feature {
     /// Enables ingestion of Session Replays (Replay Recordings and Replay Events).
+    #[serde(rename = "organizations:session-replay")]
     SessionReplay,
     /// Enables data scrubbing of replay recording payloads.
+    #[serde(rename = "organizations:session-replay-recording-scrubbing")]
     SessionReplayRecordingScrubbing,
     /// Enables device.class synthesis
     ///
     /// Enables device.class tag synthesis on mobile events.
+    #[serde(rename = "organizations:device-class-synthesis")]
     DeviceClassSynthesis,
     /// Enables metric extraction from spans.
+    #[serde(rename = "projects:span-metrics-extraction")]
     SpanMetricsExtraction,
     /// Apply transaction normalization rules to transactions from legacy SDKs.
+    #[serde(rename = "organizations:transaction-name-normalize-legacy")]
     NormalizeLegacyTransactions,
+
+    /// Deprecated, still forwarded for older downstream Relays.
+    #[serde(rename = "organizations:transaction-name-mark-scrubbed-as-sanitized")]
+    Deprecated1,
+    /// Deprecated, still forwarded for older downstream Relays.
+    #[serde(rename = "organizations:transaction-name-normalize")]
+    Deprecated2,
+    /// Deprecated, still forwarded for older downstream Relays.
+    #[serde(rename = "organizations:profiling")]
+    Deprecated3,
+
     /// Forward compatibility.
-    Unknown(String),
+    #[serde(other)]
+    Unknown,
+}
+
+/// A set of [`Feature`]s.
+#[derive(Clone, Debug, Default, Serialize, PartialEq, Eq)]
+pub struct FeatureSet(pub BTreeSet<Feature>);
+
+impl FeatureSet {
+    /// Returns `true` if the set of features is empty.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
 }
 
-impl<'de> Deserialize<'de> for Feature {
+impl<'de> Deserialize<'de> for FeatureSet {
     fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
     where
         D: serde::Deserializer<'de>,
     {
-        let feature_name = Cow::<str>::deserialize(deserializer)?;
-        Ok(match feature_name.as_ref() {
-            "organizations:session-replay" => Feature::SessionReplay,
-            "organizations:session-replay-recording-scrubbing" => {
-                Feature::SessionReplayRecordingScrubbing
-            }
-            "organizations:device-class-synthesis" => Feature::DeviceClassSynthesis,
-            "projects:span-metrics-extraction" => Feature::SpanMetricsExtraction,
-            _ => Feature::Unknown(feature_name.to_string()),
-        })
+        let mut set = BTreeSet::<Feature>::deserialize(deserializer)?;
+        set.remove(&Feature::Unknown);
+        Ok(Self(set))
     }
 }
+#[cfg(test)]
+mod tests {
+    use super::*;
 
-impl Serialize for Feature {
-    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-    where
-        S: serde::Serializer,
-    {
-        serializer.serialize_str(match self {
-            Feature::SessionReplay => "organizations:session-replay",
-            Feature::SessionReplayRecordingScrubbing => {
-                "organizations:session-replay-recording-scrubbing"
-            }
-            Feature::DeviceClassSynthesis => "organizations:device-class-synthesis",
-            Feature::SpanMetricsExtraction => "projects:span-metrics-extraction",
-            Feature::NormalizeLegacyTransactions => {
-                "organizations:transaction-name-normalize-legacy"
-            }
-            Feature::Unknown(s) => s,
-        })
+    #[test]
+    fn roundtrip() {
+        let features: FeatureSet =
+            serde_json::from_str(r#"["organizations:session-replay", "foo"]"#).unwrap();
+        assert_eq!(
+            &features,
+            &FeatureSet(BTreeSet::from([Feature::SessionReplay]))
+        );
+        assert_eq!(
+            serde_json::to_string(&features).unwrap(),
+            r#"["organizations:session-replay"]"#
+        );
     }
 }

relay-dynamic-config/src/project.rs

@@ -12,11 +12,10 @@ use relay_sampling::SamplingConfig;
 use serde::{Deserialize, Serialize};
 use serde_json::Value;
 
-use crate::feature::Feature;
 use crate::metrics::{
     MetricExtractionConfig, SessionMetricsConfig, TaggingRule, TransactionMetricsConfig,
 };
-use crate::ErrorBoundary;
+use crate::{ErrorBoundary, FeatureSet};
 
 /// Dynamic, per-DSN configuration passed down from Sentry.
 #[derive(Debug, Clone, Serialize, Deserialize)]
@@ -68,8 +67,8 @@ pub struct ProjectConfig {
     #[serde(skip_serializing_if = "Vec::is_empty")]
     pub metric_conditional_tagging: Vec<TaggingRule>,
     /// Exposable features enabled for this project.
-    #[serde(skip_serializing_if = "BTreeSet::is_empty")]
-    pub features: BTreeSet<Feature>,
+    #[serde(skip_serializing_if = "FeatureSet::is_empty")]
+    pub features: FeatureSet,
     /// Transaction renaming rules.
     #[serde(skip_serializing_if = "Vec::is_empty")]
     pub tx_name_rules: Vec<TransactionNameRule>,
@@ -100,7 +99,7 @@ impl Default for ProjectConfig {
             metric_extraction: Default::default(),
             span_attributes: BTreeSet::new(),
             metric_conditional_tagging: Vec::new(),
-            features: BTreeSet::new(),
+            features: Default::default(),
             tx_name_rules: Vec::new(),
             tx_name_ready: false,
             span_description_rules: None,
@@ -143,8 +142,8 @@ pub struct LimitedProjectConfig {
     pub measurements: Option<MeasurementsConfig>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub breakdowns_v2: Option<BreakdownsConfig>,
-    #[serde(skip_serializing_if = "BTreeSet::is_empty")]
-    pub features: BTreeSet<Feature>,
+    #[serde(skip_serializing_if = "FeatureSet::is_empty")]
+    pub features: FeatureSet,
     #[serde(skip_serializing_if = "Vec::is_empty")]
     pub tx_name_rules: Vec<TransactionNameRule>,
     /// Whether or not a project is ready to mark all URL transactions as "sanitized".

relay-dynamic-config/src/utils.rs

@@ -62,10 +62,20 @@ mod tests {
                 "json atom at path \".b.other\" is missing from rhs",
             ),
             (
-                // An unknown feature flag for this relay is still serialized
-                // for the following relays in chain.
+                // An unknown feature flag will not be serialized by Relay.
                 r#"{"a": 1, "features": ["organizations:is-cool"]}"#,
                 true,
+                r#"json atoms at path ".features[0]" are not equal:
+    lhs:
+        "organizations:is-cool"
+    rhs:
+        "Unknown""#,
+            ),
+            (
+                // A deprecated feature flag for this relay is still serialized
+                // for the following relays in chain.
+                r#"{"a": 1, "features": ["organizations:profiling"]}"#,
+                true,
                 "",
             ),
         ] {

relay-server/src/actors/processor.rs

@@ -2097,9 +2097,9 @@ impl EnvelopeProcessorService {
             return Ok(());
         }
 
-        let extract_spans_metrics = project_config
-            .features
-            .contains(&Feature::SpanMetricsExtraction);
+        let extract_spans_metrics = state
+            .project_state
+            .has_feature(Feature::SpanMetricsExtraction);
 
         let transaction_from_dsc = state
             .managed_envelope

relay-server/src/actors/project.rs

@@ -336,7 +336,7 @@ impl ProjectState {
     }
 
     pub fn has_feature(&self, feature: Feature) -> bool {
-        self.config.features.contains(&feature)
+        self.config.features.0.contains(&feature)
     }
 }