Code Review Feedback project-chip#2:

1. Added SuccessOrExitWithMetric and VerifyOrExitWithMetric
2. Cleaned up support .gn to remove dependedency on metrics

src/controller/CHIPDeviceController.cpp

@@ -710,16 +710,15 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re
     {
         if (params.HasConnectionObject())
         {
-            SuccessOrExit(err = mSystemState->BleLayer()->NewBleConnectionByObject(params.GetConnectionObject()), chip::Tracing::kMetricPASESessionBLE);
+            SuccessOrExitWithMetric(chip::Tracing::kMetricPASESessionBLE, err = mSystemState->BleLayer()->NewBleConnectionByObject(params.GetConnectionObject()));
         }
         else if (params.HasDiscoveredObject())
         {
             // The RendezvousParameters argument needs to be recovered if the search succeed, so save them
             // for later.
             mRendezvousParametersForDeviceDiscoveredOverBle = params;
-            SuccessOrExit(err = mSystemState->BleLayer()->NewBleConnectionByObject(
-                              params.GetDiscoveredObject(), this, OnDiscoveredDeviceOverBleSuccess, OnDiscoveredDeviceOverBleError),
-                          chip::Tracing::kMetricPASESessionBLE);
+            SuccessOrExitWithMetric(chip::Tracing::kMetricPASESessionBLE, err = mSystemState->BleLayer()->NewBleConnectionByObject(
+                              params.GetDiscoveredObject(), this, OnDiscoveredDeviceOverBleSuccess, OnDiscoveredDeviceOverBleError));
             ExitNow(CHIP_NO_ERROR);
         }
         else if (params.HasDiscriminator())
@@ -730,9 +729,8 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re
 
             SetupDiscriminator discriminator;
             discriminator.SetLongValue(params.GetDiscriminator());
-            SuccessOrExit(err = mSystemState->BleLayer()->NewBleConnectionByDiscriminator(
-                              discriminator, this, OnDiscoveredDeviceOverBleSuccess, OnDiscoveredDeviceOverBleError),
-                          chip::Tracing::kMetricPASESessionBLE);
+            SuccessOrExitWithMetric(chip::Tracing::kMetricPASESessionBLE, err = mSystemState->BleLayer()->NewBleConnectionByDiscriminator(
+                              discriminator, this, OnDiscoveredDeviceOverBleSuccess, OnDiscoveredDeviceOverBleError));
             ExitNow(CHIP_NO_ERROR);
         }
         else
@@ -742,7 +740,7 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re
     }
 #endif
     session = mSystemState->SessionMgr()->CreateUnauthenticatedSession(params.GetPeerAddress(), params.GetMRPConfig());
-    VerifyOrExit(session.HasValue(), err = CHIP_ERROR_NO_MEMORY, chip::Tracing::kMetricPASESessionPair);
+    VerifyOrExitWithMetric(chip::Tracing::kMetricPASESessionPair, session.HasValue(), err = CHIP_ERROR_NO_MEMORY);
 
     // Allocate the exchange immediately before calling PASESession::Pair.
     //
@@ -751,10 +749,10 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re
     // exchange context right before calling Pair ensures that if allocation
     // succeeds, PASESession has taken ownership.
     exchangeCtxt = mSystemState->ExchangeMgr()->NewContext(session.Value(), &device->GetPairing());
-    VerifyOrExit(exchangeCtxt != nullptr, err = CHIP_ERROR_INTERNAL, chip::Tracing::kMetricPASESessionPair);
+    VerifyOrExitWithMetric(chip::Tracing::kMetricPASESessionPair, exchangeCtxt != nullptr, err = CHIP_ERROR_INTERNAL);
 
     err = device->GetPairing().Pair(*mSystemState->SessionMgr(), params.GetSetupPINCode(), GetLocalMRPConfig(), exchangeCtxt, this);
-    SuccessOrExit(err, chip::Tracing::kMetricPASESessionPair);
+    SuccessOrExitWithMetric(chip::Tracing::kMetricPASESessionPair, err);
 
 exit:
     if (err != CHIP_NO_ERROR)

src/lib/support/BUILD.gn

@@ -141,7 +141,6 @@ source_set("verifymacros") {
     ":verifymacros_no_logging",
     "${chip_root}/src/lib/core:chip_config_header",
     "${chip_root}/src/lib/core:error",
-    "${chip_root}/src/tracing:metrics",
     "${nlassert_root}:nlassert",
   ]
 }

src/lib/support/CodeUtils.h

@@ -31,7 +31,6 @@
 #include <lib/core/ErrorStr.h>
 #include <lib/support/VerificationMacrosNoLogging.h>
 #include <lib/support/logging/TextOnlyLogging.h>
-#include <tracing/metric_macros.h>
 
 /**
  * Base-level abnormal termination.
@@ -394,7 +393,7 @@ constexpr inline const _T & max(const _T & a, const _T & b)
  *                          result of the expression aStatus.
  *
  */
-#define SuccessOrExit(aStatus, ...) nlEXPECT(LOG_METRIC_FOR_SUCCESS_OR_EXIT((aStatus), ##__VA_ARGS__), exit)
+#define SuccessOrExit(aStatus) nlEXPECT(::chip::ChipError::IsSuccess((aStatus)), exit)
 
 /**
  *  @def VerifyOrExit(aCondition, anAction)
@@ -429,8 +428,7 @@ constexpr inline const _T & max(const _T & a, const _T & b)
  *                          result of the expression anAction.
  *
  */
-#define VerifyOrExit(aCondition, anAction, ...)                                                                                    \
-    nlEXPECT_ACTION(aCondition, exit, LOG_METRIC_FOR_VERIFY_OR_EXIT_ACTION(anAction, ##__VA_ARGS__))
+#define VerifyOrExit(aCondition, anAction) nlEXPECT_ACTION(aCondition, exit, anAction)
 
 /**
  *  @def ExitNow(...)

src/tracing/BUILD.gn

@@ -38,24 +38,14 @@ static_library("tracing") {
     "log_declares.h",
     "registry.cpp",
     "registry.h",
-  ]
-
-  public_deps = [
-    ":metrics",
-    ":tracing_buildconfig",
-    "${chip_root}/src/lib/support",
-  ]
-}
-
-source_set("metrics") {
-  sources = [
     "metric_event.h",
     "metric_keys.h",
     "metric_macros.h",
   ]
 
   public_deps = [
     ":tracing_buildconfig",
+    "${chip_root}/src/lib/support",
     "${chip_root}/src/lib/core:error",
   ]
 }

src/tracing/metric_event.h

@@ -19,6 +19,7 @@
 #include <lib/core/CHIPError.h>
 #include <tracing/metric_macros.h>
 #include <tracing/metric_keys.h>
+#include <tracing/registry.h>
 
 namespace chip {
 namespace Tracing {
@@ -144,18 +145,12 @@ class MetricEvent
     Value mValue;
 };
 
-namespace Internal {
-
-void LogMetricEvent(const ::chip::Tracing::MetricEvent & event);
-
-} // namespace Internal
-
 namespace utils {
 
 /**
  * Utility to emit an instant metric if the error is not a success.
  */
-inline bool LogMetricIfError(const ::chip::ChipError & err, MetricKey metricKey)
+inline bool LogMetricIfError(MetricKey metricKey, const ::chip::ChipError & err)
 {
     bool success = ::chip::ChipError::IsSuccess(err);
     if (!success)

src/tracing/metric_macros.h

@@ -17,46 +17,88 @@
  */
 #pragma once
 
-#include <lib/core/CHIPError.h>
 #include <matter/tracing/build_config.h>
+#include <lib/core/CHIPError.h>
+#include <lib/support/CodeUtils.h>
 
 #if MATTER_TRACING_ENABLED
 
-// Utility to always return the 3rd argument from macro parameters
-#define __GET_3RD_ARGUMENT(_a1, _a2, _a3, ...) _a3
-
-// Utility macro to select the VerifyOrExit macro with the correct signature based on the invoked macro
-#define __SELECT_MACRO_FOR_VERIFY_ACTION(...) __GET_3RD_ARGUMENT(__VA_ARGS__, __VERIFY_ACTION_2ARGS, __VERIFY_ACTION_1ARGS, )
-
-// Wrapper to capture all arguments and invoke the real wrapper for VerifyOrExit's third argument
-#define __LOG_METRIC_FOR_VERIFY_ACTION(...) __SELECT_MACRO_FOR_VERIFY_ACTION(__VA_ARGS__)(__VA_ARGS__)
-
-// Utility macro that takes the action wraps it to emit the metric in a Verify macro, when a metric key is specified
-#define __VERIFY_ACTION_2ARGS(anAction, metricKey) MATTER_LOG_METRIC(metricKey, anAction)
-
-// Macro that takes the action in a Verify macro when a metric key is not specified
-#define __VERIFY_ACTION_1ARGS(anAction) anAction
 
-// Utility macro used by VerifyOrExit macro to handle an optional third metric key argument and emit an event, if specified
-#define LOG_METRIC_FOR_VERIFY_OR_EXIT_ACTION(anAction, ...) __LOG_METRIC_FOR_VERIFY_ACTION(anAction, ##__VA_ARGS__)
-
-// Utility macro to select the SuccessOrExit macro with the correct signature based on the invoked macro
-#define __SELECT_MACRO_FOR_SUCCESS_OR_EXIT(...) __GET_3RD_ARGUMENT(__VA_ARGS__, __SUCCESS_OR_EXIT_2ARGS, __SUCCESS_OR_EXIT_1ARGS, )
-
-// Wrapper to capture all arguments and invoke the real wrapper for SuccessOrExit's second argument
-#define __LOG_METRIC_FOR_SUCCESS_OR_EXIT(...) __SELECT_MACRO_FOR_SUCCESS_OR_EXIT(__VA_ARGS__)(__VA_ARGS__)
-
-// Utility macro that takes the status and wraps it to emit the metric in a SuccessOrExit macro, when a metric key is specified
-#define __SUCCESS_OR_EXIT_2ARGS(aStatus, metricKey)                                                                                \
-    chip::Tracing::utils::LogMetricIfError(aStatus, metricKey)
-
-// Utility macro that just evaluates the status when no metric key is specified
-#define __SUCCESS_OR_EXIT_1ARGS(aStatus) ::chip::ChipError::IsSuccess((aStatus))
+/**
+ *  @def SuccessOrExitWithMetric(kMetriKey, aStatus)
+ *
+ *  @brief
+ *    This checks for the specified status, which is expected to
+ *    commonly be successful (CHIP_NO_ERROR), and branches to
+ *    the local label 'exit' if the status is unsuccessful.
+ *    If unsuccessful, a metric with key kMetriKey is emitted with
+ *    the error code as the value of the metric.
+ *
+ *  Example Usage:
+ *
+ *  @code
+ *  CHIP_ERROR TryHard()
+ *  {
+ *      CHIP_ERROR err;
+ *
+ *      err = TrySomething();
+ *      SuccessOrExitWithMetric(kMetricKey, err);
+ *
+ *      err = TrySomethingElse();
+ *      SuccessOrExitWithMetric(kMetricKey, err);
+ *
+ *  exit:
+ *      return err;
+ *  }
+ *  @endcode
+ *
+ *  @param[in]  kMetricKey  Metric key for the metric event to be emitted
+ *                          if the condition evaluates to false. The value
+ *                          for the metric is result of the expression aStatus.
+ *  @param[in]  aStatus     A scalar status to be evaluated against zero (0).
+ *
+ */
+#define SuccessOrExitWithMetric(kMetricKey, aStatus) nlEXPECT(::chip::Tracing::utils::LogMetricIfError(kMetricKey, aStatus), exit)
 
-// Utility macro used by SuccessOrExit macro to handle an optional second metric key argument and emit an event, if specified
-#define LOG_METRIC_FOR_SUCCESS_OR_EXIT(aStatus, ...) __LOG_METRIC_FOR_SUCCESS_OR_EXIT(aStatus, ##__VA_ARGS__)
+/**
+ *  @def VerifyOrExitWithMetric(kMetricKey, aCondition, anAction)
+ *
+ *  @brief
+ *    This checks for the specified condition, which is expected to
+ *    commonly be true, and both executes @a anAction and branches to
+ *    the local label 'exit' if the condition is false. If the condition
+ *    is false a metric event with the specified key is emitted with value
+ *    set to the result of the expression anAction.
+ *
+ *  Example Usage:
+ *
+ *  @code
+ *  CHIP_ERROR MakeBuffer(const uint8_t *& buf)
+ *  {
+ *      CHIP_ERROR err = CHIP_NO_ERROR;
+ *
+ *      buf = (uint8_t *)malloc(1024);
+ *      VerifyOrExitWithMetric(kMetricKey, buf != NULL, err = CHIP_ERROR_NO_MEMORY);
+ *
+ *      memset(buf, 0, 1024);
+ *
+ *  exit:
+ *      return err;
+ *  }
+ *  @endcode
+ *
+ *  @param[in]  kMetricKey  Metric key for the metric event to be emitted
+ *                          if the aCondition evaluates to false. The value
+ *                          for the metric is result of the expression anAction.
+ *  @param[in]  aCondition  A Boolean expression to be evaluated.
+ *  @param[in]  anAction    An expression or block to execute when the
+ *                          assertion fails.
+ */
+#define VerifyOrExitWithMetric(kMetricKey, aCondition, anAction) nlEXPECT_ACTION(aCondition, exit, MATTER_LOG_METRIC(kMetricKey, anAction))
 
-////////////////////// Metric LOGGING
+/*
+ * Utility Macros to support optional arguments for MATTER_LOG_METRIC_XYZ macros
+ */
 
 // Utility to always return the 4th argument from macro parameters
 #define __GET_4TH_ARG(_a1, _a2, _a3, _a4, ...) _a4
@@ -93,6 +135,11 @@
         ::chip::Tracing::Internal::LogMetricEvent(_metric_event);                                                                        \
     } while (false)
 
+
+////////////////////////
+// Metric logging macros
+////////////////////////
+
 /**
  *  @def MATTER_LOG_METRIC
  *
@@ -156,20 +203,26 @@
 
 #else // Tracing is disabled
 
+////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+// Remap Success, Return, and Verify macros to the ones without metrics
+////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+
+#define SuccessOrExitWithMetric(kMetricKey, aStatus) SuccessOrExit(aStatus)
+
+#define VerifyOrExitWithMetric(kMetricKey, aCondition, anAction) VerifyOrExit(aCondition, anAction)
+
+
+////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+// Map all MATTER_LOG_METRIC_XYZ macros to noops
+////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+
 #define __MATTER_LOG_METRIC_DISABLE(...)                                                                                           \
     do                                                                                                                             \
     {                                                                                                                              \
     } while (false)
 
-// Noop for logging metrics
 #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__)
 
-// Default behavior is to just execute the action
-#define LOG_METRIC_FOR_VERIFY_OR_EXIT_ACTION(anAction, ...) anAction
-
-// Default behavior is to just evaluate the status
-#define LOG_METRIC_FOR_SUCCESS_OR_EXIT(aStatus, ...) ::chip::ChipError::IsSuccess((aStatus))
-
 #endif // MATTER_TRACING_ENABLED