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 70e6d0d4a2..fed6b68777 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 @@ -62,6 +62,7 @@ samples/ios/app/.venv/ htmlcov/ *.log +glean-core/python/glean/_glean_ffi.py __pycache__ *.py[cod] .Python 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) 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/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 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/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/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..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 @@ -54,6 +54,31 @@ 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> +} + +/** + * 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()) + } +} + /** * This implements the developer facing API for recording events. * @@ -63,7 +88,7 @@ enum class NoExtraKeys( * 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 @@ -99,12 +124,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 - fun record(extra: Map? = null) { + @Deprecated("Specify types for your event extras. See the reference for details.") + fun record(extra: Map) { if (disabled) { return } @@ -120,14 +148,52 @@ 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. + 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, + timestamp, + keys, + values, + len + ) + } + } + + /** + * 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. + */ + @JvmOverloads + fun record(extra: ExtraObject? = null) { + 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 { 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 extras = extra.toFfiExtra() + keys = extras.first + values = StringArray(extras.second.toTypedArray(), "utf-8") + len = keys.size } LibGleanFFI.INSTANCE.glean_event_record( 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..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 @@ -175,12 +176,13 @@ 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() - 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 891ccd252f..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 @@ -125,7 +126,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( @@ -135,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 95f3efc558..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 @@ -44,6 +44,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] }), values) + } +} + enum class testNameKeys { testName } @@ -52,6 +71,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 { @@ -60,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, @@ -72,12 +92,12 @@ 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) - 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() @@ -102,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, @@ -139,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, @@ -157,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, @@ -170,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, @@ -207,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, @@ -247,7 +267,7 @@ class EventMetricTypeTest { clearStores = true ) - val event = EventMetricType( + val event = EventMetricType( disabled = false, category = "telemetry", name = "test_event", @@ -307,7 +327,7 @@ class EventMetricTypeTest { clearStores = true ) - val event = EventMetricType( + val event = EventMetricType( disabled = false, category = "telemetry", name = "test_event", @@ -384,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, @@ -415,7 +435,7 @@ class EventMetricTypeTest { ) val pingName = "another-ping" - val event = EventMetricType( + val event = EventMetricType( disabled = false, category = "telemetry", name = "test_event", @@ -490,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/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/Glean/Metrics/EventMetric.swift b/glean-core/ios/Glean/Metrics/EventMetric.swift index f8c93d18bb..4e373a890b 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,33 @@ 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]) { + 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, @@ -50,7 +90,7 @@ public enum NoExtraKeys: ExtraKeys { /// /// 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] @@ -96,12 +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) { - guard !self.disabled else { return } + @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: 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 @@ -110,20 +159,10 @@ 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 = 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 1ca51c85b0..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,29 @@ 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? + + 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 @@ -67,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"], @@ -78,15 +106,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,14 +128,19 @@ 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") } func testEventRecordedWithEmptyCategory() { - let metric = EventMetricType( + let metric = EventMetricType( category: "", name: "click", sendInPings: ["store1"], @@ -130,7 +168,7 @@ class EventMetricTypeTests: XCTestCase { } func testEventNotRecordedWhenDisabled() { - let metric = EventMetricType( + let metric = EventMetricType( category: "ui", name: "click", sendInPings: ["store1"], @@ -146,7 +184,7 @@ class EventMetricTypeTests: XCTestCase { } func testCounterGetValueThrowsExceptionIfNothingIsStored() { - let metric = EventMetricType( + let metric = EventMetricType( category: "ui", name: "click", sendInPings: ["store1"], @@ -160,7 +198,7 @@ class EventMetricTypeTests: XCTestCase { } func testEventSavesToSecondaryPings() { - let metric = EventMetricType( + let metric = EventMetricType( category: "ui", name: "click", sendInPings: ["store1", "store2"], @@ -191,7 +229,7 @@ class EventMetricTypeTests: XCTestCase { } func testEventNotRecordWhenUploadDisabled() { - let metric = EventMetricType( + let metric = EventMetricType( category: "ui", name: "click", sendInPings: ["store1", "store2"], @@ -219,7 +257,7 @@ class EventMetricTypeTests: XCTestCase { setupHttpResponseStub() expectation = expectation(description: "Completed upload") - let event = EventMetricType( + let event = EventMetricType( category: "telemetry", name: "test_event", sendInPings: ["events"], @@ -251,7 +289,7 @@ class EventMetricTypeTests: XCTestCase { setupHttpResponseStub() expectation = expectation(description: "Completed upload") - let event = EventMetricType( + let event = EventMetricType( category: "telemetry", name: "test_event", sendInPings: ["events"], @@ -296,7 +334,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/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/glean/_loader.py b/glean-core/python/glean/_loader.py index 1b9d4efcb5..3b27380c5e 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,60 @@ 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(): + 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__}" + ) + 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) + + def to_ffi_extra(self): + keys = [] + values = [] + + for idx, (name, typ) in enumerate(argnames): + attr = getattr(self, name, None) + if attr is not None: + keys.append(idx) + if typ == "boolean" and isinstance(attr, bool): + # Special-case needed for booleans to turn them lowercase (true/false) + values.append(str(attr).lower()) + 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) + + 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 +169,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_with_types + 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/__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..9257608e6b 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,63 @@ 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/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/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 f51f5ca3f6..c2637fbf10 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,105 @@ def test_event_enum_is_generated_correctly(): "key1": "value1", "key2": "value2", } == metrics.environment.event_example.test_get_value()[0].extra + + +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.PreferenceToggledExtra(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: + 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 + + +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() 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.** /// 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/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 1fc45a4524..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,12 +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( - mapOf( - BrowserEngagement.clickKeys.key1 to "extra_value_1", - BrowserEngagement.clickKeys.key2 to "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 + @Suppress("DEPRECATION") + BrowserEngagement.oldEventStyle.record(mapOf( + BrowserEngagement.oldEventStyleKeys.key1 to "extra_value1", + BrowserEngagement.oldEventStyleKeys.key2 to "extra_value2" + )) } uploadSwitch.setOnCheckedChangeListener { _, isChecked -> 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" 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