Fixing code review comments

src/app/OperationalSessionSetup.cpp

@@ -244,8 +244,6 @@ void OperationalSessionSetup::UpdateDeviceData(const Transport::PeerAddress & ad
         return;
     }
 
-    MATTER_LOG_METRIC_END(kMetricDeviceOperationalDiscovery, CHIP_NO_ERROR);
-
     CHIP_ERROR err = EstablishConnection(config);
     LogErrorOnFailure(err);
     if (err == CHIP_NO_ERROR)
@@ -295,6 +293,9 @@ CHIP_ERROR OperationalSessionSetup::EstablishConnection(const ReliableMessagePro
     mCASEClient = mClientPool->Allocate();
     ReturnErrorCodeIf(mCASEClient == nullptr, CHIP_ERROR_NO_MEMORY);
 
+    // Session has been established. This is when discovery is also stopped
+    MATTER_LOG_METRIC_END(kMetricDeviceOperationalDiscovery, CHIP_NO_ERROR);
+
     MATTER_LOG_METRIC_BEGIN(kMetricDeviceCASESession);
     CHIP_ERROR err = mCASEClient->EstablishSession(mInitParams, mPeerId, mDeviceAddress, config, this);
     if (err != CHIP_NO_ERROR)
@@ -601,6 +602,8 @@ CHIP_ERROR OperationalSessionSetup::LookupPeerAddress()
         return CHIP_NO_ERROR;
     }
 
+    // This call be called multiple times for each discovered address.The metric backend can handle this 
+    // and always pick the earliest occurance as the start of the event.
     MATTER_LOG_METRIC_BEGIN(kMetricDeviceOperationalDiscovery);
 
     auto const * fabricInfo = mInitParams.fabricTable->FindFabricWithIndex(mPeerId.GetFabricIndex());

src/controller/AutoCommissioner.cpp

@@ -24,14 +24,12 @@
 #include <controller/CHIPDeviceController.h>
 #include <credentials/CHIPCert.h>
 #include <lib/support/SafeInt.h>
-#include <tracing/metric_event.h>
 
 namespace chip {
 namespace Controller {
 
 using namespace chip::app::Clusters;
 using namespace chip::Crypto;
-using namespace chip::Tracing;
 using chip::app::DataModel::MakeNullable;
 using chip::app::DataModel::NullNullable;
 

src/controller/CHIPDeviceController.cpp

@@ -1967,7 +1967,7 @@ void DeviceCommissioner::OnDeviceConnectedFn(void * context, Messaging::Exchange
                                              const SessionHandle & sessionHandle)
 {
     // CASE session established.
-    MATTER_LOG_METRIC_END(kMetricDeviceOperationalSetup, CHIP_NO_ERROR);
+    MATTER_LOG_METRIC_END(kMetricDeviceCommissioningOperationalSetup, CHIP_NO_ERROR);
     DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
     VerifyOrDie(commissioner->mCommissioningStage == CommissioningStage::kFindOperationalForStayActive ||
                 commissioner->mCommissioningStage == CommissioningStage::kFindOperationalForCommissioningComplete);
@@ -1982,7 +1982,7 @@ void DeviceCommissioner::OnDeviceConnectedFn(void * context, Messaging::Exchange
 void DeviceCommissioner::OnDeviceConnectionFailureFn(void * context, const ScopedNodeId & peerId, CHIP_ERROR error)
 {
     // CASE session establishment failed.
-    MATTER_LOG_METRIC_END(kMetricDeviceOperationalSetup, error);
+    MATTER_LOG_METRIC_END(kMetricDeviceCommissioningOperationalSetup, error);
     DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
     VerifyOrDie(commissioner->mCommissioningStage == CommissioningStage::kFindOperationalForStayActive ||
                 commissioner->mCommissioningStage == CommissioningStage::kFindOperationalForCommissioningComplete);
@@ -3359,7 +3359,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
     case CommissioningStage::kFindOperationalForCommissioningComplete: {
         // If there is an error, CommissioningStageComplete will be called from OnDeviceConnectionFailureFn.
         auto scopedPeerId = GetPeerScopedId(proxy->GetDeviceId());
-        MATTER_LOG_METRIC_BEGIN(kMetricDeviceOperationalSetup);
+        MATTER_LOG_METRIC_BEGIN(kMetricDeviceCommissioningOperationalSetup);
         mSystemState->CASESessionMgr()->FindOrEstablishSession(scopedPeerId, &mOnDeviceConnectedCallback,
                                                                &mOnDeviceConnectionFailureCallback
 #if CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES

src/controller/CommissioningDelegate.cpp

@@ -144,115 +144,115 @@ const char * MetricKeyForCommissioningStage(CommissioningStage stage)
     switch (stage)
     {
     case kError:
-        return "core_cm_stg_error";
+        return "core_commissioning_stage_error";
 
     case kSecurePairing:
-        return "core_cm_stg_secure_pairing";
+        return "core_commissioning_stage_secure_pairing";
 
     case kReadCommissioningInfo:
-        return "core_cm_stg_rd_comm_info";
+        return "core_commissioning_stage_read_commissioning_info";
 
     case kReadCommissioningInfo2:
-        return "core_cm_stg_rd_comm_info2";
+        return "core_commissioning_stage_read_commissioning_info2";
 
     case kArmFailsafe:
-        return "core_cm_stg_afs";
+        return "core_commissioning_stage_arm_failsafe";
 
     case kScanNetworks:
-        return "core_cm_stg_scn_nws";
+        return "core_commissioning_stage_scan_networks";
 
     case kConfigRegulatory:
-        return "core_cm_stg_cfg_reg";
+        return "core_commissioning_stage_config_regulatory";
 
     case kConfigureUTCTime:
-        return "core_cm_stg_cfg_utc";
+        return "core_commissioning_stage_configure_utc_time";
 
     case kConfigureTimeZone:
-        return "core_cm_stg_cfg_tmz";
+        return "core_commissioning_stage_configure_timezone";
 
     case kConfigureDSTOffset:
-        return "core_cm_stg_cfg_dst_off";
+        return "core_commissioning_stage_configure_dst_offset";
 
     case kConfigureDefaultNTP:
-        return "core_cm_stg_cfg_ntp";
+        return "core_commissioning_stage_configure_default_ntp";
 
     case kSendPAICertificateRequest:
-        return "core_cm_stg_csr_req";
+        return "core_commissioning_stage_send_pai_certificate_request";
 
     case kSendDACCertificateRequest:
-        return "core_cm_stg_dac_req";
+        return "core_commissioning_stage_send_dac_certificate_request";
 
     case kSendAttestationRequest:
-        return "core_cm_stg_att_req";
+        return "core_commissioning_stage_send_attestation_request";
 
     case kAttestationVerification:
-        return "core_cm_stg_att_ver";
+        return "core_commissioning_stage_attestation_verification";
 
     case kSendOpCertSigningRequest:
-        return "core_cm_stg_opcrt_csr";
+        return "core_commissioning_stage_opcert_signing_request";
 
     case kValidateCSR:
-        return "core_cm_stg_val_csr";
+        return "core_commissioning_stage_validate_csr";
 
     case kGenerateNOCChain:
-        return "core_cm_stg_gen_noc";
+        return "core_commissioning_stage_generate_noc_chain";
 
     case kSendTrustedRootCert:
-        return "core_cm_stg_trust_rca";
+        return "core_commissioning_stage_send_trusted_root_cert";
 
     case kSendNOC:
-        return "core_cm_stg_snd_noc";
+        return "core_commissioning_stage_send_noc";
 
     case kConfigureTrustedTimeSource:
-        return "core_cm_stg_cfg_tms";
+        return "core_commissioning_stage_configure_trusted_time_source";
 
     case kICDGetRegistrationInfo:
-        return "core_cm_stg_icd_reg_info";
+        return "core_commissioning_stage_icd_get_registration_info";
 
     case kICDRegistration:
-        return "core_cm_stg_icd_reg";
+        return "core_commissioning_stage_icd_registration";
 
     case kWiFiNetworkSetup:
-        return "core_cm_stg_wifi_nw_cred";
+        return "core_commissioning_stage_wifi_network_setup";
 
     case kThreadNetworkSetup:
-        return "core_cm_stg_thrd_nw_cred";
+        return "core_commissioning_stage_thread_network_setup";
 
     case kFailsafeBeforeWiFiEnable:
-        return "core_cm_stg_afs_bf_wifi";
+        return "core_commissioning_stage_failsafe_before_wifi_enable";
 
     case kFailsafeBeforeThreadEnable:
-        return "core_cm_stg_afs_bf_thrd";
+        return "core_commissioning_stage_failsafe_before_thread_enable";
 
     case kWiFiNetworkEnable:
-        return "core_cm_stg_wifi_enbl";
+        return "core_commissioning_stage_wifi_network_enable";
 
     case kThreadNetworkEnable:
-        return "core_cm_stg_thrd_enbl";
+        return "core_commissioning_stage_thread_network_enable";
 
     case kEvictPreviousCaseSessions:
-        return "core_cm_stg_evict_case";
+        return "core_commissioning_stage_evict_previous_case_sessions";
 
     case kFindOperationalForStayActive:
-        return "core_cm_stg_op_stay_active";
+        return "core_commissioning_stage_find_operational_for_stay_active";
 
     case kFindOperationalForCommissioningComplete:
-        return "core_cm_stg_op_cm_comp";
+        return "core_commissioning_stage_find_operational_for_commissioning_complete";
 
     case kICDSendStayActive:
-        return "core_cm_stg_icd_stay_act";
+        return "core_commissioning_stage_icd_send_stay_active";
 
     case kSendComplete:
-        return "core_cm_stg_snd_comp";
+        return "core_commissioning_stage_send_complete";
 
     case kCleanup:
-        return "core_cm_stg_cleanup";
+        return "core_commissioning_stage_cleanup";
 
     case kNeedsNetworkCreds:
-        return "core_cm_stg_need_nw_creds";
+        return "core_commissioning_stage_need_network_creds";
 
     default:
-        return "core_cm_stg_unknown";
+        return "core_commissioning_stage_unknown";
     }
 }
 #endif

src/protocols/secure_channel/CASESession.cpp

@@ -47,7 +47,6 @@
 #include <system/SystemClock.h>
 #include <system/TLVPacketBufferBackingStore.h>
 #include <tracing/macros.h>
-#include <tracing/metric_event.h>
 #include <transport/SessionManager.h>
 
 namespace {

src/tracing/metric_keys.h

@@ -35,21 +35,21 @@ constexpr MetricKey kMetricWiFiRSSI = "wifi_rssi";
 constexpr MetricKey kMetricDeviceCommissionerPASESession = "core_dcm_pase_session";
 
 // Overall commissioning into a fabric
-constexpr MetricKey kMetricDeviceCommissionerCommission = "core_dcm_commission";
+constexpr MetricKey kMetricDeviceCommissionerCommission = "core_dcm_commission_device";
 
-constexpr MetricKey kMetricDeviceCommissionerCommissionStage = "core_ccm_commission_stage";
+constexpr MetricKey kMetricDeviceCommissionerCommissionStage = "core_dcm_commission_stage";
 
 // Setup Code Pairer
-constexpr MetricKey kMetricSetupCodePairerPairDevice = "core_scp_pair_dev";
+constexpr MetricKey kMetricSetupCodePairerPairDevice = "core_setup_code_pairer_pair_dev";
 
-// Operational Setup (Discovery + CASE)
-constexpr MetricKey kMetricDeviceOperationalSetup = "core_dev_opr_setup";
+// Commissioning Operational Setup (Discovery + CASE)
+constexpr MetricKey kMetricDeviceCommissioningOperationalSetup = "core_dcm_operational_setup";
 
 // Operational Discovery
-constexpr MetricKey kMetricDeviceOperationalDiscovery = "core_dev_opr_discovery";
+constexpr MetricKey kMetricDeviceOperationalDiscovery = "core_dev_operational_discovery";
 
 // Operational Discovery Attempt Count
-constexpr MetricKey kMetricDeviceOperationalDiscoveryAttemptCount = "core_dev_opdisc_attempt_ctr";
+constexpr MetricKey kMetricDeviceOperationalDiscoveryAttemptCount = "ore_dev_operational_discovery_attempt_ctr";
 
 // CASE Session
 constexpr MetricKey kMetricDeviceCASESession = "core_dev_case_session";