From 3ea524658762a069ba030b7de2cd4fd8563d5f77 Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Thu, 16 May 2019 12:29:17 +0200 Subject: [PATCH 1/5] Add a Contact section to the readme file This also updates the bugzilla new bug template to add the expected priority and whiteboard tag so that bugs show up on bugzhub. --- README.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/README.md b/README.md index a296465f4..402112bce 100644 --- a/README.md +++ b/README.md @@ -30,6 +30,14 @@ User-facing documentation can be found in [./docs/user](docs/user). The Rust documentation is available [online as well](https://mozilla.github.com/glean/docs). +## Contact + +To contact us you can: +- Find us on the Mozilla Slack in *#glean*, on [Mozilla IRC](https://wiki.mozilla.org/IRC) in *#telemetry*. +- To report issues or request changes, file a bug in [Bugzilla in Data Platform & Tools :: Glean: SDK](https://bugzilla.mozilla.org/enter_bug.cgi?assigned_to=nobody%40mozilla.org&bug_ignored=0&bug_severity=normal&bug_status=NEW&cf_fission_milestone=---&cf_fx_iteration=---&cf_fx_points=---&cf_status_firefox65=---&cf_status_firefox66=---&cf_status_firefox67=---&cf_status_firefox_esr60=---&cf_status_thunderbird_esr60=---&cf_tracking_firefox65=---&cf_tracking_firefox66=---&cf_tracking_firefox67=---&cf_tracking_firefox_esr60=---&cf_tracking_firefox_relnote=---&cf_tracking_thunderbird_esr60=---&product=Data%20Platform%20and%20Tools&component=Glean%3A%20SDK&contenttypemethod=list&contenttypeselection=text%2Fplain&defined_groups=1&flag_type-203=X&flag_type-37=X&flag_type-41=X&flag_type-607=X&flag_type-721=X&flag_type-737=X&flag_type-787=X&flag_type-799=X&flag_type-800=X&flag_type-803=X&flag_type-835=X&flag_type-846=X&flag_type-855=X&flag_type-864=X&flag_type-916=X&flag_type-929=X&flag_type-930=X&flag_type-935=X&flag_type-936=X&flag_type-937=X&form_name=enter_bug&maketemplate=Remember%20values%20as%20bookmarkable%20template&op_sys=Unspecified&priority=P3&&rep_platform=Unspecified&status_whiteboard=%5Btelemetry%3Aglean-rs%3Am%3F%5D&target_milestone=---&version=unspecified). +- Send an email to *glean-team@mozilla.com*. +- The Glean Core team is: *:dexter*, *:janerik*, *:mdroettboom*, *:gfritzsche* + ## License This Source Code Form is subject to the terms of the Mozilla Public From d3e4f01df7b807c4fc956e3ca396444b5816c4ac Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Wed, 15 May 2019 16:34:24 +0200 Subject: [PATCH 2/5] Use i32 everywhere for counters We actually should use u32 internally, but: because Kotlin doesn't (yet) have unsigned integers, it's always going to pass us signed values. We also do all the error checking on the Rust side, so we need to receive the signed value to check for negativity and bail out. We still don't do overflow checks though. --- .../glean/private/CounterMetricType.kt | 5 ++-- .../telemetry/glean/rust/LibGleanFFI.kt | 4 ++-- .../glean/private/CounterMetricTypeTest.kt | 24 +++++++++++++++++++ glean-core/ffi/src/lib.rs | 4 ++-- glean-core/src/metrics/counter.rs | 12 +++++++--- glean-core/src/metrics/mod.rs | 2 +- 6 files changed, 40 insertions(+), 11 deletions(-) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/CounterMetricType.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/CounterMetricType.kt index 758a65618..72b3ca612 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/CounterMetricType.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/CounterMetricType.kt @@ -64,7 +64,7 @@ class CounterMetricType( LibGleanFFI.INSTANCE.glean_counter_add( Glean.handle, this@CounterMetricType.handle, - amount.toLong()) + amount) } } @@ -96,10 +96,9 @@ class CounterMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testGetValue(pingName: String = sendInPings.first()): Int { - // FIXME(#19): glean-core should give us an int to begin with if (!testHasValue(pingName)) { throw NullPointerException() } - return LibGleanFFI.INSTANCE.glean_counter_test_get_value(Glean.handle, this.handle, pingName).toInt() + return LibGleanFFI.INSTANCE.glean_counter_test_get_value(Glean.handle, this.handle, pingName) } } diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt index e95db6b32..a14b5c57f 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt @@ -60,9 +60,9 @@ internal interface LibGleanFFI : Library { fun glean_boolean_set(glean_handle: Long, metric_id: Long, value: Byte) - fun glean_counter_add(glean_handle: Long, metric_id: Long, amount: Long) + fun glean_counter_add(glean_handle: Long, metric_id: Long, amount: Int) - fun glean_counter_test_get_value(glean_handle: Long, metric_id: Long, storage_name: String): Long + fun glean_counter_test_get_value(glean_handle: Long, metric_id: Long, storage_name: String): Int fun glean_counter_test_has_value(glean_handle: Long, metric_id: Long, storage_name: String): Byte diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/CounterMetricTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/CounterMetricTypeTest.kt index e69344403..27511c35d 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/CounterMetricTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/CounterMetricTypeTest.kt @@ -135,4 +135,28 @@ class CounterMetricTypeTest { assertTrue(counterMetric.testHasValue("store2")) assertEquals(11, counterMetric.testGetValue("store2")) } + + @Test + fun `negative values are not counted`() { + // Define a 'counterMetric' counter metric, which will be stored in "store1" + val counterMetric = CounterMetricType( + disabled = false, + category = "telemetry", + lifetime = Lifetime.Application, + name = "counter_metric", + sendInPings = listOf("store1") + ) + + // Increment to 1 (initial value) + counterMetric.add() + + // Check that the count was incremented + assertTrue(counterMetric.testHasValue("store1")) + assertEquals(1, counterMetric.testGetValue("store1")) + + counterMetric.add(-10) + // Check that count was NOT incremented. + assertTrue(counterMetric.testHasValue("store1")) + assertEquals(1, counterMetric.testGetValue("store1")) + } } diff --git a/glean-core/ffi/src/lib.rs b/glean-core/ffi/src/lib.rs index e830d6d33..f3cdaf929 100755 --- a/glean-core/ffi/src/lib.rs +++ b/glean-core/ffi/src/lib.rs @@ -173,7 +173,7 @@ pub extern "C" fn glean_new_counter_metric( } #[no_mangle] -pub extern "C" fn glean_counter_add(glean_handle: u64, metric_id: u64, amount: u64) { +pub extern "C" fn glean_counter_add(glean_handle: u64, metric_id: u64, amount: i32) { GLEAN.call_infallible(glean_handle, |glean| { COUNTER_METRICS.call_infallible(metric_id, |metric| { metric.add(glean, amount); @@ -201,7 +201,7 @@ pub extern "C" fn glean_counter_test_get_value( glean_handle: u64, metric_id: u64, storage_name: FfiStr, -) -> u64 { +) -> i32 { GLEAN.call_infallible(glean_handle, |glean| { COUNTER_METRICS.call_infallible(metric_id, |metric| { metric.test_get_value(glean, storage_name.as_str()).unwrap() diff --git a/glean-core/src/metrics/counter.rs b/glean-core/src/metrics/counter.rs index 11c889804..e72b94916 100644 --- a/glean-core/src/metrics/counter.rs +++ b/glean-core/src/metrics/counter.rs @@ -17,11 +17,17 @@ impl CounterMetric { Self { meta } } - pub fn add(&self, glean: &Glean, amount: u64) { + pub fn add(&self, glean: &Glean, amount: i32) { if !self.meta.should_record() || !glean.is_upload_enabled() { return; } + if amount <= 0 { + // TODO: Turn this into logging an error + log::warn!("CounterMetric::add: got negative amount. Not recording."); + return; + } + glean .storage() .record_with(&self.meta, |old_value| match old_value { @@ -35,7 +41,7 @@ impl CounterMetric { /// Get the currently stored value as an integer. /// /// This doesn't clear the stored value. - pub fn test_get_value(&self, glean: &Glean, storage_name: &str) -> Option { + pub fn test_get_value(&self, glean: &Glean, storage_name: &str) -> Option { let snapshot = match StorageManager.snapshot_as_json(glean.storage(), storage_name, false) { Some(snapshot) => snapshot, None => return None, @@ -45,6 +51,6 @@ impl CounterMetric { .and_then(|o| o.get("counter")) .and_then(|o| o.as_object()) .and_then(|o| o.get(&self.meta.identifier())) - .and_then(|o| o.as_i64().map(|i| i as u64)) + .and_then(|o| o.as_i64().map(|i| i as i32)) } } diff --git a/glean-core/src/metrics/mod.rs b/glean-core/src/metrics/mod.rs index e46008a0a..957179196 100644 --- a/glean-core/src/metrics/mod.rs +++ b/glean-core/src/metrics/mod.rs @@ -21,7 +21,7 @@ pub use self::uuid::UuidMetric; pub enum Metric { String(String), Boolean(bool), - Counter(u64), + Counter(i32), Uuid(String), StringList(Vec), } From f65a1b796d0359ed54efbf20538505f5e90c2b33 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Thu, 16 May 2019 14:05:10 +0200 Subject: [PATCH 3/5] Don't try to guess ANDROID_HOME It was only ever correct for default installs on macOS anyway. --- Makefile | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Makefile b/Makefile index 35f40eee2..813ede63f 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,3 @@ -ifeq ($(ANDROID_HOME),) - ANDROID_HOME := ~/Library/Android/sdk -endif - build-apk: ./gradlew glean-core:build ./gradlew glean-sample-app:build From dd5e947c4a4ef0f9f090a6fe8a104bf4f158bda8 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Thu, 16 May 2019 14:05:33 +0200 Subject: [PATCH 4/5] Make lint depend on properly formatted Rust code --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 813ede63f..9d308758d 100644 --- a/Makefile +++ b/Makefile @@ -11,7 +11,7 @@ emulator: $(ANDROID_HOME)/emulator/emulator -avd Nexus_5X_API_P -netdelay none -netspeed full .PHONY: install -lint: +lint: fmt cargo clippy --all ./gradlew ktlint detekt .PHONY: lint From 634f447dfb40be9ac78db6503bbfd8fbc537d9a3 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Thu, 16 May 2019 15:06:34 +0200 Subject: [PATCH 5/5] Multi-threaded tests locally --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 9d308758d..7db34a354 100644 --- a/Makefile +++ b/Makefile @@ -21,7 +21,7 @@ fmt: .PHONY: fmt test: - RUST_TEST_THREADS=1 cargo test --all + cargo test --all ./gradlew test .PHONY: test