From 39b9544e98f5f075b01223b237fa4de00127c875 Mon Sep 17 00:00:00 2001 From: Anush Nadathur Date: Thu, 22 Feb 2024 18:22:18 +0530 Subject: [PATCH] Code Review Feedback #2: 1. Added SuccessOrExitWithMetric and VerifyOrExitWithMetric 2. Cleaned up support .gn to remove dependedency on metrics --- src/controller/CHIPDeviceController.cpp | 18 ++-- src/lib/support/BUILD.gn | 1 - src/lib/support/CodeUtils.h | 6 +- src/tracing/BUILD.gn | 12 +-- src/tracing/metric_event.h | 9 +- src/tracing/metric_macros.h | 133 +++++++++++++++++------- 6 files changed, 106 insertions(+), 73 deletions(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index a148447138a9b9..3fb653cb805220 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/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) diff --git a/src/lib/support/BUILD.gn b/src/lib/support/BUILD.gn index 52ef8b7c9b023d..02ca1eef593def 100644 --- a/src/lib/support/BUILD.gn +++ b/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", ] } diff --git a/src/lib/support/CodeUtils.h b/src/lib/support/CodeUtils.h index d32c8ef500eeeb..69dedd0e42a69c 100644 --- a/src/lib/support/CodeUtils.h +++ b/src/lib/support/CodeUtils.h @@ -31,7 +31,6 @@ #include #include #include -#include /** * 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(...) diff --git a/src/tracing/BUILD.gn b/src/tracing/BUILD.gn index bff4d96113d75b..2bce4132504100 100644 --- a/src/tracing/BUILD.gn +++ b/src/tracing/BUILD.gn @@ -38,17 +38,6 @@ 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", @@ -56,6 +45,7 @@ source_set("metrics") { public_deps = [ ":tracing_buildconfig", + "${chip_root}/src/lib/support", "${chip_root}/src/lib/core:error", ] } diff --git a/src/tracing/metric_event.h b/src/tracing/metric_event.h index f90643f0978648..e3657f04db98c5 100644 --- a/src/tracing/metric_event.h +++ b/src/tracing/metric_event.h @@ -19,6 +19,7 @@ #include #include #include +#include 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) diff --git a/src/tracing/metric_macros.h b/src/tracing/metric_macros.h index ad8ea5c73bf2c5..cd47489360ead9 100644 --- a/src/tracing/metric_macros.h +++ b/src/tracing/metric_macros.h @@ -17,46 +17,88 @@ */ #pragma once -#include #include +#include +#include #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