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

Add a new flag to pings: send_if_empty #528

Merged
merged 6 commits into from
Nov 28, 2019
Merged
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
[Full changelog](https://github.com/mozilla/glean/compare/v21.2.0...master)

* Timers are reset when disabled. That avoids recording timespans across disabled/enabled toggling.
* Add a new flag to pings: `send_if_empty`
* Upgrade `glean_parser` to v1.12.0`

# v21.2.0 (2019-11-21)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,16 @@ import mozilla.telemetry.glean.rust.toByte
*/
class PingType(
internal val name: String,
includeClientId: Boolean
includeClientId: Boolean,
sendIfEmpty: Boolean
) {
internal var handle: Long

init {
this.handle = LibGleanFFI.INSTANCE.glean_new_ping_type(
name = name,
include_client_id = includeClientId.toByte()
include_client_id = includeClientId.toByte(),
send_if_empty = sendIfEmpty.toByte()
)
Glean.registerPingType(this)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ internal interface LibGleanFFI : Library {

// Ping type

fun glean_new_ping_type(name: String, include_client_id: Byte): Long
fun glean_new_ping_type(name: String, include_client_id: Byte, send_if_empty: Byte): Long

fun glean_destroy_ping_type(handle: Long)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,8 @@ class GleanTest {
val pingName = "custom_ping_1"
val ping = PingType(
name = pingName,
includeClientId = true
includeClientId = true,
sendIfEmpty = false
)
val stringMetric = StringMetricType(
disabled = false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ class PingTypeTest {

val customPing = PingType(
name = "custom",
includeClientId = true
includeClientId = true,
sendIfEmpty = false
)

val counter = CounterMetricType(
Expand Down Expand Up @@ -83,7 +84,8 @@ class PingTypeTest {

val customPing = PingType(
name = "custom",
includeClientId = false
includeClientId = false,
sendIfEmpty = false
)

val counter = CounterMetricType(
Expand Down
4 changes: 2 additions & 2 deletions glean-core/examples/sample.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ fn main() {
max_events: None,
};
let mut glean = Glean::new(cfg).unwrap();
glean.register_ping_type(&PingType::new("baseline", true));
glean.register_ping_type(&PingType::new("metrics", true));
glean.register_ping_type(&PingType::new("baseline", true, false));
glean.register_ping_type(&PingType::new("metrics", true, false));

let local_metric: StringMetric = StringMetric::new(CommonMetricData {
name: "local_metric".into(),
Expand Down
2 changes: 1 addition & 1 deletion glean-core/ffi/examples/glean_app.c
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ int main(void)
NULL
};
uint64_t glean = glean_initialize(&cfg);
uint64_t store1 = glean_new_ping_type("store1", true);
uint64_t store1 = glean_new_ping_type("store1", true, false);
glean_register_ping_type(glean, store1);

printf("Glean upload enabled? %d\n", glean_is_upload_enabled(glean));
Expand Down
2 changes: 1 addition & 1 deletion glean-core/ffi/glean.h
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ uint64_t glean_new_memory_distribution_metric(FfiStr category,
uint8_t disabled,
int32_t memory_unit);

uint64_t glean_new_ping_type(FfiStr ping_name, uint8_t include_client_id);
uint64_t glean_new_ping_type(FfiStr ping_name, uint8_t include_client_id, uint8_t send_if_empty);

uint64_t glean_new_quantity_metric(FfiStr category,
FfiStr name,
Expand Down
12 changes: 10 additions & 2 deletions glean-core/ffi/src/ping_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,18 @@ lazy_static! {
crate::define_infallible_handle_map_deleter!(PING_TYPES, glean_destroy_ping_type);

#[no_mangle]
pub extern "C" fn glean_new_ping_type(ping_name: FfiStr, include_client_id: u8) -> u64 {
pub extern "C" fn glean_new_ping_type(
ping_name: FfiStr,
include_client_id: u8,
send_if_empty: u8,
) -> u64 {
PING_TYPES.insert_with_log(|| {
let ping_name = ping_name.to_string_fallible()?;
Ok(PingType::new(ping_name, include_client_id != 0))
Ok(PingType::new(
ping_name,
include_client_id != 0,
send_if_empty != 0,
))
})
}

Expand Down
2 changes: 1 addition & 1 deletion glean-core/ios/Glean/GleanFfi.h
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ uint64_t glean_new_memory_distribution_metric(FfiStr category,
uint8_t disabled,
int32_t memory_unit);

uint64_t glean_new_ping_type(FfiStr ping_name, uint8_t include_client_id);
uint64_t glean_new_ping_type(FfiStr ping_name, uint8_t include_client_id, uint8_t send_if_empty);

uint64_t glean_new_quantity_metric(FfiStr category,
FfiStr name,
Expand Down
4 changes: 2 additions & 2 deletions glean-core/ios/Glean/Metrics/Ping.swift
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ public class Ping {
let includeClientId: Bool

/// The public constructor used by automatically generated metrics.
public init(name: String, includeClientId: Bool) {
public init(name: String, includeClientId: Bool, sendIfEmpty: Bool) {
self.name = name
self.includeClientId = includeClientId
self.handle = glean_new_ping_type(name, includeClientId.toByte())
self.handle = glean_new_ping_type(name, includeClientId.toByte(), sendIfEmpty.toByte())
NSLog("Registering this ping: \(name)")
Glean.shared.registerPingType(self)
}
Expand Down
4 changes: 2 additions & 2 deletions glean-core/ios/GleanTests/Metrics/PingTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class PingTests: XCTestCase {
}

func testSendingOfCustomPings() {
let customPing = Ping(name: "custom", includeClientId: true)
let customPing = Ping(name: "custom", includeClientId: true, sendIfEmpty: false)

let counter = CounterMetricType(
category: "telemetry",
Expand Down Expand Up @@ -68,7 +68,7 @@ class PingTests: XCTestCase {
}

func testSendingOfCustomPingsWithoutClientId() {
let customPing = Ping(name: "custom", includeClientId: false)
let customPing = Ping(name: "custom", includeClientId: false, sendIfEmpty: false)

let counter = CounterMetricType(
category: "telemetry",
Expand Down
2 changes: 1 addition & 1 deletion glean-core/ios/sdk_generator.sh
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@

set -e

GLEAN_PARSER_VERSION=1.11.0
GLEAN_PARSER_VERSION=1.12.0

# When the special argument "internal" is passed, don't add a namespace import and also allow all reserved items.
NAMESPACE=Glean
Expand Down
1 change: 1 addition & 0 deletions glean-core/python/glean/_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"disabled",
"histogram_type",
"include_client_id",
"send_if_empty",
"lifetime",
"memory_unit",
"name",
Expand Down
4 changes: 2 additions & 2 deletions glean-core/python/glean/metrics/ping.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@


class PingType:
def __init__(self, name: str, include_client_id: bool):
def __init__(self, name: str, include_client_id: bool, send_if_empty: bool):
"""
This implements the developer facing API for custom pings.

Expand All @@ -17,7 +17,7 @@ def __init__(self, name: str, include_client_id: bool):
"""
self._name = name
self._handle = _ffi.lib.glean_new_ping_type(
_ffi.ffi_encode_string(name), include_client_id
_ffi.ffi_encode_string(name), include_client_id, send_if_empty
)
Glean.register_ping_type(self)

Expand Down
2 changes: 1 addition & 1 deletion glean-core/python/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
parsed_toml = toml.load(cargo)
version = parsed_toml["package"]["version"]

requirements = ["cffi==1.13.1", "glean_parser==1.11.0"]
requirements = ["cffi==1.13.1", "glean_parser==1.12.0"]

setup_requirements = []

Expand Down
6 changes: 3 additions & 3 deletions glean-core/python/tests/test_glean.py
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ def test_dont_schedule_pings_if_metrics_disabled(safe_httpserver):
send_in_pings=["store1"],
)

custom_ping = PingType(name="store1", include_client_id=True)
custom_ping = PingType(name="store1", include_client_id=True, send_if_empty=False)

counter_metric.add(10)

Expand All @@ -211,7 +211,7 @@ def test_dont_schedule_pings_if_metrics_disabled(safe_httpserver):
def test_dont_schedule_pings_if_there_is_no_ping_content(safe_httpserver):
safe_httpserver.serve_content(b"", code=200)

custom_ping = PingType(name="store1", include_client_id=True)
custom_ping = PingType(name="store1", include_client_id=True, send_if_empty=False)

custom_ping.send()

Expand Down Expand Up @@ -297,7 +297,7 @@ def test_collect():
send_in_pings=["store1"],
)

custom_ping = PingType(name="store1", include_client_id=True)
custom_ping = PingType(name="store1", include_client_id=True, send_if_empty=False)

counter_metric.add(10)

Expand Down
2 changes: 1 addition & 1 deletion glean-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ pub struct Configuration {
/// max_events: None,
/// };
/// let mut glean = Glean::new(cfg).unwrap();
/// let ping = PingType::new("sample", true);
/// let ping = PingType::new("sample", true, false);
/// glean.register_ping_type(&ping);
///
/// let call_counter: CounterMetric = CounterMetric::new(CommonMetricData {
Expand Down
5 changes: 4 additions & 1 deletion glean-core/src/metrics/ping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ pub struct PingType {
pub name: String,
/// Whether the ping should include the client ID.
pub include_client_id: bool,
/// Whether the ping should be sent if it is empty
pub send_if_empty: bool,
}

impl PingType {
Expand All @@ -26,10 +28,11 @@ impl PingType {
/// * `name` - The name of the ping.
/// * `include_client_id` - Whether to include the client ID in the assembled ping when.
/// sending.
pub fn new<A: Into<String>>(name: A, include_client_id: bool) -> Self {
pub fn new<A: Into<String>>(name: A, include_client_id: bool, send_if_empty: bool) -> Self {
Self {
name: name.into(),
include_client_id,
send_if_empty,
}
}

Expand Down
5 changes: 4 additions & 1 deletion glean-core/src/ping/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,12 @@ impl PingMaker {
let metrics_data = StorageManager.snapshot_as_json(glean.storage(), &ping.name, true);
let events_data = glean.event_storage().snapshot_as_json(&ping.name, true);

if metrics_data.is_none() && events_data.is_none() {
let is_empty = metrics_data.is_none() && events_data.is_none();
if !ping.send_if_empty && is_empty {
info!("Storage for {} empty. Bailing out.", ping.name);
return None;
} else if is_empty {
info!("Storage for {} empty. Ping will still be sent.", ping.name);
}

let ping_info = self.get_ping_info(glean, &ping.name);
Expand Down
2 changes: 1 addition & 1 deletion glean-core/tests/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ fn test_sending_of_event_ping_when_it_fills_up() {
let store_names: Vec<String> = vec!["events".into()];

for store_name in &store_names {
glean.register_ping_type(&PingType::new(store_name.clone(), true));
glean.register_ping_type(&PingType::new(store_name.clone(), true, false));
}

let click = EventMetric::new(
Expand Down
24 changes: 22 additions & 2 deletions glean-core/tests/ping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use glean_core::CommonMetricData;
fn write_ping_to_disk() {
let (mut glean, _temp) = new_glean();

let ping = PingType::new("metrics", true);
let ping = PingType::new("metrics", true, false);
glean.register_ping_type(&ping);

// We need to store a metric as an empty ping is not stored.
Expand All @@ -33,7 +33,7 @@ fn write_ping_to_disk() {
fn disabling_upload_clears_pending_pings() {
let (mut glean, _) = new_glean();

let ping = PingType::new("metrics", true);
let ping = PingType::new("metrics", true, false);
glean.register_ping_type(&ping);

// We need to store a metric as an empty ping is not stored.
Expand All @@ -58,3 +58,23 @@ fn disabling_upload_clears_pending_pings() {
assert!(ping.send(&glean).unwrap());
assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len());
}

#[test]
fn empty_pings_with_flag_are_sent() {
let (mut glean, _) = new_glean();

let ping1 = PingType::new("custom-ping1", true, true);
glean.register_ping_type(&ping1);
let ping2 = PingType::new("custom-ping2", true, false);
glean.register_ping_type(&ping2);

// No data is stored in either of the custom pings

// Sending this should succeed.
assert_eq!(true, ping1.send(&glean).unwrap());
assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len());

// Sending this should fail.
assert_eq!(false, ping2.send(&glean).unwrap());
assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len());
}
14 changes: 7 additions & 7 deletions glean-core/tests/ping_maker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ fn set_up_basic_ping() -> (Glean, PingMaker, PingType, tempfile::TempDir) {
};
let mut glean = Glean::new(cfg).unwrap();
let ping_maker = PingMaker::new();
let ping_type = PingType::new("store1", true);
let ping_type = PingType::new("store1", true, false);
glean.register_ping_type(&ping_type);

// Record something, so the ping will have data
Expand Down Expand Up @@ -88,7 +88,7 @@ fn collect_must_report_none_when_no_data_is_stored() {

let (mut glean, ping_maker, ping_type, _t) = set_up_basic_ping();

let unknown_ping_type = PingType::new("unknown", true);
let unknown_ping_type = PingType::new("unknown", true, false);
glean.register_ping_type(&ping_type);

assert!(ping_maker.collect(&glean, &unknown_ping_type).is_none());
Expand All @@ -110,7 +110,7 @@ fn seq_number_must_be_sequential() {

for i in 0..=1 {
for ping_name in ["store1", "store2"].iter() {
let ping_type = PingType::new(*ping_name, true);
let ping_type = PingType::new(*ping_name, true, false);
let content = ping_maker.collect(&glean, &ping_type).unwrap();
let seq_num = content["ping_info"]["seq"].as_i64().unwrap();
// Ensure sequence numbers in different stores are independent of
Expand All @@ -121,7 +121,7 @@ fn seq_number_must_be_sequential() {

// Test that ping sequence numbers increase independently.
{
let ping_type = PingType::new("store1", true);
let ping_type = PingType::new("store1", true, false);

// 3rd ping of store1
let content = ping_maker.collect(&glean, &ping_type).unwrap();
Expand All @@ -135,7 +135,7 @@ fn seq_number_must_be_sequential() {
}

{
let ping_type = PingType::new("store2", true);
let ping_type = PingType::new("store2", true, false);

// 3rd ping of store2
let content = ping_maker.collect(&glean, &ping_type).unwrap();
Expand All @@ -144,7 +144,7 @@ fn seq_number_must_be_sequential() {
}

{
let ping_type = PingType::new("store1", true);
let ping_type = PingType::new("store1", true, false);

// 5th ping of store1
let content = ping_maker.collect(&glean, &ping_type).unwrap();
Expand All @@ -157,7 +157,7 @@ fn seq_number_must_be_sequential() {
fn test_clear_pending_pings() {
let (mut glean, _) = new_glean();
let ping_maker = PingMaker::new();
let ping_type = PingType::new("store1", true);
let ping_type = PingType::new("store1", true, false);
glean.register_ping_type(&ping_type);

// Record something, so the ping will have data
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class GleanMetricsYamlTransform extends ArtifactTransform {
@SuppressWarnings("GrPackage")
class GleanPlugin implements Plugin<Project> {
// The version of glean_parser to install from PyPI.
private String GLEAN_PARSER_VERSION = "1.11.0"
private String GLEAN_PARSER_VERSION = "1.12.0"
// The version of Miniconda is explicitly specified.
// Miniconda3-4.5.12 is known to not work on Windows.
private String MINICONDA_VERSION = "4.5.11"
Expand Down