Code Review Feedback project-chip#3:

1. Added ScopedMetricEvent to use RAII to track begin and end within a
   scope

src/app/clusters/wifi-network-diagnostics-server/wifi-network-diagnostics-server.cpp

@@ -29,7 +29,6 @@
 #include <platform/DiagnosticDataProvider.h>
 #include <tracing/macros.h>
 #include <tracing/metric_event.h>
-#include <tracing/metric_keys.h>
 
 using namespace chip;
 using namespace chip::app;

src/controller/CHIPDeviceController.cpp

@@ -27,7 +27,6 @@
 
 #ifndef __STDC_LIMIT_MACROS
 #define __STDC_LIMIT_MACROS
-#include <tracing/metric_keys.h>
 #endif
 #ifndef __STDC_FORMAT_MACROS
 #define __STDC_FORMAT_MACROS
@@ -631,9 +630,8 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, co
 CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, RendezvousParameters & params)
 {
     MATTER_TRACE_SCOPE("EstablishPASEConnection", "DeviceCommissioner");
-    MATTER_LOG_METRIC_BEGIN(chip::Tracing::kMetricPASESession);
 
-    CHIP_ERROR err                     = CHIP_NO_ERROR;
+    MATTER_LOG_METRIC_SCOPE_WITH_ERROR(chip::Tracing::kMetricPASESession, err, CHIP_NO_ERROR);
     CommissioneeDeviceProxy * device   = nullptr;
     CommissioneeDeviceProxy * current  = nullptr;
     Transport::PeerAddress peerAddress = Transport::PeerAddress::UDP(Inet::IPAddress::Any);
@@ -708,6 +706,8 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re
 #if CONFIG_NETWORK_LAYER_BLE
     if (params.GetPeerAddress().GetTransportType() == Transport::Type::kBle)
     {
+        MATTER_LOG_METRIC_SCOPE(chip::Tracing::kMetricPASESessionBLE);
+
         if (params.HasConnectionObject())
         {
             SuccessOrExitWithMetric(chip::Tracing::kMetricPASESessionBLE, err = mSystemState->BleLayer()->NewBleConnectionByObject(params.GetConnectionObject()));
@@ -763,7 +763,6 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re
         }
     }
 
-    MATTER_LOG_METRIC_END(chip::Tracing::kMetricPASESession);
     return err;
 }
 

src/tracing/metric_event.h

@@ -109,13 +109,13 @@ class MetricEvent
 
     MetricEvent(Type type, MetricKey key, uint16_t value) : MetricEvent(type, key, uint32_t(value)) {}
 
-    Type type() const 
-    { 
+    Type type() const
+    {
         return mType;
     }
 
-    MetricKey key() const 
-    { 
+    MetricKey key() const
+    {
         return mKey;
     }
 
@@ -162,6 +162,66 @@ inline bool LogMetricIfError(MetricKey metricKey, const ::chip::ChipError & err)
     return success;
 }
 
+/**
+ * This utility class helps generate a Begin and End metric event within the scope of a block using RAII.
+ * This class is also meant to be used in expressions where ChipError object are typically used to capture 
+ * error values.
+ */
+class ScopedMetricEvent
+{
+public:
+    ScopedMetricEvent(const ScopedMetricEvent&) = default;
+    ScopedMetricEvent(ScopedMetricEvent &&) = default;
+    ScopedMetricEvent &operator=(const ScopedMetricEvent&) = default;
+    ScopedMetricEvent &operator=(ScopedMetricEvent &&) = default;
+
+    ScopedMetricEvent(MetricKey key, const ChipError & error = CHIP_NO_ERROR) : mKey(key), mError(error) 
+    {
+        MATTER_LOG_METRIC_BEGIN(mKey);
+    }
+
+    ~ScopedMetricEvent()
+    {
+        MATTER_LOG_METRIC_END(mKey, mError);
+    }
+
+    operator ChipError() const
+    {
+        return mError;
+    }
+
+    ScopedMetricEvent& operator = (const ChipError & err)
+    {
+        mError = err;
+        return *this;
+    }
+
+    friend bool operator == (const ScopedMetricEvent & event, const ChipError & err)
+    {
+        return event.mError == err;
+    }
+
+    friend bool operator != (const ScopedMetricEvent & event, const ChipError & err)
+    {
+        return event.mError != err;
+    }
+
+    friend bool operator == (const ChipError & err, const ScopedMetricEvent & event)
+    {
+        return event.mError == err;
+    }
+
+    friend bool operator != (const ChipError & err, const ScopedMetricEvent & event)
+    {
+        return event.mError != err;
+    }
+
+private:
+    MetricKey mKey;
+    ChipError mError;
+};
+
+
 } // namespace utils
 
 } // namespace Tracing

src/tracing/metric_macros.h

@@ -21,6 +21,9 @@
 #include <lib/core/CHIPError.h>
 #include <lib/support/CodeUtils.h>
 
+#define __LOG_METRIC_CONCAT_IMPL(a, b) a##b
+#define __LOG_METRIC_MACRO_CONCAT(a, b) __LOG_METRIC_CONCAT_IMPL(a, b)
+
 #if MATTER_TRACING_ENABLED
 
 
@@ -153,16 +156,15 @@
  *      The above example generates an instant metric event with key kMetricPASESession.
  *      The metric also holds the 32 bit value corresponding to the ChipError object 'err'.
  *
- *  @param[in]  key The key representing the metric name/event. The name is one the kMetricXYZ string literal
- *                  constants as defined in metric_event.h.
+ *  @param[in]  key The key representing the metric name/event.
  *
  *  @param[in]  value An optional value for the metric. This value corresponds to one of the values supported
  *                    in MetricEvent::Value
  */
 #define MATTER_LOG_METRIC(key, ...) __MATTER_LOG_METRIC(chip::Tracing::MetricEvent::Type::kInstantEvent, key, ##__VA_ARGS__)
 
 /**
- * @def MATTER_TRACE_BEGIN
+ * @def MATTER_LOG_METRIC_BEGIN
  *
  * @brief
  * Generate a metric with the Begin Type
@@ -173,16 +175,15 @@
  *  @endcode
  *      The above example generates a Begin metric event with key kMetricPASESession.
  *
- *  @param[in]  key The key representing the metric name/event. The name is one the kMetricXYZ string literal
- *                  constants as defined in metric_event.h.
+ *  @param[in]  key The key representing the metric name/event.
  *
  *  @param[in]  value An optional value for the metric. This value corresponds to one of the values supported
  *                    in MetricEvent::Value
  */
 #define MATTER_LOG_METRIC_BEGIN(key, ...) __MATTER_LOG_METRIC(chip::Tracing::MetricEvent::Type::kBeginEvent, key, ##__VA_ARGS__)
 
 /**
- * @def MATTER_TRACE_END
+ * @def MATTER_LOG_METRIC_END
  *
  * @brief
  * Generate a metric with the End Type
@@ -193,14 +194,50 @@
  *  @endcode
  *      The above example generates an End metric event with key kMetricPASESession.
  *
- *  @param[in]  key The key representing the metric name/event. The name is one the kMetricXYZ string literal
- *                  constants as defined in metric_event.h.
+ *  @param[in]  key The key representing the metric name/event.
  *
  *  @param[in]  value An optional value for the metric. This value corresponds to one of the values supported
  *                    in MetricEvent::Value
  */
 #define MATTER_LOG_METRIC_END(key, ...) __MATTER_LOG_METRIC(chip::Tracing::MetricEvent::Type::kEndEvent, key, ##__VA_ARGS__)
 
+
+/**
+ * @def MATTER_LOG_METRIC_SCOPE
+ *
+ * @brief
+ * Generate a scoped metric tracking Begin and End within a given scope.
+ *
+ *  Example usage:
+ *  @code
+ *      MATTER_LOG_METRIC_SCOPE(chip::Tracing::kMetricPASESession);
+ *  @endcode
+ *      The above example generates an Begin and the End metric using RAII.
+ *
+ *  @param[in]  key The key representing the metric name/event.
+ */
+#define MATTER_LOG_METRIC_SCOPE(key) ::chip::Tracing::utils::ScopedMetricEvent __LOG_METRIC_MACRO_CONCAT(_metric_scope, __COUNTER__)(key)
+
+/**
+ * @def MATTER_LOG_METRIC_SCOPE_WITH_ERROR
+ *
+ * @brief
+ * Generate a scoped metric tracking Begin and End within a given scope. In addition, it creates the object using
+ * the name specified. This object is meant to be used as ChipError object. The End metric will also hold this error
+ * value.
+ *
+ *  Example usage:
+ *  @code
+ *      MATTER_LOG_METRIC_SCOPE_WITH_ERROR(chip::Tracing::kMetricPASESession, err, CHIP_NO_ERROR);
+ *  @endcode
+ *      The above example generates an Begin and the End metric using RAII.
+ *
+ *  @param[in]  key The key representing the metric name/event.
+ *  @param[in]  errorObj The name of the object for the ScopedMetricEvent.
+ *  @param[in]  errorValue The initial error code value.
+ */
+#define MATTER_LOG_METRIC_SCOPE_WITH_ERROR(key, errorObj, errorValue) chip::Tracing::utils::ScopedMetricEvent errorObj(key, errorValue)
+
 #else // Tracing is disabled
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////////////////
@@ -224,5 +261,24 @@
 #define MATTER_LOG_METRIC(...) __MATTER_LOG_METRIC_DISABLE(__VA_ARGS__)
 #define MATTER_LOG_METRIC_BEGIN(key, ...) __MATTER_LOG_METRIC_DISABLE(__VA_ARGS__)
 #define MATTER_LOG_METRIC_END(key, ...) __MATTER_LOG_METRIC_DISABLE(__VA_ARGS__)
+#define MATTER_LOG_METRIC_SCOPE(key) __MATTER_LOG_METRIC_DISABLE(__VA_ARGS__)
+
+/**
+ * @def MATTER_LOG_METRIC_SCOPE_WITH_ERROR
+ *
+ * @brief
+ * When tracing is disabled, this defaults to creating an ChipError object with a specified value.
+ *
+ *  Example usage:
+ *  @code
+ *      MATTER_LOG_METRIC_SCOPE_WITH_ERROR(chip::Tracing::kMetricPASESession, err, CHIP_NO_ERROR);
+ *  @endcode
+ *      The above example generates a ChipError with the specified value.
+ *
+ *  @param[in]  key The key representing the metric name/event. This parameter is ignored since tracing is disabled.
+ *  @param[in]  errorObj The name of the ChipError object.
+ *  @param[in]  errorValue The initial error code value.
+ */
+#define MATTER_LOG_METRIC_SCOPE_WITH_ERROR(key, errorObj, errorValue) chip::ChipError errorObj = errorValue
 
 #endif // MATTER_TRACING_ENABLED

src/tracing/tracing_args.gni

@@ -23,11 +23,7 @@ declare_args() {
   #
   # Additionally, if tracing is enabled, the main() function has to add
   # backends explicitly
-  matter_enable_tracing_support = current_os == "android"
-
-  if (chip_device_platform == "darwin") {
-    matter_enable_tracing_support = true
-  }
+  matter_enable_tracing_support = (current_os == "android") || (chip_device_platform == "darwin")
 
   # Defines the trace backend. Current matter tracing splits the logic
   # into two parts: