-
Notifications
You must be signed in to change notification settings - Fork 159
Bug2007406 - Add server knobs config to ping_info #3396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,3 +19,38 @@ This configuration would be what is entered into the branch configuration setup | |
| } | ||
| } | ||
| ``` | ||
|
|
||
| ## Server Knobs Configuration in Pings | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, so it's literally the same. Any reason you didn't write this as an include, like label format? |
||
|
|
||
| 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is incorrect. The category is |
||
|
|
||
| ### 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 | ||
|
Comment on lines
+44
to
+45
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it's going to be in every
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (and test it?) |
||
| } | ||
| }, | ||
| "metrics": { | ||
| "counter": { | ||
| "urlbar.engagement": 5, | ||
| "urlbar.impression": 2 | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -398,6 +398,54 @@ 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: | | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh right, we support per-field descriptions now! |
||
| 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: number | ||
| 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: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1136,6 +1136,16 @@ 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 | ||
| self.additional_metrics | ||
| .server_knobs_config | ||
| .set_sync(self, serde_json::to_value(&config_clone).unwrap()); | ||
|
Comment on lines
+1141
to
+1147
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Forgive me for the premature optimization here... but if Also, there are |
||
|
|
||
| // Update remote_settings epoch | ||
| self.remote_settings_epoch.fetch_add(1, Ordering::SeqCst); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -162,6 +162,18 @@ impl PingMaker { | |||||
| .insert("experiments".to_string(), experiment_data); | ||||||
| }; | ||||||
|
|
||||||
| // Get the Server Knobs configuration, if available. | ||||||
| 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(); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this safe to unwrap? |
||||||
| map.as_object_mut() | ||||||
| .unwrap() // safe unwrap, we created the object above | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only safe if the saved JSON was successfully parsed to an object not a primitive or array. |
||||||
| .insert("server_knobs_config".to_string(), server_knobs_config); | ||||||
| } | ||||||
|
|
||||||
| map | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -533,4 +545,76 @@ 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), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| }; | ||||||
| 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 | ||||||
| ); | ||||||
| } | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All pings... beside those with
metadata.include_info_sections: false, correct? (( Has "All but your four fastest ships" energy ))