Bug 1682960 - Make the ping name optional in FOG JS and C++ test APIs r=janerik
authorChris H-C <chutten@mozilla.com>
Thu, 17 Dec 2020 12:42:26 +0000
changeset 561103 c2a095ef323d631ead11cf5bc906b9208d5be1c4
parent 561102 60e886fcc3db9f0ed4e173732653ef00ccead3c3
child 561104 b2cf325325e1ae08de0e387015ae2b0ac02a18a8
push id38040
push usercsabou@mozilla.com
push dateThu, 17 Dec 2020 21:49:27 +0000
treeherdermozilla-central@a08abfa374b6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjanerik
bugs1682960
milestone86.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1682960 - Make the ping name optional in FOG JS and C++ test APIs r=janerik Differential Revision: https://phabricator.services.mozilla.com/D99945
toolkit/components/glean/api/src/ffi/macros.rs
toolkit/components/glean/bindings/private/Boolean.h
toolkit/components/glean/bindings/private/Counter.h
toolkit/components/glean/bindings/private/Datetime.h
toolkit/components/glean/bindings/private/Event.h
toolkit/components/glean/bindings/private/MemoryDistribution.h
toolkit/components/glean/bindings/private/Ping.h
toolkit/components/glean/bindings/private/String.h
toolkit/components/glean/bindings/private/Timespan.h
toolkit/components/glean/bindings/private/Uuid.h
toolkit/components/glean/gtest/TestFog.cpp
toolkit/components/glean/xpcom/nsIGleanMetrics.idl
toolkit/components/glean/xpcshell/test_Glean.js
--- a/toolkit/components/glean/api/src/ffi/macros.rs
+++ b/toolkit/components/glean/api/src/ffi/macros.rs
@@ -26,30 +26,36 @@ macro_rules! metric_get {
 ///
 /// * `$map`     - The name of the hash map within `metrics::__glean_metric_maps`
 ///                as generated by glean_parser.
 /// * `$id`      - The ID of the metric to get.
 /// * `$storage` - the storage name to look into.
 macro_rules! test_has {
     ($map:ident, $id:ident, $storage:ident) => {{
         let metric = metric_get!($map, $id);
-        let storage = $storage.to_utf8();
-        let storage = Some(&storage[..]);
-        metric.test_get_value(storage).is_some()
+        let storage = if $storage.is_empty() {
+            None
+        } else {
+            Some($storage.to_utf8())
+        };
+        metric.test_get_value(storage.as_deref()).is_some()
     }};
 }
 
 /// Get the currently stored value for the metric identified by its ID.
 ///
 /// # Arguments
 ///
 /// * `$map`     - The name of the hash map within `metrics::__glean_metric_maps`
 ///                as generated by glean_parser.
 /// * `$id`      - The ID of the metric to get.
 /// * `$storage` - the storage name to look into.
 macro_rules! test_get {
     ($map:ident, $id:ident, $storage:ident) => {{
         let metric = metric_get!($map, $id);
-        let storage = $storage.to_utf8();
-        let storage = Some(&storage[..]);
-        metric.test_get_value(storage).unwrap()
+        let storage = if $storage.is_empty() {
+            None
+        } else {
+            Some($storage.to_utf8())
+        };
+        metric.test_get_value(storage.as_deref()).unwrap()
     }};
 }
--- a/toolkit/components/glean/bindings/private/Boolean.h
+++ b/toolkit/components/glean/bindings/private/Boolean.h
@@ -33,23 +33,26 @@ class BooleanMetric {
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
    *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
+   *
    * @return value of the stored metric.
    */
-  Maybe<bool> TestGetValue(const nsACString& aStorageName) const {
-    if (!fog_boolean_test_has_value(mId, &aStorageName)) {
+  Maybe<bool> TestGetValue(const nsACString& aPingName = nsCString()) const {
+    if (!fog_boolean_test_has_value(mId, &aPingName)) {
       return Nothing();
     }
-    return Some(fog_boolean_test_get_value(mId, &aStorageName));
+    return Some(fog_boolean_test_get_value(mId, &aPingName));
   }
 
  private:
   const uint32_t mId;
 };
 }  // namespace impl
 
 class GleanBoolean final : public nsIGleanBoolean {
--- a/toolkit/components/glean/bindings/private/Counter.h
+++ b/toolkit/components/glean/bindings/private/Counter.h
@@ -33,23 +33,26 @@ class CounterMetric {
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
    *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
+   *
    * @return value of the stored metric, or Nothing() if there is no value.
    */
-  Maybe<int32_t> TestGetValue(const nsACString& aStorageName) const {
-    if (!fog_counter_test_has_value(mId, &aStorageName)) {
+  Maybe<int32_t> TestGetValue(const nsACString& aPingName = nsCString()) const {
+    if (!fog_counter_test_has_value(mId, &aPingName)) {
       return Nothing();
     }
-    return Some(fog_counter_test_get_value(mId, &aStorageName));
+    return Some(fog_counter_test_get_value(mId, &aPingName));
   }
 
  private:
   const uint32_t mId;
 };
 }  // namespace impl
 
 class GleanCounter final : public nsIGleanCounter {
--- a/toolkit/components/glean/bindings/private/Datetime.h
+++ b/toolkit/components/glean/bindings/private/Datetime.h
@@ -48,24 +48,28 @@ class DatetimeMetric {
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
    *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
+   *
    * @return value of the stored metric, or Nothing() if there is no value.
    */
-  Maybe<nsCString> TestGetValue(const nsACString& aStorageName) const {
-    if (!fog_datetime_test_has_value(mId, &aStorageName)) {
+  Maybe<nsCString> TestGetValue(
+      const nsACString& aPingName = nsCString()) const {
+    if (!fog_datetime_test_has_value(mId, &aPingName)) {
       return Nothing();
     }
     nsCString ret;
-    fog_datetime_test_get_value(mId, &aStorageName, &ret);
+    fog_datetime_test_get_value(mId, &aPingName, &ret);
     return Some(ret);
   }
 
  private:
   const uint32_t mId;
 };
 }  // namespace impl
 
--- a/toolkit/components/glean/bindings/private/Event.h
+++ b/toolkit/components/glean/bindings/private/Event.h
@@ -68,21 +68,24 @@ class EventMetric {
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
    *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
+   *
    * @return value of the stored metric, or Nothing() if there is no value.
    */
   Maybe<nsTArray<RecordedEvent>> TestGetValue(
-      const nsACString& aStorageName) const {
-    if (!fog_event_test_has_value(mId, &aStorageName)) {
+      const nsACString& aPingName = nsCString()) const {
+    if (!fog_event_test_has_value(mId, &aPingName)) {
       return Nothing();
     }
 
     // TODO(bug 1678567): Implement this.
     nsTArray<RecordedEvent> empty;
     return Some(std::move(empty));
   }
 
--- a/toolkit/components/glean/bindings/private/MemoryDistribution.h
+++ b/toolkit/components/glean/bindings/private/MemoryDistribution.h
@@ -46,19 +46,23 @@ class MemoryDistributionMetric {
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
    *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
+   *
    * @return value of the stored metric, or Nothing() if there is no value.
    */
-  Maybe<DistributionData> TestGetValue(const nsACString& aPingName) const {
+  Maybe<DistributionData> TestGetValue(
+      const nsACString& aPingName = nsCString()) const {
     if (!fog_memory_distribution_test_has_value(mId, &aPingName)) {
       return Nothing();
     }
     nsTArray<uint64_t> buckets;
     nsTArray<uint64_t> counts;
     DistributionData ret;
     fog_memory_distribution_test_get_value(mId, &aPingName, &ret.sum, &buckets,
                                            &counts);
--- a/toolkit/components/glean/bindings/private/Ping.h
+++ b/toolkit/components/glean/bindings/private/Ping.h
@@ -35,17 +35,17 @@ class Ping {
    * Pings always contain the `ping_info` and `client_info` sections.
    * See [ping
    * sections](https://mozilla.github.io/glean/book/user/pings/index.html#ping-sections)
    * for details.
    *
    * @param aReason - Optional. The reason the ping is being submitted.
    *                  Must match one of the configured `reason_codes`.
    */
-  void Submit(const nsACString& aReason) const {
+  void Submit(const nsACString& aReason = nsCString()) const {
     fog_submit_ping_by_id(mId, &aReason);
   }
 
  private:
   const uint32_t mId;
 };
 
 }  // namespace impl
--- a/toolkit/components/glean/bindings/private/String.h
+++ b/toolkit/components/glean/bindings/private/String.h
@@ -37,24 +37,28 @@ class StringMetric {
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
    *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
+   *
    * @return value of the stored metric, or Nothing() if there is no value.
    */
-  Maybe<nsCString> TestGetValue(const nsACString& aStorageName) const {
-    if (!fog_string_test_has_value(mId, &aStorageName)) {
+  Maybe<nsCString> TestGetValue(
+      const nsACString& aPingName = nsCString()) const {
+    if (!fog_string_test_has_value(mId, &aPingName)) {
       return Nothing();
     }
     nsCString ret;
-    fog_string_test_get_value(mId, &aStorageName, &ret);
+    fog_string_test_get_value(mId, &aPingName, &ret);
     return Some(ret);
   }
 
  private:
   const uint32_t mId;
 };
 }  // namespace impl
 
--- a/toolkit/components/glean/bindings/private/Timespan.h
+++ b/toolkit/components/glean/bindings/private/Timespan.h
@@ -45,23 +45,26 @@ class TimespanMetric {
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
    *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
+   *
    * @return value of the stored metric, or Nothing() if there is no value.
    */
-  Maybe<int64_t> TestGetValue(const nsACString& aStorageName) const {
-    if (!fog_timespan_test_has_value(mId, &aStorageName)) {
+  Maybe<int64_t> TestGetValue(const nsACString& aPingName = nsCString()) const {
+    if (!fog_timespan_test_has_value(mId, &aPingName)) {
       return Nothing();
     }
-    return Some(fog_timespan_test_get_value(mId, &aStorageName));
+    return Some(fog_timespan_test_get_value(mId, &aPingName));
   }
 
  private:
   const uint32_t mId;
 };
 }  // namespace impl
 
 class GleanTimespan final : public nsIGleanTimespan {
--- a/toolkit/components/glean/bindings/private/Uuid.h
+++ b/toolkit/components/glean/bindings/private/Uuid.h
@@ -38,26 +38,29 @@ class UuidMetric {
    * Gets the currently stored value as a hyphenated string.
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
-   * Panics if there is no value to get.
+   *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
    *
    * @return value of the stored metric, or Nothing() if there is no value.
    */
-  Maybe<nsCString> TestGetValue(const nsACString& aStorageName) const {
-    if (!fog_uuid_test_has_value(mId, &aStorageName)) {
+  Maybe<nsCString> TestGetValue(
+      const nsACString& aPingName = nsCString()) const {
+    if (!fog_uuid_test_has_value(mId, &aPingName)) {
       return Nothing();
     }
     nsCString ret;
-    fog_uuid_test_get_value(mId, &aStorageName, &ret);
+    fog_uuid_test_get_value(mId, &aPingName, &ret);
     return Some(ret);
   }
 
  private:
   const uint32_t mId;
 };
 }  // namespace impl
 
--- a/toolkit/components/glean/gtest/TestFog.cpp
+++ b/toolkit/components/glean/gtest/TestFog.cpp
@@ -58,16 +58,18 @@ TEST(FOG, BuiltinPingsRegistered)
 
 TEST(FOG, TestCppCounterWorks)
 {
   mozilla::glean::test_only::bad_code.Add(42);
 
   ASSERT_EQ(
       42,
       mozilla::glean::test_only::bad_code.TestGetValue("test-ping"_ns).value());
+  // And test that the ping name's optional, while you're at it:
+  ASSERT_EQ(42, test_only::bad_code.TestGetValue().value());
 }
 
 TEST(FOG, TestCppStringWorks)
 {
   auto kValue = "cheez!"_ns;
   mozilla::glean::test_only::cheesy_string.Set(kValue);
 
   ASSERT_STREQ(kValue.get(), mozilla::glean::test_only::cheesy_string
--- a/toolkit/components/glean/xpcom/nsIGleanMetrics.idl
+++ b/toolkit/components/glean/xpcom/nsIGleanMetrics.idl
@@ -22,19 +22,22 @@ interface nsIGleanBoolean : nsISupports
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
    *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
+   *
    * @return value of the stored metric, or undefined if there is no value.
    */
-  jsval testGetValue(in AUTF8String aStorageName);
+  jsval testGetValue([optional] in AUTF8String aPingName);
 };
 
 [scriptable, uuid(aa15fd20-1e8a-11eb-9bec-0800200c9a66)]
 interface nsIGleanDatetime : nsISupports
 {
   /**
    * Set the datetime to the provided value, or the local now.
    *
@@ -50,20 +53,23 @@ interface nsIGleanDatetime : nsISupports
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
    *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
+   *
    * @return value of the stored metric, or undefined if there is no value.
    */
   [implicit_jscontext]
-  jsval testGetValue(in AUTF8String aStorageName);
+  jsval testGetValue([optional] in AUTF8String aPingName);
 };
 
 [scriptable, uuid(05b89d2a-d57c-11ea-82da-3f63399a6f5a)]
 interface nsIGleanCounter : nsISupports
 {
   /*
   * Increases the counter by `amount`.
   *
@@ -78,19 +84,22 @@ interface nsIGleanCounter : nsISupports
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
    *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
+   *
    * @return value of the stored metric, or undefined if there is no value.
    */
-  jsval testGetValue(in AUTF8String aStorageName);
+  jsval testGetValue([optional] in AUTF8String aPingName);
 };
 
 [scriptable, uuid(eea5ed46-16ba-46cd-bb1f-504581987fe1)]
 interface nsIGleanMemoryDistribution : nsISupports
 {
   /*
    * Accumulates the provided sample in the metric.
    *
@@ -109,20 +118,23 @@ interface nsIGleanMemoryDistribution : n
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
    *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
+   *
    * @return value of the stored metric, or Nothing() if there is no value.
    */
   [implicit_jscontext]
-	jsval testGetValue(in ACString aStorageName);
+	jsval testGetValue([optional] in ACString aPingName);
 };
 
 [scriptable, uuid(5223a48b-687d-47ff-a629-fd4a72d1ecfa)]
 interface nsIGleanPing : nsISupports
 {
   /**
    * Collect and submit the ping for eventual upload.
    *
@@ -162,20 +174,23 @@ interface nsIGleanString : nsISupports
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
    *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
+   *
    * @return value of the stored metric, or undefined if there is no value.
    */
   [implicit_jscontext]
-  jsval testGetValue(in AUTF8String aStorageName);
+  jsval testGetValue([optional] in AUTF8String aPingName);
 };
 
 [scriptable, uuid(2586530c-030f-11eb-93cb-cbf30d25225a)]
 interface nsIGleanTimespan : nsISupports
 {
   /**
    * Start tracking time for the provided metric.
    *
@@ -202,19 +217,22 @@ interface nsIGleanTimespan : nsISupports
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
    *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
+   *
    * @return value of the stored metric, or undefined if there is no value.
    */
-  jsval testGetValue(in AUTF8String aStorageName);
+  jsval testGetValue([optional] in AUTF8String aPingName);
 };
 
 [scriptable, uuid(395700e7-06f6-46be-adcc-ea58977fda6d)]
 interface nsIGleanUuid : nsISupports
 {
   /**
    * Set to the specified value.
    *
@@ -234,20 +252,23 @@ interface nsIGleanUuid : nsISupports
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
    *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
+   *
    * @return value of the stored metric, or undefined if there is no value.
    */
   [implicit_jscontext]
-  jsval testGetValue(in AUTF8String aStorageName);
+  jsval testGetValue([optional] in AUTF8String aPingName);
 };
 
 [scriptable, uuid(1b01424a-1f55-11eb-92a5-0754f6c3f240)]
 interface nsIGleanEvent : nsISupports
 {
   /*
    * Record an event.
    *
@@ -263,13 +284,16 @@ interface nsIGleanEvent : nsISupports
    *
    * This function will attempt to await the last parent-process task (if any)
    * writing to the the metric's storage engine before returning a value.
    * This function will not wait for data from child processes.
    *
    * This doesn't clear the stored value.
    * Parent process only. Panics in child processes.
    *
+   * @param aPingName The (optional) name of the ping to retrieve the metric
+   *        for. Defaults to the first value in `send_in_pings`.
+   *
    * @return value of the stored metric.
    */
   [implicit_jscontext]
-  jsval testGetValue(in AUTF8String aStorageName);
+  jsval testGetValue([optional] in AUTF8String aPingName);
 };
--- a/toolkit/components/glean/xpcshell/test_Glean.js
+++ b/toolkit/components/glean/xpcshell/test_Glean.js
@@ -130,16 +130,18 @@ add_task({ skip_if: () => true }, functi
 
   const received = Glean.test_only.what_a_date.testGetValue("test-ping");
   Assert.ok(received.startsWith("2020-06-11T12:00:00"));
 });
 
 add_task(function test_fog_boolean_works() {
   Glean.test_only.can_we_flag_it.set(false);
   Assert.equal(false, Glean.test_only.can_we_flag_it.testGetValue("test-ping"));
+  // While you're here, might as well test that the ping name's optional.
+  Assert.equal(false, Glean.test_only.can_we_flag_it.testGetValue());
 });
 
 add_task(async function test_fog_event_works() {
   Glean.test_only_ipc.no_extra_event.record();
   // FIXME(bug 1678567): Check that the value was recorded when we can.
   // Assert.ok(Glean.test_only_ipc.no_extra_event.testGetValue("store1"));
 
   let extra = { extra1: "can set extras", extra2: "passing more data" };