From 0a5da4f9712f8912ec6b67e9ebc2919900424dc9 Mon Sep 17 00:00:00 2001 From: Travis Long Date: Mon, 9 Feb 2026 08:56:07 -0600 Subject: [PATCH 1/4] Bug2007406 - Add server knobs config to ping_info This adds the currently applied server knobs config to all pings in the ping_info section, or omits it if no server knobs config is applied. This should help to calculate effective sampling rates for metrics and pings that are being sampled in the client. --- .../metrics/experimenter-configuration.md | 35 +++++++++ .../user/server-knobs/other/max-events.md | 35 +++++++++ .../pings/experimenter-configuration.md | 35 +++++++++ glean-core/metrics.yaml | 38 ++++++++++ glean-core/src/core/mod.rs | 20 +++++ glean-core/src/ping/mod.rs | 75 +++++++++++++++++++ 6 files changed, 238 insertions(+) diff --git a/docs/user/user/server-knobs/metrics/experimenter-configuration.md b/docs/user/user/server-knobs/metrics/experimenter-configuration.md index 798db46ec7..7dae7d6a5c 100644 --- a/docs/user/user/server-knobs/metrics/experimenter-configuration.md +++ b/docs/user/user/server-knobs/metrics/experimenter-configuration.md @@ -21,3 +21,38 @@ This configuration would be what is entered into the branch configuration setup } } ``` + +## Server Knobs Configuration in Pings + +When Server Knobs configuration is applied through `applyServerKnobsConfig`, the entire configuration is automatically recorded as an `ObjectMetric` and included in the `ping_info` section of all pings. This makes it easier to identify which metrics are being controlled by Server Knobs and to calculate effective sampling rates in analysis. + +The configuration is stored using a standard `ObjectMetric` (at `glean.internal.server_knobs_config`), which provides schema definition support for downstream tooling and requires minimal changes to ingestion pipeline schemas. + +### How It Appears in Pings + +The complete Server Knobs configuration is included in `ping_info.server_knobs_config`: + +```json +{ + "ping_info": { + "seq": 123, + "start_time": "2024-01-01T00:00:00Z", + "end_time": "2024-01-01T01:00:00Z", + "server_knobs_config": { + "metrics_enabled": { + "urlbar.engagement": true, + "urlbar.impression": true + }, + "pings_enabled": {}, + "event_threshold": null + } + }, + "metrics": { + "counter": { + "urlbar.engagement": 5, + "urlbar.impression": 2 + } + } +} +``` + diff --git a/docs/user/user/server-knobs/other/max-events.md b/docs/user/user/server-knobs/other/max-events.md index 9d4b043ed3..d3487de6d3 100644 --- a/docs/user/user/server-knobs/other/max-events.md +++ b/docs/user/user/server-knobs/other/max-events.md @@ -14,3 +14,38 @@ For instance, if you wanted to disable batching in order to transmit an events p } } ``` + +## Server Knobs Configuration in Pings + +When Server Knobs configuration is applied through `applyServerKnobsConfig`, the entire configuration is automatically recorded as an `ObjectMetric` and included in the `ping_info` section of all pings. This makes it easier to identify which metrics are being controlled by Server Knobs and to calculate effective sampling rates in analysis. + +The configuration is stored using a standard `ObjectMetric` (at `glean.internal.server_knobs_config`), which provides schema definition support for downstream tooling and requires minimal changes to ingestion pipeline schemas. + +### How It Appears in Pings + +The complete Server Knobs configuration is included in `ping_info.server_knobs_config`: + +```json +{ + "ping_info": { + "seq": 123, + "start_time": "2024-01-01T00:00:00Z", + "end_time": "2024-01-01T01:00:00Z", + "server_knobs_config": { + "metrics_enabled": { + "urlbar.engagement": true, + "urlbar.impression": true + }, + "pings_enabled": {}, + "event_threshold": null + } + }, + "metrics": { + "counter": { + "urlbar.engagement": 5, + "urlbar.impression": 2 + } + } +} +``` + diff --git a/docs/user/user/server-knobs/pings/experimenter-configuration.md b/docs/user/user/server-knobs/pings/experimenter-configuration.md index 71591cdd9a..103909f4d3 100644 --- a/docs/user/user/server-knobs/pings/experimenter-configuration.md +++ b/docs/user/user/server-knobs/pings/experimenter-configuration.md @@ -19,3 +19,38 @@ This configuration would be what is entered into the branch configuration setup } } ``` + +## Server Knobs Configuration in Pings + +When Server Knobs configuration is applied through `applyServerKnobsConfig`, the entire configuration is automatically recorded as an `ObjectMetric` and included in the `ping_info` section of all pings. This makes it easier to identify which metrics are being controlled by Server Knobs and to calculate effective sampling rates in analysis. + +The configuration is stored using a standard `ObjectMetric` (at `glean.internal.server_knobs_config`), which provides schema definition support for downstream tooling and requires minimal changes to ingestion pipeline schemas. + +### How It Appears in Pings + +The complete Server Knobs configuration is included in `ping_info.server_knobs_config`: + +```json +{ + "ping_info": { + "seq": 123, + "start_time": "2024-01-01T00:00:00Z", + "end_time": "2024-01-01T01:00:00Z", + "server_knobs_config": { + "metrics_enabled": { + "urlbar.engagement": true, + "urlbar.impression": true + }, + "pings_enabled": {}, + "event_threshold": null + } + }, + "metrics": { + "counter": { + "urlbar.engagement": 5, + "urlbar.impression": 2 + } + } +} +``` + diff --git a/glean-core/metrics.yaml b/glean-core/metrics.yaml index 69785b2e5d..de274c2bcb 100644 --- a/glean-core/metrics.yaml +++ b/glean-core/metrics.yaml @@ -398,6 +398,44 @@ glean.internal.metrics: - glean-team@mozilla.com expires: never + server_knobs_config: + type: object + lifetime: application + send_in_pings: + - glean_internal_info + description: | + The Server Knobs configuration applied via applyServerKnobsConfig. + This contains the complete configuration including metrics_enabled, + pings_enabled, and event_threshold settings that control metric + recording behavior through remote configuration. + bugs: + - https://bugzilla.mozilla.org/show_bug.cgi?id=2007406 + data_reviews: + - https://bugzilla.mozilla.org/show_bug.cgi?id=2007406 + data_sensitivity: + - technical + notification_emails: + - glean-team@mozilla.com + expires: never + structure: + type: object + properties: + metrics_enabled: + type: object + description: | + Map of metric identifiers (category.name) to boolean values + indicating whether the metric is enabled. + pings_enabled: + type: object + description: | + Map of ping names to boolean values indicating whether + the ping is enabled. + event_threshold: + type: integer + description: | + Optional threshold for event buffering before an events + ping is collected and submitted. Can be null if not set. + glean.internal.metrics.attribution: source: diff --git a/glean-core/src/core/mod.rs b/glean-core/src/core/mod.rs index 1f50f50e48..c160db6f1b 100644 --- a/glean-core/src/core/mod.rs +++ b/glean-core/src/core/mod.rs @@ -1113,6 +1113,17 @@ impl Glean { .get_value(self, None) } + /// Returns the ObjectMetric used to store Server Knobs configuration. + pub(crate) fn server_knobs_metric() -> metrics::ObjectMetric { + metrics::ObjectMetric::new(CommonMetricData { + name: "server_knobs_config".into(), + category: "glean.internal".into(), + send_in_pings: vec![INTERNAL_STORAGE.into()], + lifetime: Lifetime::Application, + ..Default::default() + }) + } + /// Set configuration to override the default state, typically initiated from a /// remote_settings experiment or rollout /// @@ -1136,6 +1147,15 @@ impl Glean { remote_settings_config.event_threshold = cfg.event_threshold; + // Store the Server Knobs configuration as an ObjectMetric + // This allows it to be included in pings automatically + let config_clone = remote_settings_config.clone(); + drop(remote_settings_config); // Release lock before storage operation + + // Store the configuration using the server knobs ObjectMetric + let server_knobs_metric = Self::server_knobs_metric(); + server_knobs_metric.set_sync(self, serde_json::to_value(&config_clone).unwrap()); + // Update remote_settings epoch self.remote_settings_epoch.fetch_add(1, Ordering::SeqCst); } diff --git a/glean-core/src/ping/mod.rs b/glean-core/src/ping/mod.rs index f5d3725476..5f00bd82ae 100644 --- a/glean-core/src/ping/mod.rs +++ b/glean-core/src/ping/mod.rs @@ -162,6 +162,15 @@ impl PingMaker { .insert("experiments".to_string(), experiment_data); }; + // Get the Server Knobs configuration, if available. + let server_knobs_metric = crate::Glean::server_knobs_metric(); + if let Some(config_json) = server_knobs_metric.get_value(glean, INTERNAL_STORAGE) { + let server_knobs_config = serde_json::from_str(&config_json).unwrap(); + map.as_object_mut() + .unwrap() // safe unwrap, we created the object above + .insert("server_knobs_config".to_string(), server_knobs_config); + } + map } @@ -533,4 +542,70 @@ mod test { assert_eq!(0, ping_maker.get_ping_seq(&glean, "store1")); assert_eq!(1, ping_maker.get_ping_seq(&glean, "store1")); } + + #[test] + fn test_server_knobs_config_appears_in_ping_info() { + use crate::metrics::RemoteSettingsConfig; + use std::collections::HashMap; + + let (glean, _t) = new_glean(None); + + // Apply complete Server Knobs config with all three fields + let mut metrics_enabled = HashMap::new(); + metrics_enabled.insert("test.counter".to_string(), true); + + let mut pings_enabled = HashMap::new(); + pings_enabled.insert("custom".to_string(), false); + + let config = RemoteSettingsConfig { + metrics_enabled, + pings_enabled, + event_threshold: Some(10), + }; + glean.apply_server_knobs_config(config); + + // Verify complete config structure appears in ping_info + let ping_maker = PingMaker::new(); + let ping_info = ping_maker.get_ping_info(&glean, "store1", None, TimeUnit::Minute); + + let server_knobs = &ping_info["server_knobs_config"]; + assert_eq!(server_knobs["metrics_enabled"]["test.counter"], true); + assert_eq!(server_knobs["pings_enabled"]["custom"], false); + assert_eq!(server_knobs["event_threshold"], 10); + } + + #[test] + fn test_server_knobs_not_included_when_no_config() { + let (glean, _t) = new_glean(None); + + let ping_maker = PingMaker::new(); + let ping_info = ping_maker.get_ping_info(&glean, "store1", None, TimeUnit::Minute); + + assert!(ping_info.get("server_knobs_config").is_none()); + } + + #[test] + fn test_server_knobs_appears_in_all_pings() { + use crate::metrics::RemoteSettingsConfig; + use std::collections::HashMap; + + let (glean, _t) = new_glean(None); + + let mut metrics_enabled = HashMap::new(); + metrics_enabled.insert("test.counter".to_string(), true); + + let config = RemoteSettingsConfig { + metrics_enabled, + ..Default::default() + }; + glean.apply_server_knobs_config(config); + + // Verify config appears in multiple different pings + let ping_maker = PingMaker::new(); + let ping_info1 = ping_maker.get_ping_info(&glean, "store1", None, TimeUnit::Minute); + let ping_info2 = ping_maker.get_ping_info(&glean, "store2", None, TimeUnit::Minute); + + assert_eq!(ping_info1["server_knobs_config"]["metrics_enabled"]["test.counter"], true); + assert_eq!(ping_info2["server_knobs_config"]["metrics_enabled"]["test.counter"], true); + } } From 7706a257709938ffc953713990d5f0cab33eaeb3 Mon Sep 17 00:00:00 2001 From: Travis Long Date: Fri, 13 Feb 2026 12:50:46 -0600 Subject: [PATCH 2/4] Define server_knobs_config metric in internal_metrics.rs --- glean-core/metrics.yaml | 12 +++++++++++- glean-core/src/core/mod.rs | 16 +++------------- glean-core/src/internal_metrics.rs | 12 ++++++++++++ glean-core/src/ping/mod.rs | 17 +++++++++++++---- 4 files changed, 39 insertions(+), 18 deletions(-) diff --git a/glean-core/metrics.yaml b/glean-core/metrics.yaml index de274c2bcb..0f4b8aac5a 100644 --- a/glean-core/metrics.yaml +++ b/glean-core/metrics.yaml @@ -425,13 +425,23 @@ glean.internal.metrics: description: | Map of metric identifiers (category.name) to boolean values indicating whether the metric is enabled. + properties: + key: + type: string + value: + type: boolean pings_enabled: type: object description: | Map of ping names to boolean values indicating whether the ping is enabled. + properties: + key: + type: string + value: + type: boolean event_threshold: - type: integer + type: number description: | Optional threshold for event buffering before an events ping is collected and submitted. Can be null if not set. diff --git a/glean-core/src/core/mod.rs b/glean-core/src/core/mod.rs index c160db6f1b..d68481c647 100644 --- a/glean-core/src/core/mod.rs +++ b/glean-core/src/core/mod.rs @@ -1113,17 +1113,6 @@ impl Glean { .get_value(self, None) } - /// Returns the ObjectMetric used to store Server Knobs configuration. - pub(crate) fn server_knobs_metric() -> metrics::ObjectMetric { - metrics::ObjectMetric::new(CommonMetricData { - name: "server_knobs_config".into(), - category: "glean.internal".into(), - send_in_pings: vec![INTERNAL_STORAGE.into()], - lifetime: Lifetime::Application, - ..Default::default() - }) - } - /// Set configuration to override the default state, typically initiated from a /// remote_settings experiment or rollout /// @@ -1153,8 +1142,9 @@ impl Glean { drop(remote_settings_config); // Release lock before storage operation // Store the configuration using the server knobs ObjectMetric - let server_knobs_metric = Self::server_knobs_metric(); - server_knobs_metric.set_sync(self, serde_json::to_value(&config_clone).unwrap()); + self.additional_metrics + .server_knobs_config + .set_sync(self, serde_json::to_value(&config_clone).unwrap()); // Update remote_settings epoch self.remote_settings_epoch.fetch_add(1, Ordering::SeqCst); diff --git a/glean-core/src/internal_metrics.rs b/glean-core/src/internal_metrics.rs index eef6476acf..fd46c642b1 100644 --- a/glean-core/src/internal_metrics.rs +++ b/glean-core/src/internal_metrics.rs @@ -44,6 +44,9 @@ pub struct AdditionalMetrics { /// The number of times we had to clamp an event timestamp /// for exceeding the range of a signed 64-bit integer (9223372036854775807). pub event_timestamp_clamped: CounterMetric, + + /// Server knobs configuration received from remote settings. + pub server_knobs_config: ObjectMetric, } impl CoreMetrics { @@ -211,6 +214,15 @@ impl AdditionalMetrics { disabled: false, dynamic_label: None, }), + + server_knobs_config: ObjectMetric::new(CommonMetricData { + name: "server_knobs_config".into(), + category: "glean.internal.metrics".into(), + send_in_pings: vec!["glean_internal_info".into()], + lifetime: Lifetime::Application, + disabled: false, + dynamic_label: None, + }), } } } diff --git a/glean-core/src/ping/mod.rs b/glean-core/src/ping/mod.rs index 5f00bd82ae..aeee39f3cd 100644 --- a/glean-core/src/ping/mod.rs +++ b/glean-core/src/ping/mod.rs @@ -163,8 +163,11 @@ impl PingMaker { }; // Get the Server Knobs configuration, if available. - let server_knobs_metric = crate::Glean::server_knobs_metric(); - if let Some(config_json) = server_knobs_metric.get_value(glean, INTERNAL_STORAGE) { + if let Some(config_json) = glean + .additional_metrics + .server_knobs_config + .get_value(glean, INTERNAL_STORAGE) + { let server_knobs_config = serde_json::from_str(&config_json).unwrap(); map.as_object_mut() .unwrap() // safe unwrap, we created the object above @@ -605,7 +608,13 @@ mod test { let ping_info1 = ping_maker.get_ping_info(&glean, "store1", None, TimeUnit::Minute); let ping_info2 = ping_maker.get_ping_info(&glean, "store2", None, TimeUnit::Minute); - assert_eq!(ping_info1["server_knobs_config"]["metrics_enabled"]["test.counter"], true); - assert_eq!(ping_info2["server_knobs_config"]["metrics_enabled"]["test.counter"], true); + assert_eq!( + ping_info1["server_knobs_config"]["metrics_enabled"]["test.counter"], + true + ); + assert_eq!( + ping_info2["server_knobs_config"]["metrics_enabled"]["test.counter"], + true + ); } } From bf9abe6fa3c0bc832ec3fd507cfc28bb0391bdb5 Mon Sep 17 00:00:00 2001 From: Travis Long Date: Thu, 12 Mar 2026 09:55:42 -0500 Subject: [PATCH 3/4] Address PR review feedback for server knobs config in ping_info - Remove clone() by serializing directly from MutexGuard ref - Add safety comments for all unwrap() calls - Skip serializing empty maps and null event_threshold to reduce ping size - Extract shared docs into an include file to avoid duplication - Fix metric category in docs to glean.internal.metrics - Add "except metadata.include_info_sections: false" qualifier to docs - Change test event_threshold to non-default value (41) - Add test verifying empty fields are omitted from serialized config - Remove accidentally committed uv.lock --- .../_includes/server-knobs-config-in-pings.md | 33 +++++++++++++++++ .../metrics/experimenter-configuration.md | 35 +------------------ .../user/server-knobs/other/max-events.md | 35 +------------------ .../pings/experimenter-configuration.md | 35 +------------------ glean-core/src/core/mod.rs | 11 +++--- .../src/metrics/remote_settings_config.rs | 6 ++-- glean-core/src/ping/mod.rs | 34 ++++++++++++++++-- 7 files changed, 76 insertions(+), 113 deletions(-) create mode 100644 docs/user/_includes/server-knobs-config-in-pings.md diff --git a/docs/user/_includes/server-knobs-config-in-pings.md b/docs/user/_includes/server-knobs-config-in-pings.md new file mode 100644 index 0000000000..cb3230cc06 --- /dev/null +++ b/docs/user/_includes/server-knobs-config-in-pings.md @@ -0,0 +1,33 @@ +## Server Knobs Configuration in Pings + +When Server Knobs configuration is applied through `applyServerKnobsConfig`, the entire configuration is automatically recorded as an `ObjectMetric` and included in the `ping_info` section of all pings except for those with `metadata.include_info_sections: false`. + +This makes it easier to identify which metrics are being controlled by Server Knobs and to calculate effective sampling rates in analysis. + +The configuration is stored using a standard `ObjectMetric` (at `glean.internal.metrics.server_knobs_config`), which provides schema definition support for downstream tooling and requires minimal changes to ingestion pipeline schemas. + +### How It Appears in Pings + +The complete Server Knobs configuration is included in `ping_info.server_knobs_config`: + +```json +{ + "ping_info": { + "seq": 123, + "start_time": "2024-01-01T00:00:00Z", + "end_time": "2024-01-01T01:00:00Z", + "server_knobs_config": { + "metrics_enabled": { + "urlbar.engagement": true, + "urlbar.impression": true + } + } + }, + "metrics": { + "counter": { + "urlbar.engagement": 5, + "urlbar.impression": 2 + } + } +} +``` diff --git a/docs/user/user/server-knobs/metrics/experimenter-configuration.md b/docs/user/user/server-knobs/metrics/experimenter-configuration.md index 7dae7d6a5c..b6451054d2 100644 --- a/docs/user/user/server-knobs/metrics/experimenter-configuration.md +++ b/docs/user/user/server-knobs/metrics/experimenter-configuration.md @@ -22,37 +22,4 @@ This configuration would be what is entered into the branch configuration setup } ``` -## Server Knobs Configuration in Pings - -When Server Knobs configuration is applied through `applyServerKnobsConfig`, the entire configuration is automatically recorded as an `ObjectMetric` and included in the `ping_info` section of all pings. This makes it easier to identify which metrics are being controlled by Server Knobs and to calculate effective sampling rates in analysis. - -The configuration is stored using a standard `ObjectMetric` (at `glean.internal.server_knobs_config`), which provides schema definition support for downstream tooling and requires minimal changes to ingestion pipeline schemas. - -### How It Appears in Pings - -The complete Server Knobs configuration is included in `ping_info.server_knobs_config`: - -```json -{ - "ping_info": { - "seq": 123, - "start_time": "2024-01-01T00:00:00Z", - "end_time": "2024-01-01T01:00:00Z", - "server_knobs_config": { - "metrics_enabled": { - "urlbar.engagement": true, - "urlbar.impression": true - }, - "pings_enabled": {}, - "event_threshold": null - } - }, - "metrics": { - "counter": { - "urlbar.engagement": 5, - "urlbar.impression": 2 - } - } -} -``` - +{{#include ../../../_includes/server-knobs-config-in-pings.md}} diff --git a/docs/user/user/server-knobs/other/max-events.md b/docs/user/user/server-knobs/other/max-events.md index d3487de6d3..7dff480571 100644 --- a/docs/user/user/server-knobs/other/max-events.md +++ b/docs/user/user/server-knobs/other/max-events.md @@ -15,37 +15,4 @@ For instance, if you wanted to disable batching in order to transmit an events p } ``` -## Server Knobs Configuration in Pings - -When Server Knobs configuration is applied through `applyServerKnobsConfig`, the entire configuration is automatically recorded as an `ObjectMetric` and included in the `ping_info` section of all pings. This makes it easier to identify which metrics are being controlled by Server Knobs and to calculate effective sampling rates in analysis. - -The configuration is stored using a standard `ObjectMetric` (at `glean.internal.server_knobs_config`), which provides schema definition support for downstream tooling and requires minimal changes to ingestion pipeline schemas. - -### How It Appears in Pings - -The complete Server Knobs configuration is included in `ping_info.server_knobs_config`: - -```json -{ - "ping_info": { - "seq": 123, - "start_time": "2024-01-01T00:00:00Z", - "end_time": "2024-01-01T01:00:00Z", - "server_knobs_config": { - "metrics_enabled": { - "urlbar.engagement": true, - "urlbar.impression": true - }, - "pings_enabled": {}, - "event_threshold": null - } - }, - "metrics": { - "counter": { - "urlbar.engagement": 5, - "urlbar.impression": 2 - } - } -} -``` - +{{#include ../../../_includes/server-knobs-config-in-pings.md}} diff --git a/docs/user/user/server-knobs/pings/experimenter-configuration.md b/docs/user/user/server-knobs/pings/experimenter-configuration.md index 103909f4d3..cae178667d 100644 --- a/docs/user/user/server-knobs/pings/experimenter-configuration.md +++ b/docs/user/user/server-knobs/pings/experimenter-configuration.md @@ -20,37 +20,4 @@ This configuration would be what is entered into the branch configuration setup } ``` -## Server Knobs Configuration in Pings - -When Server Knobs configuration is applied through `applyServerKnobsConfig`, the entire configuration is automatically recorded as an `ObjectMetric` and included in the `ping_info` section of all pings. This makes it easier to identify which metrics are being controlled by Server Knobs and to calculate effective sampling rates in analysis. - -The configuration is stored using a standard `ObjectMetric` (at `glean.internal.server_knobs_config`), which provides schema definition support for downstream tooling and requires minimal changes to ingestion pipeline schemas. - -### How It Appears in Pings - -The complete Server Knobs configuration is included in `ping_info.server_knobs_config`: - -```json -{ - "ping_info": { - "seq": 123, - "start_time": "2024-01-01T00:00:00Z", - "end_time": "2024-01-01T01:00:00Z", - "server_knobs_config": { - "metrics_enabled": { - "urlbar.engagement": true, - "urlbar.impression": true - }, - "pings_enabled": {}, - "event_threshold": null - } - }, - "metrics": { - "counter": { - "urlbar.engagement": 5, - "urlbar.impression": 2 - } - } -} -``` - +{{#include ../../../_includes/server-knobs-config-in-pings.md}} diff --git a/glean-core/src/core/mod.rs b/glean-core/src/core/mod.rs index d68481c647..4fc612dcc0 100644 --- a/glean-core/src/core/mod.rs +++ b/glean-core/src/core/mod.rs @@ -1137,14 +1137,13 @@ impl Glean { remote_settings_config.event_threshold = cfg.event_threshold; // Store the Server Knobs configuration as an ObjectMetric - // This allows it to be included in pings automatically - let config_clone = remote_settings_config.clone(); - drop(remote_settings_config); // Release lock before storage operation - - // Store the configuration using the server knobs ObjectMetric + // Since RemoteSettingsConfig only contains maps with string keys and primitives, + // serialization via the derived Serialize impl cannot fail so it is safe to unwrap. + let config_value = serde_json::to_value(&*remote_settings_config).unwrap(); + drop(remote_settings_config); self.additional_metrics .server_knobs_config - .set_sync(self, serde_json::to_value(&config_clone).unwrap()); + .set_sync(self, config_value); // Update remote_settings epoch self.remote_settings_epoch.fetch_add(1, Ordering::SeqCst); diff --git a/glean-core/src/metrics/remote_settings_config.rs b/glean-core/src/metrics/remote_settings_config.rs index ae0fbca8cc..1c7c4c5c09 100644 --- a/glean-core/src/metrics/remote_settings_config.rs +++ b/glean-core/src/metrics/remote_settings_config.rs @@ -26,19 +26,19 @@ pub struct RemoteSettingsConfig { /// If a particular metric has a value of `true` here, it means /// the default of the metric will be overriden and set to the /// enabled state. - #[serde(default)] + #[serde(default, skip_serializing_if = "HashMap::is_empty")] pub metrics_enabled: HashMap, /// This is a `HashMap` consisting of ping names as keys and /// boolean values representing on override for the default /// enabled state of the ping of the same name. - #[serde(default)] + #[serde(default, skip_serializing_if = "HashMap::is_empty")] pub pings_enabled: HashMap, /// The threshold of events that will be buffered before an events ping is /// collected and submitted. /// It overrides the value configured at initialization time. - #[serde(default)] + #[serde(default, skip_serializing_if = "Option::is_none")] pub event_threshold: Option, } diff --git a/glean-core/src/ping/mod.rs b/glean-core/src/ping/mod.rs index aeee39f3cd..ce21b26e0d 100644 --- a/glean-core/src/ping/mod.rs +++ b/glean-core/src/ping/mod.rs @@ -168,6 +168,8 @@ impl PingMaker { .server_knobs_config .get_value(glean, INTERNAL_STORAGE) { + // Object metrics always hold a string produced by serde_json::to_string, + // so deserializing it back into a JsonValue cannot fail. let server_knobs_config = serde_json::from_str(&config_json).unwrap(); map.as_object_mut() .unwrap() // safe unwrap, we created the object above @@ -563,7 +565,7 @@ mod test { let config = RemoteSettingsConfig { metrics_enabled, pings_enabled, - event_threshold: Some(10), + event_threshold: Some(41), }; glean.apply_server_knobs_config(config); @@ -574,7 +576,7 @@ mod test { let server_knobs = &ping_info["server_knobs_config"]; assert_eq!(server_knobs["metrics_enabled"]["test.counter"], true); assert_eq!(server_knobs["pings_enabled"]["custom"], false); - assert_eq!(server_knobs["event_threshold"], 10); + assert_eq!(server_knobs["event_threshold"], 41); } #[test] @@ -617,4 +619,32 @@ mod test { true ); } + + #[test] + fn test_server_knobs_config_omits_empty_fields() { + use crate::metrics::RemoteSettingsConfig; + use std::collections::HashMap; + + let (glean, _t) = new_glean(None); + + // Apply config with only metrics_enabled set; pings_enabled and event_threshold are empty/None + let mut metrics_enabled = HashMap::new(); + metrics_enabled.insert("test.counter".to_string(), true); + + let config = RemoteSettingsConfig { + metrics_enabled, + ..Default::default() + }; + glean.apply_server_knobs_config(config); + + let ping_maker = PingMaker::new(); + let ping_info = ping_maker.get_ping_info(&glean, "store1", None, TimeUnit::Minute); + + let server_knobs = &ping_info["server_knobs_config"]; + // metrics_enabled should be present + assert_eq!(server_knobs["metrics_enabled"]["test.counter"], true); + // pings_enabled and event_threshold should be absent (not empty object / null) + assert!(server_knobs.get("pings_enabled").is_none()); + assert!(server_knobs.get("event_threshold").is_none()); + } } From 39d764f6e10d58c4f419669c8fdc57a2a624617d Mon Sep 17 00:00:00 2001 From: Travis Long Date: Thu, 2 Apr 2026 14:30:38 -0500 Subject: [PATCH 4/4] Final change requests --- CHANGELOG.md | 1 + glean-core/src/core/mod.rs | 44 ++++++++++++++++++++------------------ 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 507e39b201..8f07f3f509 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ * General * Updated to `glean_parser` v19.0.0 ([#3421](https://github.com/mozilla/glean/issues/3421)) + * Add server knobs config to `ping_info` ([#3396](https://github.com/mozilla/glean/pull/3396)) # v67.1.0 (2026-03-12) diff --git a/glean-core/src/core/mod.rs b/glean-core/src/core/mod.rs index 4fc612dcc0..e38798ffb4 100644 --- a/glean-core/src/core/mod.rs +++ b/glean-core/src/core/mod.rs @@ -1120,27 +1120,29 @@ impl Glean { /// /// * `cfg` - The stringified JSON representation of a `RemoteSettingsConfig` object pub fn apply_server_knobs_config(&self, cfg: RemoteSettingsConfig) { - // Set the current RemoteSettingsConfig, keeping the lock until the epoch is - // updated to prevent against reading a "new" config but an "old" epoch - let mut remote_settings_config = self.remote_settings_config.lock().unwrap(); - - // Merge the exising metrics configuration with the supplied one - remote_settings_config - .metrics_enabled - .extend(cfg.metrics_enabled); - - // Merge the exising ping configuration with the supplied one - remote_settings_config - .pings_enabled - .extend(cfg.pings_enabled); - - remote_settings_config.event_threshold = cfg.event_threshold; - - // Store the Server Knobs configuration as an ObjectMetric - // Since RemoteSettingsConfig only contains maps with string keys and primitives, - // serialization via the derived Serialize impl cannot fail so it is safe to unwrap. - let config_value = serde_json::to_value(&*remote_settings_config).unwrap(); - drop(remote_settings_config); + let config_value = { + // Hold the lock while merging config and serializing, then release + // before performing IO in set_sync. + let mut remote_settings_config = self.remote_settings_config.lock().unwrap(); + + // Merge the exising metrics configuration with the supplied one + remote_settings_config + .metrics_enabled + .extend(cfg.metrics_enabled); + + // Merge the exising ping configuration with the supplied one + remote_settings_config + .pings_enabled + .extend(cfg.pings_enabled); + + remote_settings_config.event_threshold = cfg.event_threshold; + + // Store the Server Knobs configuration as an ObjectMetric + // Since RemoteSettingsConfig only contains maps with string keys and primitives, + // serialization via the derived Serialize impl cannot fail so it is safe to unwrap. + serde_json::to_value(&*remote_settings_config).unwrap() + }; + self.additional_metrics .server_knobs_config .set_sync(self, config_value);