Skip to content
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

Bug 1932320 - Don't record pings_submitted for custom pings #3010

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion glean-core/metrics.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -756,14 +756,17 @@ glean.validation:
pings_submitted:
type: labeled_counter
description: |
A count of the pings submitted, by ping type.
A count of the built-in pings submitted, by ping type.

This metric appears in both the metrics and baseline pings.

- On the metrics ping, the counts include the number of pings sent since
the last metrics ping (including the last metrics ping)
- On the baseline ping, the counts include the number of pings send since
the last baseline ping (including the last baseline ping)

Note: Previously this also recorded the number of submitted custom pings.
Now it only records counts for the Glean built-in pings.
send_in_pings:
- metrics
- baseline
Expand Down
15 changes: 10 additions & 5 deletions glean-core/src/metrics/ping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,15 +240,20 @@ impl PingType {
return false;
}

const BUILTIN_PINGS: [&str; 4] =
["baseline", "metrics", "events", "deletion-request"];

// This metric is recorded *after* the ping is collected (since
// that is the only way to know *if* it will be submitted). The
// implication of this is that the count for a metrics ping will
// be included in the *next* metrics ping.
glean
.additional_metrics
.pings_submitted
.get(ping.name)
.add_sync(glean, 1);
if BUILTIN_PINGS.contains(&ping.name) {
glean
.additional_metrics
.pings_submitted
.get(ping.name)
.add_sync(glean, 1);
}

if let Err(e) = ping_maker.store_ping(glean.get_data_path(), &ping) {
log::warn!(
Expand Down
27 changes: 27 additions & 0 deletions glean-core/tests/ping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,9 @@ fn test_pings_submitted_metric() {
let baseline_ping = PingType::new("baseline", true, false, true, true, true, vec![], vec![]);
glean.register_ping_type(&baseline_ping);

let custom_ping = PingType::new("custom", true, true, true, true, true, vec![], vec![]);
glean.register_ping_type(&custom_ping);

// We need to store a metric as an empty ping is not stored.
let counter = CounterMetric::new(CommonMetricData {
name: "counter".into(),
Expand All @@ -167,6 +170,8 @@ fn test_pings_submitted_metric() {
counter.add_sync(&glean, 1);

assert!(metrics_ping.submit_sync(&glean, None));
// A custom ping is just never recorded.
assert!(custom_ping.submit_sync(&glean, None));

// Check recording in the metrics ping
assert_eq!(
Expand All @@ -177,6 +182,10 @@ fn test_pings_submitted_metric() {
None,
pings_submitted.get("baseline").get_value(&glean, "metrics")
);
assert_eq!(
None,
pings_submitted.get("custom").get_value(&glean, "metrics")
);

// Check recording in the baseline ping
assert_eq!(
Expand All @@ -189,13 +198,19 @@ fn test_pings_submitted_metric() {
.get("baseline")
.get_value(&glean, "baseline")
);
assert_eq!(
None,
pings_submitted.get("custom").get_value(&glean, "baseline")
);

// Trigger 2 baseline pings.
// This should record a count of 2 baseline pings in the metrics ping, but
// it resets each time on the baseline ping, so we should only ever get 1
// baseline ping recorded in the baseline ping itsef.
assert!(baseline_ping.submit_sync(&glean, None));
assert!(baseline_ping.submit_sync(&glean, None));
// A custom ping is just never recorded.
assert!(custom_ping.submit_sync(&glean, None));

// Check recording in the metrics ping
assert_eq!(
Expand All @@ -210,6 +225,12 @@ fn test_pings_submitted_metric() {
.get("baseline")
.get_value(&glean, Some("metrics"))
);
assert_eq!(
None,
pings_submitted
.get("custom")
.get_value(&glean, Some("metrics"))
);

// Check recording in the baseline ping
assert_eq!(
Expand All @@ -224,6 +245,12 @@ fn test_pings_submitted_metric() {
.get("baseline")
.get_value(&glean, Some("baseline"))
);
assert_eq!(
None,
pings_submitted
.get("custom")
.get_value(&glean, Some("baseline"))
);
}

#[test]
Expand Down
Loading