From cc2d5729f21f4b28b22d8be124d4b1296b9ea3d5 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 30 Apr 2021 12:16:08 +0200 Subject: [PATCH 01/13] Upgrade to glean_parser 3.4.0 --- Makefile | 2 +- glean-core/Cargo.toml | 2 +- glean-core/csharp/Glean/GleanParser.cs | 2 +- glean-core/ios/sdk_generator.sh | 2 +- glean-core/python/glean/__init__.py | 2 +- glean-core/python/setup.py | 2 +- .../telemetry/glean-gradle-plugin/GleanGradlePlugin.groovy | 2 +- samples/ios/app/Cartfile | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Makefile b/Makefile index 126da2b080..93e9630e84 100644 --- a/Makefile +++ b/Makefile @@ -144,7 +144,7 @@ python-docs: build-python ## Build the Python documentation .PHONY: docs rust-docs kotlin-docs swift-docs metrics-docs: python-setup ## Build the internal metrics documentation - $(GLEAN_PYENV)/bin/pip install glean_parser==3.2.0 + $(GLEAN_PYENV)/bin/pip install glean_parser==3.4.0 $(GLEAN_PYENV)/bin/glean_parser translate --allow-reserved \ -f markdown \ -o ./docs/user/user/collected-metrics \ diff --git a/glean-core/Cargo.toml b/glean-core/Cargo.toml index 1190d23973..4f05a8a3cf 100644 --- a/glean-core/Cargo.toml +++ b/glean-core/Cargo.toml @@ -18,7 +18,7 @@ include = [ ] [package.metadata.glean] -glean-parser = "3.2.0" +glean-parser = "3.4.0" [badges] circle-ci = { repository = "mozilla/glean", branch = "main" } diff --git a/glean-core/csharp/Glean/GleanParser.cs b/glean-core/csharp/Glean/GleanParser.cs index ea80db4377..793a6e43e7 100644 --- a/glean-core/csharp/Glean/GleanParser.cs +++ b/glean-core/csharp/Glean/GleanParser.cs @@ -18,7 +18,7 @@ public class GleanParser : ToolTask private const string DefaultVirtualEnvDir = ".venv"; // The glean_parser pypi package version - private const string GleanParserVersion = "3.2.0"; + private const string GleanParserVersion = "3.4.0"; // This script runs a given Python module as a "main" module, like // `python -m module`. However, it first checks that the installed diff --git a/glean-core/ios/sdk_generator.sh b/glean-core/ios/sdk_generator.sh index 07544e2184..6562c07c12 100755 --- a/glean-core/ios/sdk_generator.sh +++ b/glean-core/ios/sdk_generator.sh @@ -25,7 +25,7 @@ set -e -GLEAN_PARSER_VERSION=3.2.0 +GLEAN_PARSER_VERSION=3.4.0 # CMDNAME is used in the usage text below. # shellcheck disable=SC2034 diff --git a/glean-core/python/glean/__init__.py b/glean-core/python/glean/__init__.py index 0157189cd2..01badd8a29 100644 --- a/glean-core/python/glean/__init__.py +++ b/glean-core/python/glean/__init__.py @@ -30,7 +30,7 @@ __email__ = "glean-team@mozilla.com" -GLEAN_PARSER_VERSION = "3.2.0" +GLEAN_PARSER_VERSION = "3.4.0" if glean_parser.__version__ != GLEAN_PARSER_VERSION: diff --git a/glean-core/python/setup.py b/glean-core/python/setup.py index 9b1d427a19..80e94eb94b 100644 --- a/glean-core/python/setup.py +++ b/glean-core/python/setup.py @@ -60,7 +60,7 @@ requirements = [ "cffi>=1.13.0", - "glean_parser==3.2.0", + "glean_parser==3.4.0", "iso8601>=0.1.10; python_version<='3.6'", ] diff --git a/gradle-plugin/src/main/groovy/mozilla/telemetry/glean-gradle-plugin/GleanGradlePlugin.groovy b/gradle-plugin/src/main/groovy/mozilla/telemetry/glean-gradle-plugin/GleanGradlePlugin.groovy index eee8d14426..904b97d13a 100644 --- a/gradle-plugin/src/main/groovy/mozilla/telemetry/glean-gradle-plugin/GleanGradlePlugin.groovy +++ b/gradle-plugin/src/main/groovy/mozilla/telemetry/glean-gradle-plugin/GleanGradlePlugin.groovy @@ -36,7 +36,7 @@ class GleanMetricsYamlTransform extends ArtifactTransform { @SuppressWarnings("GrPackage") class GleanPlugin implements Plugin { // The version of glean_parser to install from PyPI. - private String GLEAN_PARSER_VERSION = "3.2.0" + private String GLEAN_PARSER_VERSION = "3.4.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" diff --git a/samples/ios/app/Cartfile b/samples/ios/app/Cartfile index 27e7cb3e3d..653ea0d03d 100644 --- a/samples/ios/app/Cartfile +++ b/samples/ios/app/Cartfile @@ -1,3 +1,3 @@ github "httpswift/swifter" "1.5.0" github "1024jp/GzipSwift" "5.1.1" -github "mozilla/glean" "main" +github "mozilla/glean" "new-event-extras" From 58172c6cbced4a82a09c8a18c923ea046bee4132 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Wed, 24 Mar 2021 10:47:19 +0100 Subject: [PATCH 02/13] Kotlin: Allow new event extra properties to be passed as an object --- docs/user/user/collected-metrics/metrics.md | 2 +- .../glean/private/EventMetricType.kt | 56 +++++++++++++++++++ .../java/mozilla/telemetry/glean/GleanTest.kt | 3 +- .../telemetry/glean/pings/CustomPingTest.kt | 2 + .../glean/private/EventMetricTypeTest.kt | 24 +++++++- .../org/mozilla/samples/glean/MainActivity.kt | 1 + 6 files changed, 85 insertions(+), 3 deletions(-) diff --git a/docs/user/user/collected-metrics/metrics.md b/docs/user/user/collected-metrics/metrics.md index 33245d567a..7e617c8a85 100644 --- a/docs/user/user/collected-metrics/metrics.md +++ b/docs/user/user/collected-metrics/metrics.md @@ -168,5 +168,5 @@ In addition to those built-in metrics, the following metrics are added to the pi Data categories are [defined here](https://wiki.mozilla.org/Firefox/Data_Collection). - + diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/EventMetricType.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/EventMetricType.kt index 6582565e6c..33accc1a73 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/EventMetricType.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/EventMetricType.kt @@ -54,6 +54,21 @@ enum class NoExtraKeys( // deliberately empty } +/** + * A class that can be converted into key-value pairs of event extras. + * This will be automatically implemented for event properties of an [EventMetricType]. + */ +interface EventExtras { + /** + * Convert the event extras into 2 lists: + * + * 1. The list of extra key indices. + * Unset keys will be skipped. + * 2. The list of extra values. + */ + fun toFfiExtra(): Pair +} + /** * This implements the developer facing API for recording events. * @@ -99,11 +114,15 @@ class EventMetricType> internal constructor( /** * Record an event by using the information provided by the instance of this class. * + * **THIS METHOD IS DEPRECATED.** Specify types for your event extras. + * See the reference for details: https://mozilla.github.io/glean/book/reference/metrics/event.html#recording-api + * * @param extra optional. This is a map, both keys and values need to be strings, keys are * identifiers. This is used for events where additional richer context is needed. * The maximum length for values is 100 bytes. */ @JvmOverloads + @Deprecated("Specify types for your event extras. See the reference for details.") fun record(extra: Map? = null) { if (disabled) { return @@ -140,6 +159,43 @@ class EventMetricType> internal constructor( } } + /** + * Record an event by using the information provided by the instance of this class. + * + * @param extra The event extra properties. + * Values are converted to strings automatically + * This is used for events where additional richer context is needed. + * The maximum length for values is 100 bytes. + * + * Note: `extra` is not optional here to avoid overlapping with the above definition of `record`. + * If no `extra` data is passed the above function will be invoked correctly. + */ + fun record(extra: EventExtras) { + if (disabled) { + return + } + + // We capture the event time now, since we don't know when the async code below + // might get executed. + val timestamp = LibGleanFFI.INSTANCE.glean_get_timestamp_ms() + + @Suppress("EXPERIMENTAL_API_USAGE") + Dispatchers.API.launch { + val extras = extra.toFfiExtra() + val keys = extras.first + val values = extras.second + val len = keys.size + + LibGleanFFI.INSTANCE.glean_event_record( + this@EventMetricType.handle, + timestamp, + keys, + values, + len + ) + } + } + /** * Tests whether a value is stored for the metric for testing purposes only. * diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt index a586195ef0..cf8aec0e43 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt @@ -175,8 +175,9 @@ class GleanTest { assertFalse(Glean.testIsExperimentActive("experiment_preinit_disabled")) } + // Suppressing our own deprecation before we move over to the new event recording API. @Test - @Suppress("ComplexMethod", "LongMethod") + @Suppress("ComplexMethod", "LongMethod", "DEPRECATION") fun `test sending of foreground and background pings`() { val server = getMockWebServer() diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt index 891ccd252f..78231aae11 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt @@ -125,7 +125,9 @@ class CustomPingTest { assertEquals(1L, pingInfo.tryGetLong("seq")!!) } + // Suppressing our own deprecation before we move over to the new event recording API. @Test + @Suppress("DEPRECATION") fun `events for custom pings are flushed at startup`() { delayMetricsPing(context) resetGlean(context, Glean.configuration.copy( diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt index 95f3efc558..d5ba8457ff 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt @@ -37,6 +37,7 @@ import org.junit.Assert.fail import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith +import com.sun.jna.StringArray // Declared here, since Kotlin can not declare nested enum classes. enum class clickKeys { @@ -44,6 +45,25 @@ enum class clickKeys { other } +// The event extra properties. +// This would be generated by the glean_parser usually. +data class ClickExtras(val objectId: String? = null, val other: String? = null) : EventExtras { + override fun toFfiExtra(): Pair { + var keys = mutableListOf() + var values = mutableListOf() + + this.objectId?.let { + keys.add(0) + values.add(it) + } + this.other?.let { + keys.add(1) + values.add(it) + } + return Pair(IntArray(keys.size, { keys[it] }), StringArray(values.toTypedArray(), "utf-8")) + } +} + enum class testNameKeys { testName } @@ -52,6 +72,8 @@ enum class SomeExtraKeys { SomeExtra } +// Suppressing our own deprecation before we move over to the new event recording API. +@Suppress("DEPRECATION") @RunWith(AndroidJUnit4::class) class EventMetricTypeTest { @@ -72,7 +94,7 @@ class EventMetricTypeTest { ) // Record two events of the same type, with a little delay. - click.record(extra = mapOf(clickKeys.objectId to "buttonA", clickKeys.other to "foo")) + click.record(ClickExtras(objectId = "buttonA", other = "foo")) val expectedTimeSinceStart: Long = 37 SystemClock.sleep(expectedTimeSinceStart) diff --git a/samples/android/app/src/main/java/org/mozilla/samples/glean/MainActivity.kt b/samples/android/app/src/main/java/org/mozilla/samples/glean/MainActivity.kt index 1fc45a4524..2646f50fa5 100644 --- a/samples/android/app/src/main/java/org/mozilla/samples/glean/MainActivity.kt +++ b/samples/android/app/src/main/java/org/mozilla/samples/glean/MainActivity.kt @@ -36,6 +36,7 @@ open class MainActivity : AppCompatActivity() { // order to illustrate adding extra information to the event, it is also adding to the // 'extras' field a dictionary of values. Note that the dictionary keys must be // declared in the metrics.yaml file under the 'extra_keys' section of an event metric. + @Suppress("DEPRECATION") BrowserEngagement.click.record( mapOf( BrowserEngagement.clickKeys.key1 to "extra_value_1", From 427bd21b288daf777d8c498de70686e373813b9d Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Wed, 24 Mar 2021 11:57:06 +0100 Subject: [PATCH 03/13] Swift: Allow new event extra properties to be passed as an object --- .../ios/Glean/Metrics/EventMetric.swift | 53 ++++++++++++++----- .../GleanTests/Metrics/EventMetricTests.swift | 39 ++++++++++++-- 2 files changed, 74 insertions(+), 18 deletions(-) diff --git a/glean-core/ios/Glean/Metrics/EventMetric.swift b/glean-core/ios/Glean/Metrics/EventMetric.swift index f8c93d18bb..d6f736c401 100644 --- a/glean-core/ios/Glean/Metrics/EventMetric.swift +++ b/glean-core/ios/Glean/Metrics/EventMetric.swift @@ -33,6 +33,19 @@ public protocol ExtraKeys: Hashable { func index() -> Int32 } +/// Extra keys for events by name. +/// +/// For user-defined `EventMetricType`s every event with extras +/// will get their own corresponding event extra data class. +public protocol EventExtras { + /// Convert the event extras into 2 lists: + /// + /// 1. The list of extra key indices. + /// Unset keys will be skipped. + /// 2. The list of extra values. + func toFfiExtra() -> ([Int32], [String]) +} + /// Default of no extra keys for events. /// /// An enum with no values for convenient use as the default set of extra keys @@ -43,6 +56,23 @@ public enum NoExtraKeys: ExtraKeys { } } +/// Default-implement `EventExtras` for a map of ExtraKeys to Strings. +extension Dictionary: EventExtras where Key: ExtraKeys, Value == String { + public func toFfiExtra() -> ([Int32], [String]) { + var keys = [Int32]() + var values = [String]() + keys.reserveCapacity(self.count) + values.reserveCapacity(self.count) + + for (key, value) in self { + keys.append(key.index()) + values.append(value) + } + + return (keys, values) + } +} + /// This implements the developer facing API for recording events. /// /// Instances of this class type are automatically generated by the parsers at built time, @@ -97,6 +127,10 @@ public class EventMetricType { /// This is used for events where additional richer context is needed. /// The maximum length for values is defined by `MAX_LENGTH_EXTRA_KEY_VALUE`. public func record(extra: [ExtraKeysEnum: String]? = nil) { + self.record(extra) + } + + public func record(_ properties: EventExtras? = nil) { guard !self.disabled else { return } // We capture the event time now, since we don't know when the async code below @@ -110,20 +144,11 @@ public class EventMetricType { var keys: [Int32]? var values: [String]? - if let extra = extra { - len = extra.count - - if len > 0 { - keys = [] - values = [] - keys?.reserveCapacity(len) - values?.reserveCapacity(len) - - for (key, value) in extra { - keys?.append(key.index()) - values?.append(value) - } - } + if let extra = properties { + let (k, v) = extra.toFfiExtra() + len = k.count + keys = k + values = v } withArrayOfCStrings(values) { values in diff --git a/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift b/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift index 1ca51c85b0..1521917e40 100644 --- a/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift +++ b/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift @@ -15,6 +15,27 @@ enum ClickKeys: Int32, ExtraKeys { } } +struct ClickExtras: EventExtras { + var objectId: String? + var other: String? + + func toFfiExtra() -> ([Int32], [String]) { + var keys = [Int32]() + var values = [String]() + + if let objectId = self.objectId { + keys.append(0) + values.append(objectId) + } + if let other = self.other { + keys.append(1) + values.append(other) + } + + return (keys, values) + } +} + enum TestNameKeys: Int32, ExtraKeys { case testName = 0 @@ -78,15 +99,20 @@ class EventMetricTypeTests: XCTestCase { XCTAssertFalse(metric.testHasValue()) - metric.record(extra: [.objectId: "buttonA", .other: "foo"]) + // Newer API + metric.record(ClickExtras(objectId: "buttonA", other: "foo")) + // Some extra keys can be left undefined. + metric.record(ClickExtras(objectId: "buttonA")) /* SKIPPED: resetting system clock to return fixed time value */ + // Old API, this is available only because we manually implemented the enum. + // Generated code will have only one of the APIs available. metric.record(extra: [.objectId: "buttonB", .other: "bar"]) XCTAssert(metric.testHasValue()) let events = try! metric.testGetValue() - XCTAssertEqual(2, events.count) + XCTAssertEqual(3, events.count) XCTAssertEqual("ui", events[0].category) XCTAssertEqual("click", events[0].name) @@ -95,8 +121,13 @@ class EventMetricTypeTests: XCTestCase { XCTAssertEqual("ui", events[1].category) XCTAssertEqual("click", events[1].name) - XCTAssertEqual("buttonB", events[1].extra?["object_id"]) - XCTAssertEqual("bar", events[1].extra?["other"]) + XCTAssertEqual("buttonA", events[1].extra?["object_id"]) + XCTAssertEqual(nil, events[1].extra?["other"]) + + XCTAssertEqual("ui", events[2].category) + XCTAssertEqual("click", events[2].name) + XCTAssertEqual("buttonB", events[2].extra?["object_id"]) + XCTAssertEqual("bar", events[2].extra?["other"]) XCTAssertLessThanOrEqual(events[0].timestamp, events[1].timestamp, "The sequence of events must be preserved") } From c1227965b44fd9e65b002d3da455d0102940d04a Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Thu, 29 Apr 2021 15:51:16 +0200 Subject: [PATCH 04/13] Rust: Allow new event extra properties to be passed as an object BREAKING CHANGE: This removes support for the old API, passing a HashMap of ExtraKey -> String pairs. The new API will look like this for consumers: let mut extra = SomeExtra { key1: Some("1".into()), key2: Some("2".into) }; metric.record(extra); glean_parser will generate for the new API only. --- glean-core/rlb/src/private/event.rs | 36 ++++++++++++++++------------- glean-core/src/traits/event.rs | 31 +++++++++++-------------- 2 files changed, 33 insertions(+), 34 deletions(-) diff --git a/glean-core/rlb/src/private/event.rs b/glean-core/rlb/src/private/event.rs index c7621ad357..3da937ab10 100644 --- a/glean-core/rlb/src/private/event.rs +++ b/glean-core/rlb/src/private/event.rs @@ -56,13 +56,11 @@ impl EventMetric { impl traits::Event for EventMetric { type Extra = K; - fn record::Extra, String>>>>(&self, extra: M) { + fn record::Extra>>>(&self, extra: M) { let now = crate::get_timestamp_ms(); - // Translate from [ExtraKey -> String] to a [Int -> String] map - let extra = extra - .into() - .map(|h| h.into_iter().map(|(k, v)| (k.index(), v)).collect()); + // Translate from {ExtraKey -> String} to a [Int -> String] map + let extra = extra.into().map(|e| e.into_ffi_extra()); let metric = Arc::clone(&self.inner); crate::launch_with_glean(move |glean| metric.record(glean, now, extra)); } @@ -130,17 +128,20 @@ mod test { let _lock = lock_test(); let _t = new_glean(None, true); - #[derive(Debug, Clone, Copy, Hash, Eq, PartialEq)] - enum SomeExtra { - Key1, - Key2, + #[derive(Default, Debug, Clone, Hash, Eq, PartialEq)] + struct SomeExtra { + key1: Option, + key2: Option, } impl glean_core::traits::ExtraKeys for SomeExtra { const ALLOWED_KEYS: &'static [&'static str] = &["key1", "key2"]; - fn index(self) -> i32 { - self as i32 + fn into_ffi_extra(self) -> HashMap { + let mut map = HashMap::new(); + self.key1.and_then(|key1| map.insert(0, key1)); + self.key2.and_then(|key2| map.insert(1, key2)); + map } } @@ -151,13 +152,16 @@ mod test { ..Default::default() }); - let mut map1 = HashMap::new(); - map1.insert(SomeExtra::Key1, "1".into()); + let map1 = SomeExtra { + key1: Some("1".into()), + ..Default::default() + }; metric.record(map1); - let mut map2 = HashMap::new(); - map2.insert(SomeExtra::Key1, "1".into()); - map2.insert(SomeExtra::Key2, "2".into()); + let map2 = SomeExtra { + key1: Some("1".into()), + key2: Some("2".into()), + }; metric.record(map2); metric.record(None); diff --git a/glean-core/src/traits/event.rs b/glean-core/src/traits/event.rs index 08277b8cf4..21a48b6194 100644 --- a/glean-core/src/traits/event.rs +++ b/glean-core/src/traits/event.rs @@ -13,23 +13,22 @@ use crate::ErrorType; /// /// Extra keys need to be pre-defined and map to a string representation. /// -/// For user-defined `EventMetric`s these will be defined as `enums`. -/// Each variant will correspond to an entry in the `ALLOWED_KEYS` list. +/// For user-defined `EventMetric`s these will be defined as `struct`s. +/// Each extra key will be a field in that struct. +/// Each field will correspond to an entry in the `ALLOWED_KEYS` list. /// The Glean SDK requires the keys as strings for submission in pings, /// whereas in code we want to provide users a type to work with /// (e.g. to avoid typos or misuse of the API). -pub trait ExtraKeys: Hash + Eq + PartialEq + Copy { +pub trait ExtraKeys { /// List of allowed extra keys as strings. const ALLOWED_KEYS: &'static [&'static str]; - /// The index of the extra key. + /// Convert the event extras into 2 lists: /// - /// It corresponds to its position in the associated `ALLOWED_KEYS` list. - /// - /// *Note*: An index of `-1` indicates an invalid / non-existing extra key. - /// Invalid / non-existing extra keys will be recorded as an error. - /// This cannot happen for generated code. - fn index(self) -> i32; + /// 1. The list of extra key indices. + /// Unset keys will be skipped. + /// 2. The list of extra values. + fn into_ffi_extra(self) -> HashMap; } /// Default of no extra keys for events. @@ -45,9 +44,8 @@ pub enum NoExtraKeys {} impl ExtraKeys for NoExtraKeys { const ALLOWED_KEYS: &'static [&'static str] = &[]; - fn index(self) -> i32 { - // This index will never be used. - -1 + fn into_ffi_extra(self) -> HashMap { + unimplemented!("non-existing extra keys can't be turned into a list") } } @@ -87,11 +85,8 @@ pub trait Event { /// /// # Arguments /// - /// * `extra` - A [`HashMap`] of (key, value) pairs. The key is one of the allowed extra keys as - /// set in the metric defintion. - /// If a wrong key is used or a value is larger than allowed, an error is reported - /// and no event is recorded. - fn record>>>(&self, extra: M); + /// * `extra` - (optional) An object for the extra keys. + fn record>>(&self, extra: M); /// **Exported for test purposes.** /// From 178749c188a18f1a0b3652bce29b749a9a366374 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Thu, 29 Apr 2021 14:35:57 +0200 Subject: [PATCH 05/13] Python: Allow new event extra properties to be passed as an object --- glean-core/python/glean/metrics/__init__.py | 3 +- glean-core/python/glean/metrics/event.py | 66 +++++++++++++++++-- glean-core/python/tests/metrics/test_event.py | 58 ++++++++++++++++ 3 files changed, 120 insertions(+), 7 deletions(-) diff --git a/glean-core/python/glean/metrics/__init__.py b/glean-core/python/glean/metrics/__init__.py index e6b87d41d1..4bb409bd92 100644 --- a/glean-core/python/glean/metrics/__init__.py +++ b/glean-core/python/glean/metrics/__init__.py @@ -11,7 +11,7 @@ from .boolean import BooleanMetricType from .counter import CounterMetricType from .datetime import DatetimeMetricType -from .event import EventMetricType, RecordedEventData +from .event import EventMetricType, RecordedEventData, EventExtras from .experiment import RecordedExperimentData from .quantity import QuantityMetricType from .jwe import JweMetricType @@ -47,6 +47,7 @@ "MemoryUnit", "PingType", "RecordedEventData", + "EventExtras", "RecordedExperimentData", "StringMetricType", "StringListMetricType", diff --git a/glean-core/python/glean/metrics/event.py b/glean-core/python/glean/metrics/event.py index 3d2069c65d..274897e898 100644 --- a/glean-core/python/glean/metrics/event.py +++ b/glean-core/python/glean/metrics/event.py @@ -4,7 +4,7 @@ import json -from typing import Dict, List, Optional +from typing import Dict, List, Optional, Union, Tuple from .. import _ffi @@ -15,6 +15,23 @@ from .lifetime import Lifetime +class EventExtras: + """ + A class that can be converted into key-value pairs of event extras. + This will be automatically implemented for event properties of an [EventMetricType]. + """ + + def to_ffi_extra(self) -> Tuple[List[int], List[str]]: + """ + Convert the event extras into 2 lists: + + 1. The list of extra key indices. + Unset keys will be skipped. + 2. The list of extra values. + """ + return ([], []) + + class RecordedEventData: """ Deserialized event data. @@ -110,22 +127,59 @@ def __del__(self): if getattr(self, "_handle", 0) != 0: _ffi.lib.glean_destroy_event_metric(self._handle) - def record(self, extra: Optional[Dict[int, str]] = None) -> None: + def record(self, extra: Optional[Union[Dict[int, str], EventExtras]] = None) -> None: """ Record an event by using the information provided by the instance of this class. Args: - extra (dict of (int, str)): optional. This is a map from keys - (which are enumerations) to values. This is used for events - where additional richer context is needed. The maximum length - for values is 100. + extra: optional. The extra keys and values for this event. + The maximum length for values is 100. """ if self._disabled: return timestamp = _ffi.lib.glean_get_timestamp_ms() + if isinstance(extra, EventExtras): + self._record_class(timestamp, extra) + else: + self._record_dict(timestamp, extra) + + def _record_class(self, timestamp: int, extra: EventExtras) -> None: + """ + Record an event by using the information provided by the instance of + this class. + + Args: + extra: This is the object specifying extra keys and their values. + This is used for events where additional richer context is needed. + The maximum length for values is 100. + """ + @Dispatcher.launch + def record(): + keys, values = extra.to_ffi_extra() + nextra = len(keys) + + _ffi.lib.glean_event_record( + self._handle, + timestamp, + _ffi.ffi_encode_vec_int32(keys), + _ffi.ffi_encode_vec_string(values), + nextra, + ) + + def _record_dict(self, timestamp: int, extra: Optional[Dict[int, str]] = None) -> None: + """ + Record an event by using the information provided by the instance of + this class. + + Args: + extra (dict of (int, str)): optional. This is a map from keys + (which are enumerations) to values. This is used for events + where additional richer context is needed. The maximum length + for values is 100. + """ @Dispatcher.launch def record(): if extra is None: diff --git a/glean-core/python/tests/metrics/test_event.py b/glean-core/python/tests/metrics/test_event.py index f51f5ca3f6..77a4984552 100644 --- a/glean-core/python/tests/metrics/test_event.py +++ b/glean-core/python/tests/metrics/test_event.py @@ -5,6 +5,7 @@ import enum from pathlib import Path import time +from typing import List, Optional, Tuple import pytest @@ -313,3 +314,60 @@ def test_event_enum_is_generated_correctly(): "key1": "value1", "key2": "value2", } == metrics.environment.event_example.test_get_value()[0].extra + + +def test_the_convenient_extrakeys_api(): + class ClickKeys(metrics.EventExtras): + def __init__(self, object_id: Optional[str] = None, other: Optional[str] = None) -> None: + self._object_id = object_id + self._other = other + + def to_ffi_extra(self) -> Tuple[List[int], List[str]]: + keys = [] + values = [] + + if self._object_id is not None: + keys.append(0) + values.append(self._object_id) + + if self._other is not None: + keys.append(1) + values.append(self._other) + + return (keys, values) + + click = metrics.EventMetricType( + disabled=False, + category="ui", + lifetime=Lifetime.PING, + name="click", + send_in_pings=["store1"], + allowed_extra_keys=["object_id", "other"], + ) + + # Record two events of the same type, with a little delay + click.record(ClickKeys(object_id="buttonA", other="foo")) + + time.sleep(0.001) + + # Some extra keys can be left undefined. + click.record(ClickKeys(object_id="buttonB")) + click.record() + + # Check that data was properly recorded + snapshot = click.test_get_value() + assert click.test_has_value() + assert 3 == len(snapshot) + + first_event = [x for x in snapshot if x.extra.get("object_id") == "buttonA"][0] + assert "ui" == first_event.category + assert "click" == first_event.name + assert "ui.click" == first_event.identifier + assert "foo" == first_event.extra["other"] + + second_event = [x for x in snapshot if x.extra.get("object_id") == "buttonB"][0] + assert "ui" == second_event.category + assert "click" == second_event.name + assert "other" not in second_event.extra + + assert first_event.timestamp < second_event.timestamp From 7143c8a1d1dda65adc4a293e3973d071a54bdb0f Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 30 Apr 2021 12:15:48 +0200 Subject: [PATCH 06/13] Python: Create new event extra API dynamically --- .gitignore | 2 +- glean-core/python/glean/_loader.py | 59 +++++++++++++++++-- glean-core/python/glean/metrics/event.py | 6 +- .../python/tests/data/events_with_types.yaml | 28 +++++++++ glean-core/python/tests/metrics/test_event.py | 15 +++++ 5 files changed, 102 insertions(+), 8 deletions(-) create mode 100644 glean-core/python/tests/data/events_with_types.yaml diff --git a/.gitignore b/.gitignore index 70e6d0d4a2..3be3042d30 100644 --- a/.gitignore +++ b/.gitignore @@ -51,7 +51,7 @@ dist/ /samples/csharp/.venv/ # local stuff -data/ +/data/ glean-core/data glean_app* !glean_app.c diff --git a/glean-core/python/glean/_loader.py b/glean-core/python/glean/_loader.py index 1b9d4efcb5..89acaea64f 100644 --- a/glean-core/python/glean/_loader.py +++ b/glean-core/python/glean/_loader.py @@ -11,7 +11,7 @@ import enum from pathlib import Path -from typing import Any, Generator, List, Optional, Tuple, Union +from typing import Any, Dict, Generator, List, Optional, Tuple, Union from glean_parser.parser import parse_objects # type: ignore @@ -90,6 +90,44 @@ def __getattr__(self, attr): ) +def _event_extra_factory(name: str, argnames: List[Tuple[str, str]]) -> Any: + """ + Generate a new class, inheriting from `metrics.EventExtras` + and implementing the `to_ffi_extra` method, + which serializes expected attributes to pass over FFI. + """ + + def __init__(self, **kwargs): + for key, value in kwargs.items(): + if key not in argnames: + raise TypeError( + f"Argument {key} not valid for {self.__class__.__name__}" + ) + setattr(self, key, value) + + def to_ffi_extra(self): + keys = [] + values = [] + + for idx, name in enumerate(argnames): + attr = getattr(self, name, None) + if attr is not None: + keys.append(idx) + # Special-case needed for booleans to turn them lowercase (true/false) + if isinstance(attr, bool): + values.append(str(attr).lower()) + else: + values.append(str(attr)) + + return (keys, values) + + attr = {name: None for (name, _) in argnames} # type: Dict[str, Any] + attr["__init__"] = __init__ + attr["to_ffi_extra"] = to_ffi_extra + newclass = type(name, (metrics.EventExtras,), attr) + return newclass + + def _get_metric_objects( name: str, metric: glean_parser.metrics.Metric ) -> Generator[Tuple[str, Any], None, None]: @@ -115,11 +153,20 @@ def _get_metric_objects( # Events and Pings also need to define an enumeration if metric.type == "event": - enum_name = name + "_keys" - class_name = Camelize(enum_name) - values = dict((x.upper(), i) for (i, x) in enumerate(metric.allowed_extra_keys)) - keys_enum = enum.Enum(class_name, values) # type: ignore - yield enum_name, keys_enum + if metric.has_extra_types: + class_name = name + "_extra" + class_name = Camelize(class_name) + values = metric.allowed_extra_keys + keys_class = event_extra_factory(class_name, values) # type: ignore + yield class_name, keys_class + else: + enum_name = name + "_keys" + class_name = Camelize(enum_name) + values = dict( + (x.upper(), i) for (i, x) in enumerate(metric.allowed_extra_keys) + ) + keys_enum = enum.Enum(class_name, values) # type: ignore + yield enum_name, keys_enum elif metric.type == "ping": enum_name = name + "_reason_codes" class_name = Camelize(enum_name) diff --git a/glean-core/python/glean/metrics/event.py b/glean-core/python/glean/metrics/event.py index 274897e898..9257608e6b 100644 --- a/glean-core/python/glean/metrics/event.py +++ b/glean-core/python/glean/metrics/event.py @@ -156,6 +156,7 @@ def _record_class(self, timestamp: int, extra: EventExtras) -> None: This is used for events where additional richer context is needed. The maximum length for values is 100. """ + @Dispatcher.launch def record(): keys, values = extra.to_ffi_extra() @@ -169,7 +170,9 @@ def record(): nextra, ) - def _record_dict(self, timestamp: int, extra: Optional[Dict[int, str]] = None) -> None: + def _record_dict( + self, timestamp: int, extra: Optional[Dict[int, str]] = None + ) -> None: """ Record an event by using the information provided by the instance of this class. @@ -180,6 +183,7 @@ def _record_dict(self, timestamp: int, extra: Optional[Dict[int, str]] = None) - where additional richer context is needed. The maximum length for values is 100. """ + @Dispatcher.launch def record(): if extra is None: diff --git a/glean-core/python/tests/data/events_with_types.yaml b/glean-core/python/tests/data/events_with_types.yaml new file mode 100644 index 0000000000..a33da488ab --- /dev/null +++ b/glean-core/python/tests/data/events_with_types.yaml @@ -0,0 +1,28 @@ +# Any copyright is dedicated to the Public Domain. +# https://creativecommons.org/publicdomain/zero/1.0/ + +--- +$schema: moz://mozilla.org/schemas/glean/metrics/2-0-0 + +core: + preference_toggled: + type: event + description: | + Just testing events + bugs: + - https://bugzilla.mozilla.org/1123456789 + data_reviews: + - http://example.com/reviews + notification_emails: + - CHANGE-ME@example.com + extra_keys: + preference: + type: string + description: "This is key one" + enabled: + type: boolean + description: "This is key two" + swapped: + type: quantity + description: "This is key three" + expires: never diff --git a/glean-core/python/tests/metrics/test_event.py b/glean-core/python/tests/metrics/test_event.py index 77a4984552..f1c98f1fd0 100644 --- a/glean-core/python/tests/metrics/test_event.py +++ b/glean-core/python/tests/metrics/test_event.py @@ -316,6 +316,21 @@ def test_event_enum_is_generated_correctly(): } == metrics.environment.event_example.test_get_value()[0].extra +def test_event_class_is_generated_correctly(): + metrics = load_metrics( + ROOT.parent / "data" / "events_with_types.yaml", config={"allow_reserved": False} + ) + + metrics.core.preference_toggled.record( + metrics.core.PreferenceToggledClass(preference="value1", enabled=True) + ) + + assert { + "preference": "value1", + "enabled": "true", + } == metrics.core.preference_toggled.test_get_value()[0].extra + + def test_the_convenient_extrakeys_api(): class ClickKeys(metrics.EventExtras): def __init__(self, object_id: Optional[str] = None, other: Optional[str] = None) -> None: From ffab21022c2ca72cfdf5c5556455cf21e4a32caf Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 30 Apr 2021 12:29:21 +0200 Subject: [PATCH 07/13] Python: Ignore build-time generate when linting When locally building or installing glean this file is generated. It trips up flake8, because it definitely does not correspond to its standards. It's auto-generated, so there's no need for linting, let's just ignore it. --- .flake8 | 1 + .gitignore | 1 + 2 files changed, 2 insertions(+) diff --git a/.flake8 b/.flake8 index 7da1f9608e..9b916e1c8d 100644 --- a/.flake8 +++ b/.flake8 @@ -1,2 +1,3 @@ [flake8] max-line-length = 100 +exclude = glean-core/python/glean/_glean_ffi.py diff --git a/.gitignore b/.gitignore index 3be3042d30..fed6b68777 100644 --- a/.gitignore +++ b/.gitignore @@ -62,6 +62,7 @@ samples/ios/app/.venv/ htmlcov/ *.log +glean-core/python/glean/_glean_ffi.py __pycache__ *.py[cod] .Python From a126742114bf7791b5514846ac02e8556304d15d Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 3 May 2021 11:28:02 +0200 Subject: [PATCH 08/13] Python: Dynamic type checks for new event extra API --- glean-core/python/glean/_loader.py | 32 ++++++++++++----- glean-core/python/tests/metrics/test_event.py | 34 +++++++++++++++++-- 2 files changed, 56 insertions(+), 10 deletions(-) diff --git a/glean-core/python/glean/_loader.py b/glean-core/python/glean/_loader.py index 89acaea64f..3b27380c5e 100644 --- a/glean-core/python/glean/_loader.py +++ b/glean-core/python/glean/_loader.py @@ -99,9 +99,20 @@ def _event_extra_factory(name: str, argnames: List[Tuple[str, str]]) -> Any: def __init__(self, **kwargs): for key, value in kwargs.items(): - if key not in argnames: + typ = next((t for (k, t) in argnames if key == k), None) + if typ is None: raise TypeError( - f"Argument {key} not valid for {self.__class__.__name__}" + f"Argument '{key}' not valid for {self.__class__.__name__}" + ) + elif typ == "boolean" and isinstance(value, bool): + pass + elif typ == "string" and isinstance(value, str): + pass + elif typ == "quantity" and isinstance(value, int): + pass + else: + raise TypeError( + f"Field '{key}' requires type {typ} in {self.__class__.__name__}" ) setattr(self, key, value) @@ -109,15 +120,20 @@ def to_ffi_extra(self): keys = [] values = [] - for idx, name in enumerate(argnames): + for idx, (name, typ) in enumerate(argnames): attr = getattr(self, name, None) if attr is not None: keys.append(idx) - # Special-case needed for booleans to turn them lowercase (true/false) - if isinstance(attr, bool): + if typ == "boolean" and isinstance(attr, bool): + # Special-case needed for booleans to turn them lowercase (true/false) values.append(str(attr).lower()) - else: + elif typ == "string" and isinstance(attr, str): values.append(str(attr)) + elif typ == "quantity" and isinstance(attr, int): + values.append(str(attr)) + # Don't support other data types + else: + raise TypeError(f"Type {type(attr)} not supported for {name}") return (keys, values) @@ -156,8 +172,8 @@ def _get_metric_objects( if metric.has_extra_types: class_name = name + "_extra" class_name = Camelize(class_name) - values = metric.allowed_extra_keys - keys_class = event_extra_factory(class_name, values) # type: ignore + values = metric.allowed_extra_keys_with_types + keys_class = _event_extra_factory(class_name, values) # type: ignore yield class_name, keys_class else: enum_name = name + "_keys" diff --git a/glean-core/python/tests/metrics/test_event.py b/glean-core/python/tests/metrics/test_event.py index f1c98f1fd0..c2637fbf10 100644 --- a/glean-core/python/tests/metrics/test_event.py +++ b/glean-core/python/tests/metrics/test_event.py @@ -316,13 +316,13 @@ def test_event_enum_is_generated_correctly(): } == metrics.environment.event_example.test_get_value()[0].extra -def test_event_class_is_generated_correctly(): +def test_event_extra_is_generated_correctly(): metrics = load_metrics( ROOT.parent / "data" / "events_with_types.yaml", config={"allow_reserved": False} ) metrics.core.preference_toggled.record( - metrics.core.PreferenceToggledClass(preference="value1", enabled=True) + metrics.core.PreferenceToggledExtra(preference="value1", enabled=True) ) assert { @@ -386,3 +386,33 @@ def to_ffi_extra(self) -> Tuple[List[int], List[str]]: assert "other" not in second_event.extra assert first_event.timestamp < second_event.timestamp + + +def test_event_extra_does_typechecks(): + metrics = load_metrics( + ROOT.parent / "data" / "events_with_types.yaml", config={"allow_reserved": False} + ) + + # Valid combinations of extras. + # These do not throw. + metrics.core.PreferenceToggledExtra(preference="value1") + metrics.core.PreferenceToggledExtra(enabled=True) + metrics.core.PreferenceToggledExtra(swapped=1) + extras = metrics.core.PreferenceToggledExtra(preference="value1", enabled=True, swapped=1) + # Check conversion to FFI types, extras are sorted by name + ffi = extras.to_ffi_extra() + expected = ([0, 1, 2], ["true", "value1", "1"]) + assert expected == ffi + + with pytest.raises(TypeError): + metrics.core.PreferenceToggledExtra(preference=True) + with pytest.raises(TypeError): + metrics.core.PreferenceToggledExtra(enabled=1) + with pytest.raises(TypeError): + metrics.core.PreferenceToggledExtra(swapped="string") + + # Modifying an attribute only checks on conversion to FFI + extras = metrics.core.PreferenceToggledExtra(preference="string") + extras.preference = True + with pytest.raises(TypeError): + extras.to_ffi_extra() From 3a1b8502c26ede96d7aa14933272c7ee6829f5be Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Wed, 5 May 2021 13:55:37 +0200 Subject: [PATCH 09/13] Document new event API (and deprecate the old one) --- docs/user/reference/metrics/event.md | 133 +++++++++++++++++++++------ 1 file changed, 104 insertions(+), 29 deletions(-) diff --git a/docs/user/reference/metrics/event.md b/docs/user/reference/metrics/event.md index 615cc28ff4..0b745492bb 100644 --- a/docs/user/reference/metrics/event.md +++ b/docs/user/reference/metrics/event.md @@ -18,57 +18,50 @@ Each event contains the following data: ## Recording API -### `record` +### `record(object)` -Record a new event, with optional extra values. +_Added in: [v38.0.0](../../appendix/changelog.md)_ + + +Record a new event, with optional typed extra values. +This requires `type` annotations in the definition. +See [Extra metrics parameters](#extra-metric-parameters). {{#include ../../../shared/tab_header.md}}
-Note that an `enum` has been generated for handling the `extra_keys`: it has the same name as the event metric, with `Keys` added. +Note that an `enum` has been generated for handling the `extra_keys`: it has the same name as the event metric, with `Extra` added. ```Kotlin import org.mozilla.yourApplication.GleanMetrics.Views -Views.loginOpened.record(mapOf(Views.loginOpenedKeys.sourceOfLogin to "toolbar")) +Views.loginOpened.record(Views.loginOpenedExtra(sourceOfLogin = "toolbar")) ```
-
- -```Java -import org.mozilla.yourApplication.GleanMetrics.Views; - -Views.INSTANCE.loginOpened.record(); -``` - -
+
-Note that an `enum` has been generated for handling the `extra_keys`: it has the same name as the event metric, with `Keys` added. +Note that an `enum` has been generated for handling the `extra_keys`: it has the same name as the event metric, with `Extra` added. ```Swift -Views.loginOpened.record(extra: [.sourceOfLogin: "toolbar"]) +Views.loginOpened.record(LoginOpenedExtra(sourceOfLogin: "toolbar")) ```
-Note that an `enum` has been generated for handling the `extra_keys`: it has the same name as the event metric, with `_keys` added. +Note that an `enum` has been generated for handling the `extra_keys`: it has the same name as the event metric, with `_extra` added. ```Python from glean import load_metrics metrics = load_metrics("metrics.yaml") -metrics.views.login_opened.record( - { - metrics.views.login_opened_keys.SOURCE_OF_LOGIN: "toolbar" - } -) +metrics.views.login_opened.record(LoginOpenedExtra(sourceOfLogin="toolbar")) ```
@@ -78,10 +71,9 @@ metrics.views.login_opened.record( Note that an `enum` has been generated for handling the `extra_keys`: it has the same name as the event metric, with `Keys` added. ```Rust -use metrics::views; +use metrics::views::{self, LoginOpenedExtra}; -let mut extra = HashMap::new(); -extra.insert(views::LoginOpenedKeys::SourceOfLogin, "toolbar".into()); +let extra = LoginOpenedExtra { source_of_login: Some("toolbar".to_string()) }; views::login_opened.record(extra); ``` @@ -104,11 +96,8 @@ views.loginOpened.record({ sourceOfLogin: "toolbar" }); ```c++ #include "mozilla/glean/GleanMetrics.h" -using mozilla::glean::views::LoginOpenedKeys; -nsTArray> extra; -nsCString source = "toolbar"_ns; -extra.AppendElement(MakeTuple(LoginOpenedKeys::SourceOfLogin, source)); - +using mozilla::glean::views::LoginOpenedExtra; +LoginOpenedExtra extra = { .source_of_login = Some("value"_ns) }; mozilla::glean::views::login_opened.Record(std::move(extra)) ``` @@ -121,6 +110,89 @@ Glean.views.loginOpened.record(extra); +{{#include ../../../shared/tab_footer.md}} + +### `record(map)` (_deprecated_) + +_Deprecated in: [v38.0.0](../../appendix/changelog.md)_ + + +Record a new event, with optional extra values. + +{{#include ../../../shared/blockquote-info.html}} + +##### Deprecation notice + +> This API is used in v38.0.0 if an event has no `type` annotations in the definition. +> See [Extra metrics parameters](#extra-metric-parameters). +> +> In future versions extra values will default to a `string` type and this API will be removed. +> In Rust and Firefox Desktop this API is not supported. + +{{#include ../../../shared/tab_header.md}} + +
+ +Note that an `enum` has been generated for handling the `extra_keys`: it has the same name as the event metric, with `Keys` added. + +```Kotlin +import org.mozilla.yourApplication.GleanMetrics.Views + +Views.loginOpened.record(mapOf(Views.loginOpenedKeys.sourceOfLogin to "toolbar")) +``` + +
+ +
+ +```Java +import org.mozilla.yourApplication.GleanMetrics.Views; + +Views.INSTANCE.loginOpened.record(); +``` + +
+ +
+ +Note that an `enum` has been generated for handling the `extra_keys`: it has the same name as the event metric, with `Keys` added. + +```Swift +Views.loginOpened.record(extra: [.sourceOfLogin: "toolbar"]) +``` + +
+ +
+ +Note that an `enum` has been generated for handling the `extra_keys`: it has the same name as the event metric, with `_keys` added. + +```Python +from glean import load_metrics +metrics = load_metrics("metrics.yaml") + +metrics.views.login_opened.record( + { + metrics.views.login_opened_keys.SOURCE_OF_LOGIN: "toolbar" + } +) +``` + +
+ +
+ +
+ +```js +import * as views from "./path/to/generated/files/views.js"; + +views.loginOpened.record({ sourceOfLogin: "toolbar" }); +``` + +
+ +
{{#include ../../../shared/tab_footer.md}} @@ -383,6 +455,7 @@ views: extra_keys: source_of_login: description: The source from which the login view was opened, e.g. "toolbar". + type: string ``` For a full reference on metrics parameters common to all metric types, @@ -398,6 +471,8 @@ A maximum of 10 extra keys is allowed. Each extra key contains additional metadata: - `description`: **Required.** A description of the key. +* `type`: The type of value this extra key can hold. One of `string`, `boolean`, `quantity`. Defaults to `string`. + **Note**: If not specified only the legacy API on `record` is available. ## Data questions From 269741426582955de85116114a4002b3c11a2088 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Tue, 11 May 2021 17:37:33 +0200 Subject: [PATCH 10/13] Add extensive changelog entry for new event API --- CHANGELOG.md | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50e50ca883..75abd863c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,42 @@ [Full changelog](https://github.com/mozilla/glean/compare/v38.0.1...main) +* General + * Add new event extras API to all implementations. See below for details ([#1603](https://github.com/mozilla/glean/pull/1603)) +* Rust + * **Breaking Change**: Allow event extras to be passed as an object. + This replaces the old `HashMap`-based API. + Values default to `string`. + See [the event documentation](https://mozilla.github.io/glean/book/reference/metrics/event.html#recordobject) for details. + ([#1603](https://github.com/mozilla/glean/pull/1603)) + Old code: + + ``` + let mut extra = HashMap::new(); + extra.insert(SomeExtra::Key1, "1".into()); + extra.insert(SomeExtra::Key2, "2".into()); + metric.record(extra); + ``` + + New code: + + ``` + let extra = SomeExtra { + key1: Some("1".into()), + key2: Some("2".into()), + }; + metric.record(extra); + ``` +* Android + * **Deprecation**: The old event recording API is replaced by a new one, accepting a typed object ([#1603](https://github.com/mozilla/glean/pull/1603)). + See [the event documentation](https://mozilla.github.io/glean/book/reference/metrics/event.html#recordobject) for details. +* Python + * **Deprecation**: The old event recording API is replaced by a new one, accepting a typed object ([#1603](https://github.com/mozilla/glean/pull/1603)). + See [the event documentation](https://mozilla.github.io/glean/book/reference/metrics/event.html#recordobject) for details. +* Swift + * **Deprecation**: The old event recording API is replaced by a new one, accepting a typed object ([#1603](https://github.com/mozilla/glean/pull/1603)). + See [the event documentation](https://mozilla.github.io/glean/book/reference/metrics/event.html#recordobject) for details. + # v38.0.1 (2021-05-17) [Full changelog](https://github.com/mozilla/glean/compare/v38.0.0...v38.0.1) From 74ae2a36e51b71b98a99766c7f62bf9a8e68915a Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 21 May 2021 15:10:25 +0200 Subject: [PATCH 11/13] Kotlin: Make an event metric generic over both the old and new style Only one will be active, depending on whether the definition has type annotations. However to make it actually usable we need both generics around for now. When we remove the deprecated old style it will require removing the generic type here and changing the glean_parser --- .../glean/private/EventMetricType.kt | 46 +++++++++++-------- .../java/mozilla/telemetry/glean/GleanTest.kt | 3 +- .../telemetry/glean/pings/CustomPingTest.kt | 3 +- .../glean/private/EventMetricTypeTest.kt | 30 ++++++------ .../glean/private/LabeledMetricTypeTest.kt | 4 +- samples/android/app/metrics.yaml | 19 ++++++++ .../org/mozilla/samples/glean/MainActivity.kt | 16 ++++--- 7 files changed, 77 insertions(+), 44 deletions(-) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/EventMetricType.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/EventMetricType.kt index 33accc1a73..e26d28dddb 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/EventMetricType.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/EventMetricType.kt @@ -66,7 +66,17 @@ interface EventExtras { * Unset keys will be skipped. * 2. The list of extra values. */ - fun toFfiExtra(): Pair + fun toFfiExtra(): Pair> +} + +/** + * An object with no values for convenient use as the default set of extra keys + * that an [EventMetricType] can accept. + */ +class NoExtras : EventExtras { + override fun toFfiExtra(): Pair> { + return Pair(IntArray(0), listOf()) + } } /** @@ -78,7 +88,7 @@ interface EventExtras { * The Events API only exposes the [record] method, which takes care of validating the input * data and making sure that limits are enforced. */ -class EventMetricType> internal constructor( +class EventMetricType, ExtraObject : EventExtras> internal constructor( private var handle: Long, private val disabled: Boolean, private val sendInPings: List @@ -121,9 +131,8 @@ class EventMetricType> internal constructor( * identifiers. This is used for events where additional richer context is needed. * The maximum length for values is 100 bytes. */ - @JvmOverloads @Deprecated("Specify types for your event extras. See the reference for details.") - fun record(extra: Map? = null) { + fun record(extra: Map) { if (disabled) { return } @@ -139,15 +148,10 @@ class EventMetricType> internal constructor( // In Kotlin, Map.keys and Map.values are not guaranteed to return the entries // in any particular order. Therefore, we iterate over the pairs together and // create the keys and values arrays step-by-step. - var keys: IntArray? = null - var values: StringArray? = null - var len: Int = 0 - if (extra != null) { - val extraList = extra.toList() - keys = IntArray(extra.size, { extraList[it].first.ordinal }) - values = StringArray(Array(extra.size, { extraList[it].second }), "utf-8") - len = extra.size - } + val extraList = extra.toList() + val keys = IntArray(extra.size, { extraList[it].first.ordinal }) + val values = StringArray(Array(extra.size, { extraList[it].second }), "utf-8") + val len = extra.size LibGleanFFI.INSTANCE.glean_event_record( this@EventMetricType.handle, @@ -170,7 +174,8 @@ class EventMetricType> internal constructor( * Note: `extra` is not optional here to avoid overlapping with the above definition of `record`. * If no `extra` data is passed the above function will be invoked correctly. */ - fun record(extra: EventExtras) { + @JvmOverloads + fun record(extra: ExtraObject? = null) { if (disabled) { return } @@ -181,10 +186,15 @@ class EventMetricType> internal constructor( @Suppress("EXPERIMENTAL_API_USAGE") Dispatchers.API.launch { - val extras = extra.toFfiExtra() - val keys = extras.first - val values = extras.second - val len = keys.size + var keys: IntArray? = null + var values: StringArray? = null + var len: Int = 0 + if (extra != null) { + val extras = extra.toFfiExtra() + keys = extras.first + values = StringArray(extras.second.toTypedArray(), "utf-8") + len = keys.size + } LibGleanFFI.INSTANCE.glean_event_record( this@EventMetricType.handle, diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt index cf8aec0e43..e1e50d074f 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt @@ -23,6 +23,7 @@ import mozilla.telemetry.glean.private.CounterMetricType import mozilla.telemetry.glean.private.EventMetricType import mozilla.telemetry.glean.private.Lifetime import mozilla.telemetry.glean.private.NoExtraKeys +import mozilla.telemetry.glean.private.NoExtras import mozilla.telemetry.glean.private.NoReasonCodes import mozilla.telemetry.glean.private.PingType import mozilla.telemetry.glean.private.StringMetricType @@ -181,7 +182,7 @@ class GleanTest { fun `test sending of foreground and background pings`() { val server = getMockWebServer() - val click = EventMetricType( + val click = EventMetricType( disabled = false, category = "ui", lifetime = Lifetime.Ping, diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt index 78231aae11..5fc1761090 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt @@ -12,6 +12,7 @@ import mozilla.telemetry.glean.getMockWebServer import mozilla.telemetry.glean.private.EventMetricType import mozilla.telemetry.glean.private.Lifetime import mozilla.telemetry.glean.private.NoExtraKeys +import mozilla.telemetry.glean.private.NoExtras import mozilla.telemetry.glean.private.NoReasonCodes import mozilla.telemetry.glean.private.PingType import mozilla.telemetry.glean.resetGlean @@ -137,7 +138,7 @@ class CustomPingTest { val pingName = "custom-events-1" // Define a 'click' event - val click = EventMetricType( + val click = EventMetricType( disabled = false, category = "ui", lifetime = Lifetime.Ping, diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt index d5ba8457ff..ee59a68f3f 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt @@ -37,7 +37,6 @@ import org.junit.Assert.fail import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith -import com.sun.jna.StringArray // Declared here, since Kotlin can not declare nested enum classes. enum class clickKeys { @@ -48,7 +47,7 @@ enum class clickKeys { // The event extra properties. // This would be generated by the glean_parser usually. data class ClickExtras(val objectId: String? = null, val other: String? = null) : EventExtras { - override fun toFfiExtra(): Pair { + override fun toFfiExtra(): Pair> { var keys = mutableListOf() var values = mutableListOf() @@ -60,7 +59,7 @@ data class ClickExtras(val objectId: String? = null, val other: String? = null) keys.add(1) values.add(it) } - return Pair(IntArray(keys.size, { keys[it] }), StringArray(values.toTypedArray(), "utf-8")) + return Pair(IntArray(keys.size, { keys[it] }), values) } } @@ -82,9 +81,8 @@ class EventMetricTypeTest { @Test fun `The API records to its storage engine`() { - // Define a 'click' event, which will be stored in "store1" - val click = EventMetricType( + val click = EventMetricType( disabled = false, category = "ui", lifetime = Lifetime.Ping, @@ -99,7 +97,7 @@ class EventMetricTypeTest { val expectedTimeSinceStart: Long = 37 SystemClock.sleep(expectedTimeSinceStart) - click.record(extra = mapOf(clickKeys.objectId to "buttonB", clickKeys.other to "bar")) + click.record(ClickExtras(objectId = "buttonB", other = "bar")) // Check that data was properly recorded. val snapshot = click.testGetValue() @@ -124,7 +122,7 @@ class EventMetricTypeTest { @Test fun `The API records to its storage engine when category is empty`() { // Define a 'click' event, which will be stored in "store1" - val click = EventMetricType( + val click = EventMetricType( disabled = false, category = "", lifetime = Lifetime.Ping, @@ -161,7 +159,7 @@ class EventMetricTypeTest { fun `disabled events must not record data`() { // Define a 'click' event, which will be stored in "store1". It's disabled // so it should not record anything. - val click = EventMetricType( + val click = EventMetricType( disabled = true, category = "ui", lifetime = Lifetime.Ping, @@ -179,7 +177,7 @@ class EventMetricTypeTest { @Test(expected = NullPointerException::class) fun `testGetValue() throws NullPointerException if nothing is stored`() { - val testEvent = EventMetricType( + val testEvent = EventMetricType( disabled = false, category = "ui", lifetime = Lifetime.Ping, @@ -192,7 +190,7 @@ class EventMetricTypeTest { @Test fun `The API records to secondary pings`() { // Define a 'click' event, which will be stored in "store1" and "store2" - val click = EventMetricType( + val click = EventMetricType( disabled = false, category = "ui", lifetime = Lifetime.Ping, @@ -229,7 +227,7 @@ class EventMetricTypeTest { @Test fun `events should not record when upload is disabled`() { - val eventMetric = EventMetricType( + val eventMetric = EventMetricType( disabled = false, category = "ui", lifetime = Lifetime.Ping, @@ -269,7 +267,7 @@ class EventMetricTypeTest { clearStores = true ) - val event = EventMetricType( + val event = EventMetricType( disabled = false, category = "telemetry", name = "test_event", @@ -329,7 +327,7 @@ class EventMetricTypeTest { clearStores = true ) - val event = EventMetricType( + val event = EventMetricType( disabled = false, category = "telemetry", name = "test_event", @@ -406,7 +404,7 @@ class EventMetricTypeTest { @Test fun `Long extra values record an error`() { // Define a 'click' event, which will be stored in "store1" - val click = EventMetricType( + val click = EventMetricType( disabled = false, category = "ui", lifetime = Lifetime.Ping, @@ -437,7 +435,7 @@ class EventMetricTypeTest { ) val pingName = "another-ping" - val event = EventMetricType( + val event = EventMetricType( disabled = false, category = "telemetry", name = "test_event", @@ -512,7 +510,7 @@ class EventMetricTypeTest { ) val pingName = "another-ping-2" - val event = EventMetricType( + val event = EventMetricType( disabled = false, category = "telemetry", name = "test_event", diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/LabeledMetricTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/LabeledMetricTypeTest.kt index ec380f52f7..5c22595540 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/LabeledMetricTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/LabeledMetricTypeTest.kt @@ -398,7 +398,7 @@ class LabeledMetricTypeTest { @Test(expected = IllegalStateException::class) fun `Test that labeled events are an exception`() { - val eventMetric = EventMetricType( + val eventMetric = EventMetricType( disabled = false, category = "telemetry", lifetime = Lifetime.Application, @@ -406,7 +406,7 @@ class LabeledMetricTypeTest { sendInPings = listOf("metrics") ) - val labeledEventMetric = LabeledMetricType>( + val labeledEventMetric = LabeledMetricType>( disabled = false, category = "telemetry", lifetime = Lifetime.Application, diff --git a/samples/android/app/metrics.yaml b/samples/android/app/metrics.yaml index 0585a179fc..f77cfa5483 100644 --- a/samples/android/app/metrics.yaml +++ b/samples/android/app/metrics.yaml @@ -12,6 +12,25 @@ $schema: moz://mozilla.org/schemas/glean/metrics/2-0-0 browser.engagement: click: + type: event + description: | + Just testing events + bugs: + - https://bugzilla.mozilla.org/123456789 + data_reviews: + - N/A + notification_emails: + - CHANGE-ME@example.com + extra_keys: + key1: + type: string + description: "This is key one" + key2: + type: string + description: "This is key two" + expires: 2100-01-01 + + old_event_style: type: event description: | Just testing events diff --git a/samples/android/app/src/main/java/org/mozilla/samples/glean/MainActivity.kt b/samples/android/app/src/main/java/org/mozilla/samples/glean/MainActivity.kt index 2646f50fa5..e8ffbf858e 100644 --- a/samples/android/app/src/main/java/org/mozilla/samples/glean/MainActivity.kt +++ b/samples/android/app/src/main/java/org/mozilla/samples/glean/MainActivity.kt @@ -36,13 +36,17 @@ open class MainActivity : AppCompatActivity() { // order to illustrate adding extra information to the event, it is also adding to the // 'extras' field a dictionary of values. Note that the dictionary keys must be // declared in the metrics.yaml file under the 'extra_keys' section of an event metric. + BrowserEngagement.click.record(BrowserEngagement.ClickExtra(key1 = "extra_value_1", key2 = "extra_value_2")) + + // An event without any extra keys + BrowserEngagement.eventNoKeys.record() + + // Testing the old API. It should still be possible, even if deprecated @Suppress("DEPRECATION") - BrowserEngagement.click.record( - mapOf( - BrowserEngagement.clickKeys.key1 to "extra_value_1", - BrowserEngagement.clickKeys.key2 to "extra_value_2" - ) - ) + BrowserEngagement.oldEventStyle.record(mapOf( + BrowserEngagement.oldEventStyleKeys.key1 to "extra_value1", + BrowserEngagement.oldEventStyleKeys.key2 to "extra_value2" + )) } uploadSwitch.setOnCheckedChangeListener { _, isChecked -> From 900807f44ca558abb3602648f93a0ef570f3aa06 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 21 May 2021 16:38:36 +0200 Subject: [PATCH 12/13] Swift: Make an event metric generic over both the old and new style --- .../ios/Glean/Metrics/EventMetric.swift | 36 +++++++++++++------ .../Debug/GleanDebugUtilityTests.swift | 2 +- .../GleanTests/Metrics/EventMetricTests.swift | 20 +++++------ .../Metrics/LabeledMetricTests.swift | 4 +-- .../app/glean-sample-app/ViewController.swift | 11 +++--- .../EventPingTest.swift | 12 ++++--- samples/ios/app/metrics.yaml | 21 +++++++++++ 7 files changed, 74 insertions(+), 32 deletions(-) diff --git a/glean-core/ios/Glean/Metrics/EventMetric.swift b/glean-core/ios/Glean/Metrics/EventMetric.swift index d6f736c401..4e373a890b 100644 --- a/glean-core/ios/Glean/Metrics/EventMetric.swift +++ b/glean-core/ios/Glean/Metrics/EventMetric.swift @@ -56,6 +56,16 @@ public enum NoExtraKeys: ExtraKeys { } } +/// Default of no extra keys for events (for the new API). +/// +/// An empty class for convenient use as the default set of extra keys +/// that an `EventMetricType` can accept. +public class NoExtras: EventExtras { + public func toFfiExtra() -> ([Int32], [String]) { + return ([Int32](), [String]()) + } +} + /// Default-implement `EventExtras` for a map of ExtraKeys to Strings. extension Dictionary: EventExtras where Key: ExtraKeys, Value == String { public func toFfiExtra() -> ([Int32], [String]) { @@ -80,7 +90,7 @@ extension Dictionary: EventExtras where Key: ExtraKeys, Value == String { /// /// The Events API only exposes the `EventMetricType.record(extra:)` method, which takes care of validating the input /// data and making sure that limits are enforced. -public class EventMetricType { +public class EventMetricType { let handle: UInt64 let disabled: Bool let sendInPings: [String] @@ -126,16 +136,21 @@ public class EventMetricType { /// values need to be strings. /// This is used for events where additional richer context is needed. /// The maximum length for values is defined by `MAX_LENGTH_EXTRA_KEY_VALUE`. - public func record(extra: [ExtraKeysEnum: String]? = nil) { - self.record(extra) + @available(*, deprecated, message: "Specify types for your event extras. See the reference for details.") + public func record(extra: [ExtraKeysEnum: String]) { + let timestamp = glean_get_timestamp_ms() + self.recordInternal(timestamp: timestamp, extras: extra.toFfiExtra()) } - public func record(_ properties: EventExtras? = nil) { - guard !self.disabled else { return } - + public func record(_ properties: ExtraObject? = nil) { // We capture the event time now, since we don't know when the async code below // might get executed. let timestamp = glean_get_timestamp_ms() + self.recordInternal(timestamp: timestamp, extras: properties.map { $0.toFfiExtra() }) + } + + func recordInternal(timestamp: UInt64, extras: ([Int32], [String])?) { + guard !self.disabled else { return } Dispatchers.shared.launchAPI { // The map is sent over FFI as a pair of arrays, one containing the @@ -144,11 +159,10 @@ public class EventMetricType { var keys: [Int32]? var values: [String]? - if let extra = properties { - let (k, v) = extra.toFfiExtra() - len = k.count - keys = k - values = v + if let extra = extras { + keys = extra.0 + values = extra.1 + len = extra.0.count } withArrayOfCStrings(values) { values in diff --git a/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift b/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift index 228e342aae..8b2dd12ec9 100644 --- a/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift +++ b/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift @@ -74,7 +74,7 @@ class GleanDebugUtilityTests: XCTestCase { // Create a dummy event and a dummy metric so that the // respective pings will be sent - let event = EventMetricType( + let event = EventMetricType( category: "ui", name: "click", sendInPings: ["events"], diff --git a/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift b/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift index 1521917e40..4e3da2cefd 100644 --- a/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift +++ b/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift @@ -88,7 +88,7 @@ class EventMetricTypeTests: XCTestCase { } func testEventSavesToStorage() { - let metric = EventMetricType( + let metric = EventMetricType( category: "ui", name: "click", sendInPings: ["store1"], @@ -108,7 +108,7 @@ class EventMetricTypeTests: XCTestCase { // Old API, this is available only because we manually implemented the enum. // Generated code will have only one of the APIs available. - metric.record(extra: [.objectId: "buttonB", .other: "bar"]) + metric.record(ClickExtras(objectId: "buttonB", other: "bar")) XCTAssert(metric.testHasValue()) let events = try! metric.testGetValue() @@ -133,7 +133,7 @@ class EventMetricTypeTests: XCTestCase { } func testEventRecordedWithEmptyCategory() { - let metric = EventMetricType( + let metric = EventMetricType( category: "", name: "click", sendInPings: ["store1"], @@ -161,7 +161,7 @@ class EventMetricTypeTests: XCTestCase { } func testEventNotRecordedWhenDisabled() { - let metric = EventMetricType( + let metric = EventMetricType( category: "ui", name: "click", sendInPings: ["store1"], @@ -177,7 +177,7 @@ class EventMetricTypeTests: XCTestCase { } func testCounterGetValueThrowsExceptionIfNothingIsStored() { - let metric = EventMetricType( + let metric = EventMetricType( category: "ui", name: "click", sendInPings: ["store1"], @@ -191,7 +191,7 @@ class EventMetricTypeTests: XCTestCase { } func testEventSavesToSecondaryPings() { - let metric = EventMetricType( + let metric = EventMetricType( category: "ui", name: "click", sendInPings: ["store1", "store2"], @@ -222,7 +222,7 @@ class EventMetricTypeTests: XCTestCase { } func testEventNotRecordWhenUploadDisabled() { - let metric = EventMetricType( + let metric = EventMetricType( category: "ui", name: "click", sendInPings: ["store1", "store2"], @@ -250,7 +250,7 @@ class EventMetricTypeTests: XCTestCase { setupHttpResponseStub() expectation = expectation(description: "Completed upload") - let event = EventMetricType( + let event = EventMetricType( category: "telemetry", name: "test_event", sendInPings: ["events"], @@ -282,7 +282,7 @@ class EventMetricTypeTests: XCTestCase { setupHttpResponseStub() expectation = expectation(description: "Completed upload") - let event = EventMetricType( + let event = EventMetricType( category: "telemetry", name: "test_event", sendInPings: ["events"], @@ -327,7 +327,7 @@ class EventMetricTypeTests: XCTestCase { } func testEventLongExtraRecordsError() { - let metric = EventMetricType( + let metric = EventMetricType( category: "ui", name: "click", sendInPings: ["store1", "store2"], diff --git a/glean-core/ios/GleanTests/Metrics/LabeledMetricTests.swift b/glean-core/ios/GleanTests/Metrics/LabeledMetricTests.swift index fb0d330bf4..204d20d81d 100644 --- a/glean-core/ios/GleanTests/Metrics/LabeledMetricTests.swift +++ b/glean-core/ios/GleanTests/Metrics/LabeledMetricTests.swift @@ -195,7 +195,7 @@ class LabeledMetricTypeTests: XCTestCase { } func testLabeledEventsThrowAnException() { - let eventMetric = EventMetricType( + let eventMetric = EventMetricType( category: "telemetry", name: "labeled_event", sendInPings: ["metrics"], @@ -203,7 +203,7 @@ class LabeledMetricTypeTests: XCTestCase { disabled: false ) - XCTAssertThrowsError(try LabeledMetricType>( + XCTAssertThrowsError(try LabeledMetricType>( category: "telemetry", name: "labeled_event_metric", sendInPings: ["metrics"], diff --git a/samples/ios/app/glean-sample-app/ViewController.swift b/samples/ios/app/glean-sample-app/ViewController.swift index b962bde35d..fd46bbdd93 100644 --- a/samples/ios/app/glean-sample-app/ViewController.swift +++ b/samples/ios/app/glean-sample-app/ViewController.swift @@ -60,10 +60,13 @@ class ViewController: UIViewController { // order to illustrate adding extra information to the event, it is also adding to the // 'extras' field a dictionary of values. Note that the dictionary keys must be // declared in the metrics.yaml file under the 'extra_keys' section of an event metric. - BrowserEngagement.click.record(extra: [ - .key1: "extra_value_1", - .key2: "extra_value_2" - ]) + BrowserEngagement.click.record(BrowserEngagement.ClickExtra(key1: "extra_value_1", key2: "extra_value_2")) + + // An event without any extra keys + BrowserEngagement.eventNoKeys.record() + + // Testing the old API. It should still be possible, even if deprecated + BrowserEngagement.oldEventStyle.record(extra: [.key1: "extra_value1", .key2: "extra_value2"]) } @IBAction func sendButtonTapped(_: Any) { diff --git a/samples/ios/app/glean-sample-appUITests/EventPingTest.swift b/samples/ios/app/glean-sample-appUITests/EventPingTest.swift index 622bc5a9bc..fb14fc6d17 100644 --- a/samples/ios/app/glean-sample-appUITests/EventPingTest.swift +++ b/samples/ios/app/glean-sample-appUITests/EventPingTest.swift @@ -72,19 +72,23 @@ class EventPingTest: XCTestCase { XCTAssertEqual("inactive", reason, "Should have gotten a inactive events ping") let events = lastPingJson!["events"] as! [[String: Any]] - XCTAssertEqual(4, events.count, "Events ping should have all button-tap events") + // Per button tap we record 3 events. + XCTAssertEqual(4 * 3, events.count, "Events ping should have all button-tap events") let firstEvent = events[0] XCTAssertEqual(0, firstEvent["timestamp"] as! Int, "First event should be at timestamp 0") - for i in 1...3 { + for i in 1...11 { let earlier = events[i-1]["timestamp"] as! Int let this = events[i]["timestamp"] as! Int XCTAssert(earlier <= this, "Events should be ordered monotonically non-decreasing") } - let notLast = events[2]["timestamp"] as! Int - let last = events[3]["timestamp"] as! Int + // 3 events per tap, + // thus event 8 is the last event of the third tap, + // event 9 is the first event of the fourth tap. + let notLast = events[8]["timestamp"] as! Int + let last = events[9]["timestamp"] as! Int let diff = last - notLast // Sleeping and tapping the button has a delay of ~600ms, // so we account for a tiny bit more here. diff --git a/samples/ios/app/metrics.yaml b/samples/ios/app/metrics.yaml index 604d36fcb4..cc8670d65f 100644 --- a/samples/ios/app/metrics.yaml +++ b/samples/ios/app/metrics.yaml @@ -12,6 +12,27 @@ $schema: moz://mozilla.org/schemas/glean/metrics/2-0-0 browser.engagement: click: + type: event + description: | + Just testing events + bugs: + - https://bugzilla.mozilla.org/123456789 + data_reviews: + - N/A + notification_emails: + - CHANGE-ME@example.com + extra_keys: + key1: + type: string + description: "This is key one" + key2: + type: string + description: "This is key two" + expires: 2100-01-01 + no_lint: + - EXPIRATION_DATE_TOO_FAR + + old_event_style: type: event description: | Just testing events From 3510fe65cb1261db751454c81a2d4c43faa894f3 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Thu, 27 May 2021 12:02:42 +0200 Subject: [PATCH 13/13] Swift: Add explicit comments in tests and test old API alongside --- .../ios/GleanTests/Metrics/EventMetricTests.swift | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift b/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift index 4e3da2cefd..6c18df1531 100644 --- a/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift +++ b/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift @@ -6,6 +6,8 @@ import OHHTTPStubs import XCTest +// The event extra keys. +// This would be generated by the glean_parser usually. enum ClickKeys: Int32, ExtraKeys { case objectId = 0 case other = 1 @@ -15,6 +17,8 @@ enum ClickKeys: Int32, ExtraKeys { } } +// The event extra properties. +// This would be generated by the glean_parser usually. struct ClickExtras: EventExtras { var objectId: String? var other: String? @@ -88,7 +92,10 @@ class EventMetricTypeTests: XCTestCase { } func testEventSavesToStorage() { - let metric = EventMetricType( + // Note: We specify both `Keys` and `Extras` here to ease testing. + // In user code only _one_ will be specified and the other will be its `NoExtra` variant, + // thus only allowing either the old API or the new one. + let metric = EventMetricType( category: "ui", name: "click", sendInPings: ["store1"], @@ -108,7 +115,7 @@ class EventMetricTypeTests: XCTestCase { // Old API, this is available only because we manually implemented the enum. // Generated code will have only one of the APIs available. - metric.record(ClickExtras(objectId: "buttonB", other: "bar")) + metric.record(extra: [.objectId: "buttonB", .other: "bar"]) XCTAssert(metric.testHasValue()) let events = try! metric.testGetValue()