Modified MTRMetrics to vend specific object rather than id type (#32425)
- MTRMetrics vends MTRMetricData which has well defined schema
- Cleaned up internals of MTRMetricData
- Fixed potential unbounded growth if metrics are enabled on Darwin
- Removed stale metric keys that are not used and were meant to be
removed in initial framework for metrics
diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDelegateBridge.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDelegateBridge.mm
index 04ac68a..c5d47b4 100644
--- a/src/darwin/Framework/CHIP/MTRDeviceControllerDelegateBridge.mm
+++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDelegateBridge.mm
@@ -122,6 +122,10 @@
id<MTRDeviceControllerDelegate> strongDelegate = mDelegate;
MTRDeviceController * strongController = mController;
if (strongDelegate && mQueue && strongController) {
+
+ // Always collect the metrics to avoid unbounded growth of the stats in the collector
+ MTRMetrics * metrics = [[MTRMetricsCollector sharedInstance] metricSnapshot:TRUE];
+
if ([strongDelegate respondsToSelector:@selector(controller:commissioningComplete:nodeID:)] ||
[strongDelegate respondsToSelector:@selector(controller:commissioningComplete:nodeID:metrics:)]) {
dispatch_async(mQueue, ^{
@@ -133,8 +137,6 @@
// If the client implements the metrics delegate, prefer that over others
if ([strongDelegate respondsToSelector:@selector(controller:commissioningComplete:nodeID:metrics:)]) {
- // Create a snapshot and clear for next operation
- MTRMetrics * metrics = [[MTRMetricsCollector sharedInstance] metricSnapshot:TRUE];
[strongDelegate controller:strongController commissioningComplete:nsError nodeID:nodeID metrics:metrics];
} else {
[strongDelegate controller:strongController commissioningComplete:nsError nodeID:nodeID];
diff --git a/src/darwin/Framework/CHIP/MTRMetrics.h b/src/darwin/Framework/CHIP/MTRMetrics.h
index 483de01..94d4539 100644
--- a/src/darwin/Framework/CHIP/MTRMetrics.h
+++ b/src/darwin/Framework/CHIP/MTRMetrics.h
@@ -21,6 +21,31 @@
NS_ASSUME_NONNULL_BEGIN
/**
+ * Representation of metric data corresponding to a metric event.
+ */
+MTR_NEWLY_AVAILABLE
+@interface MTRMetricData : NSObject
+
+/**
+ * Value for the metric data. The value may be nil depending on the event emitted.
+ */
+@property (nonatomic, nullable, readonly, copy) NSNumber * value;
+
+/**
+ * Error code for the metric data. This value when valid, holds the error code value
+ * of the operation associated with the event.
+ */
+@property (nonatomic, nullable, readonly, copy) NSNumber * errorCode;
+
+/**
+ * Duration of event associated with the metric. This value may be nil depending on
+ * the event emitted.
+ */
+@property (nonatomic, nullable, readonly, copy) NSNumber * durationMicroseconds;
+
+@end
+
+/**
* A representation of a collection of metrics data for an operation.
*/
MTR_NEWLY_AVAILABLE
@@ -35,13 +60,13 @@
@property (nonatomic, readonly, copy) NSArray<NSString *> * allKeys;
/**
- * @brief Returns metric object corresponding to the metric identified by its key
+ * @brief Returns metric data corresponding to the metric identified by its key.
*
* @param [in] key Name of the metric
*
- * @return An object containing the metric data, nil if key is invalid
+ * @return An object containing the metric data, nil if key is invalid.
*/
-- (nullable id)valueForKey:(NSString *)key;
+- (nullable MTRMetricData *)metricDataForKey:(NSString *)key;
@end
diff --git a/src/darwin/Framework/CHIP/MTRMetrics.mm b/src/darwin/Framework/CHIP/MTRMetrics.mm
index 8c338b7..a430c6c 100644
--- a/src/darwin/Framework/CHIP/MTRMetrics.mm
+++ b/src/darwin/Framework/CHIP/MTRMetrics.mm
@@ -21,7 +21,7 @@
#include <Matter/MTRMetrics.h>
@implementation MTRMetrics {
- NSMutableDictionary<NSString *, id> * _metricsData;
+ NSMutableDictionary<NSString *, MTRMetricData *> * _metricsData;
}
- (instancetype)init
@@ -42,8 +42,7 @@
{
return [_metricsData allKeys];
}
-
-- (nullable id)valueForKey:(NSString *)key
+- (nullable MTRMetricData *)metricDataForKey:(NSString *)key
{
if (!key) {
MTR_LOG_ERROR("Cannot get metrics value for nil key");
@@ -53,7 +52,7 @@
return _metricsData[key];
}
-- (void)setValue:(id _Nullable)value forKey:(NSString *)key
+- (void)setMetricData:(MTRMetricData * _Nullable)value forKey:(NSString *)key
{
if (!key) {
MTR_LOG_ERROR("Cannot set metrics value for nil key");
@@ -63,7 +62,7 @@
[_metricsData setValue:value forKey:key];
}
-- (void)removeValueForKey:(NSString *)key
+- (void)removeMetricDataForKey:(NSString *)key
{
if (!key) {
MTR_LOG_ERROR("Cannot remove metrics value for nil key");
diff --git a/src/darwin/Framework/CHIP/MTRMetricsCollector.mm b/src/darwin/Framework/CHIP/MTRMetricsCollector.mm
index e829c57..8c8d2b7 100644
--- a/src/darwin/Framework/CHIP/MTRMetricsCollector.mm
+++ b/src/darwin/Framework/CHIP/MTRMetricsCollector.mm
@@ -18,6 +18,7 @@
#import "MTRMetricsCollector.h"
#import "MTRLogging_Internal.h"
#include "MTRMetrics_Internal.h"
+#include <MTRMetrics.h>
#import <MTRUnfairLock.h>
#include <platform/Darwin/Tracing.h>
#include <system/SystemClock.h>
@@ -26,13 +27,17 @@
using MetricEvent = chip::Tracing::MetricEvent;
-static NSString * kMTRMetricDataValueKey = @"value";
-static NSString * kMTRMetricDataTimepointKey = @"time_point";
-static NSString * kMTRMetricDataDurationKey = @"duration_us";
-
-@implementation MTRMetricsData {
+@implementation MTRMetricData {
chip::System::Clock::Microseconds64 _timePoint;
- chip::System::Clock::Microseconds64 _duration;
+ MetricEvent::Type _type;
+}
+
+- (instancetype)init
+{
+ // Default is to create data for instant event type.
+ // The key can be anything since it is not really used in this context.
+ MetricEvent event(MetricEvent::Type::kInstantEvent, "");
+ return [self initWithMetricEvent:event];
}
- (instancetype)initWithMetricEvent:(const MetricEvent &)event
@@ -41,60 +46,50 @@
return nil;
}
+ _type = event.type();
+
+ using EventType = MetricEvent::Type;
+ switch (_type) {
+ // Capture timepoint for begin and end to calculate duration
+ case EventType::kBeginEvent:
+ case EventType::kEndEvent:
+ _timePoint = chip::System::SystemClock().GetMonotonicMicroseconds64();
+ break;
+ case EventType::kInstantEvent:
+ _timePoint = chip::System::Clock::Microseconds64(0);
+ break;
+ }
+
using ValueType = MetricEvent::Value::Type;
switch (event.ValueType()) {
case ValueType::kInt32:
_value = [NSNumber numberWithInteger:event.ValueInt32()];
break;
case ValueType::kUInt32:
- _value = [NSNumber numberWithInteger:event.ValueUInt32()];
+ _value = [NSNumber numberWithUnsignedInteger:event.ValueUInt32()];
break;
case ValueType::kChipErrorCode:
- _value = [NSNumber numberWithInteger:event.ValueErrorCode()];
+ _errorCode = [NSNumber numberWithUnsignedInteger:event.ValueErrorCode()];
break;
case ValueType::kUndefined:
- default:
- _value = nil;
+ break;
}
-
- _timePoint = chip::System::SystemClock().GetMonotonicMicroseconds64();
- _duration = chip::System::Clock::Microseconds64(0);
return self;
}
-- (void)setDurationFromMetricData:(MTRMetricsData *)fromData
+- (void)setDurationFromMetricDataAndClearCounters:(MTRMetricData *)fromData
{
- _duration = _timePoint - fromData->_timePoint;
-}
+ auto duration = _timePoint - fromData->_timePoint;
+ _durationMicroseconds = [NSNumber numberWithUnsignedLongLong:duration.count()];
-- (NSNumber *)timePointMicroseconds
-{
- return [NSNumber numberWithUnsignedLongLong:_timePoint.count()];
-}
-
-- (NSNumber *)durationMicroseconds
-{
- return [NSNumber numberWithUnsignedLongLong:_duration.count()];
+ // Clear timepoints to minimize history
+ _timePoint = fromData->_timePoint = chip::System::Clock::Microseconds64(0);
}
- (NSString *)description
{
- return [NSString stringWithFormat:@"MTRMetricsData: Value = %@, TimePoint = %@, Duration = %@ us", self.value, self.timePointMicroseconds, self.durationMicroseconds];
-}
-
-- (NSDictionary *)toDictionary
-{
- NSMutableDictionary * dictRepresentation = [NSMutableDictionary dictionary];
- if (self.value) {
- [dictRepresentation setValue:self.value forKey:kMTRMetricDataValueKey];
- }
- if (auto tmPt = self.timePointMicroseconds) {
- [dictRepresentation setValue:tmPt forKey:kMTRMetricDataTimepointKey];
- }
- if (auto duration = self.durationMicroseconds) {
- [dictRepresentation setValue:duration forKey:kMTRMetricDataDurationKey];
- }
- return dictRepresentation;
+ return [NSString stringWithFormat:@"<MTRMetricData>: Type %d, Value = %@, Error Code = %@, Duration = %@ us",
+ static_cast<int>(_type), self.value, self.errorCode, self.durationMicroseconds];
}
@end
@@ -123,8 +118,9 @@
@implementation MTRMetricsCollector {
os_unfair_lock _lock;
- NSMutableDictionary<NSString *, MTRMetricsData *> * _metricsDataCollection;
+ NSMutableDictionary<NSString *, MTRMetricData *> * _metricsDataCollection;
chip::Tracing::signposts::DarwinTracingBackend _tracingBackend;
+ BOOL _tracingBackendRegistered;
}
+ (instancetype)sharedInstance
@@ -152,32 +148,43 @@
}
_lock = OS_UNFAIR_LOCK_INIT;
_metricsDataCollection = [NSMutableDictionary dictionary];
+ _tracingBackendRegistered = FALSE;
return self;
}
- (void)registerTracingBackend
{
std::lock_guard lock(_lock);
- chip::Tracing::Register(_tracingBackend);
- MTR_LOG_INFO("Registered tracing backend with the registry");
+
+ // Register only once
+ if (!_tracingBackendRegistered) {
+ chip::Tracing::Register(_tracingBackend);
+ MTR_LOG_INFO("Registered tracing backend with the registry");
+ _tracingBackendRegistered = TRUE;
+ }
}
- (void)unregisterTracingBackend
{
std::lock_guard lock(_lock);
- chip::Tracing::Unregister(_tracingBackend);
- MTR_LOG_INFO("Unregistered tracing backend with the registry");
+
+ // Unregister only if registered before
+ if (_tracingBackendRegistered) {
+ chip::Tracing::Unregister(_tracingBackend);
+ MTR_LOG_INFO("Unregistered tracing backend with the registry");
+ _tracingBackendRegistered = FALSE;
+ }
}
static inline NSString * suffixNameForMetricType(MetricEvent::Type type)
{
switch (type) {
case MetricEvent::Type::kBeginEvent:
- return @"-begin";
+ return @"_begin";
case MetricEvent::Type::kEndEvent:
- return @"-end";
+ return @"_end";
case MetricEvent::Type::kInstantEvent:
- return @"-instant";
+ return @"_instant";
}
}
@@ -211,14 +218,14 @@
// Create the new metric key based event type
auto metricsKey = [NSString stringWithFormat:@"%s%@", event.key(), suffixNameForMetric(event)];
- MTRMetricsData * data = [[MTRMetricsData alloc] initWithMetricEvent:event];
+ MTRMetricData * data = [[MTRMetricData alloc] initWithMetricEvent:event];
// If End event, compute its duration using the Begin event
if (event.type() == MetricEvent::Type::kEndEvent) {
auto metricsBeginKey = [NSString stringWithFormat:@"%s%@", event.key(), suffixNameForMetricType(MetricEvent::Type::kBeginEvent)];
- MTRMetricsData * beginMetric = _metricsDataCollection[metricsBeginKey];
+ MTRMetricData * beginMetric = _metricsDataCollection[metricsBeginKey];
if (beginMetric) {
- [data setDurationFromMetricData:beginMetric];
+ [data setDurationFromMetricDataAndClearCounters:beginMetric];
} else {
// Unbalanced end
MTR_LOG_ERROR("Unable to find Begin event corresponding to Metric Event: %s", event.key());
@@ -230,7 +237,7 @@
// If the event is a begin or end event, implicitly emit a corresponding instant event
if (event.type() == MetricEvent::Type::kBeginEvent || event.type() == MetricEvent::Type::kEndEvent) {
MetricEvent instantEvent(MetricEvent::Type::kInstantEvent, event.key());
- data = [[MTRMetricsData alloc] initWithMetricEvent:instantEvent];
+ data = [[MTRMetricData alloc] initWithMetricEvent:instantEvent];
metricsKey = [NSString stringWithFormat:@"%s%@", event.key(), suffixNameForMetric(instantEvent)];
[_metricsDataCollection setValue:data forKey:metricsKey];
}
@@ -240,10 +247,9 @@
{
std::lock_guard lock(_lock);
- // Copy the MTRMetrics as NSDictionary
MTRMetrics * metrics = [[MTRMetrics alloc] initWithCapacity:[_metricsDataCollection count]];
for (NSString * key in _metricsDataCollection) {
- [metrics setValue:[_metricsDataCollection[key] toDictionary] forKey:key];
+ [metrics setMetricData:_metricsDataCollection[key] forKey:key];
}
// Clear curent stats, if specified
diff --git a/src/darwin/Framework/CHIP/MTRMetrics_Internal.h b/src/darwin/Framework/CHIP/MTRMetrics_Internal.h
index 757ff07..d719c61 100644
--- a/src/darwin/Framework/CHIP/MTRMetrics_Internal.h
+++ b/src/darwin/Framework/CHIP/MTRMetrics_Internal.h
@@ -19,32 +19,13 @@
NS_ASSUME_NONNULL_BEGIN
-/**
- * A representation of a metric data for an operation.
- */
-@interface MTRMetricsData : NSObject
-
-// Value for the metric. This can be null if the metric is just a fire event with no value
-@property (nonatomic, nullable, readonly, copy) NSNumber * value;
-
-// Relative time point at which the metric was emitted. This may be null.
-@property (nonatomic, nullable, readonly, copy) NSNumber * timePointMicroseconds;
-
-// During for the event. This may be null.
-@property (nonatomic, nullable, readonly, copy) NSNumber * durationMicroseconds;
-
-// Convert contents to a dictionary
-- (NSDictionary *)toDictionary;
-
-@end
-
@interface MTRMetrics ()
- (instancetype)initWithCapacity:(NSUInteger)numItems;
-- (void)setValue:(id _Nullable)value forKey:(NSString *)key;
+- (void)setMetricData:(MTRMetricData * _Nullable)value forKey:(NSString *)key;
-- (void)removeValueForKey:(NSString *)key;
+- (void)removeMetricDataForKey:(NSString *)key;
@end
diff --git a/src/darwin/Framework/CHIPTests/MTRMetricsTests.m b/src/darwin/Framework/CHIPTests/MTRMetricsTests.m
index 696f55b..d4bbcc3 100644
--- a/src/darwin/Framework/CHIPTests/MTRMetricsTests.m
+++ b/src/darwin/Framework/CHIPTests/MTRMetricsTests.m
@@ -50,10 +50,14 @@
- (void)test001_TestAllKeys
{
MTRMetrics * metrics = [[MTRMetrics alloc] initWithCapacity:4];
- [metrics setValue:@"metricsCounter1" forKey:@"com.matter.metrics.counter1"];
- [metrics setValue:@"metricsCounter2" forKey:@"com.matter.metrics.counter2"];
- [metrics setValue:@"metricsCounter3" forKey:@"com.matter.metrics.counter3"];
- [metrics setValue:@"metricsCounter4" forKey:@"com.matter.metrics.counter4"];
+ MTRMetricData * metricData1 = [MTRMetricData new];
+ MTRMetricData * metricData2 = [MTRMetricData new];
+ MTRMetricData * metricData3 = [MTRMetricData new];
+ MTRMetricData * metricData4 = [MTRMetricData new];
+ [metrics setMetricData:metricData1 forKey:@"com.matter.metrics.counter1"];
+ [metrics setMetricData:metricData2 forKey:@"com.matter.metrics.counter2"];
+ [metrics setMetricData:metricData3 forKey:@"com.matter.metrics.counter3"];
+ [metrics setMetricData:metricData4 forKey:@"com.matter.metrics.counter4"];
NSArray<NSString *> * keys = [metrics allKeys];
XCTAssertTrue([keys count] == 4);
@@ -63,67 +67,63 @@
XCTAssertTrue([keys containsObject:@"com.matter.metrics.counter4"]);
XCTAssertFalse([keys containsObject:@"com.matter.metrics.counter5"]);
}
+
- (void)test002_TestOneKey
{
MTRMetrics * metrics = [[MTRMetrics alloc] initWithCapacity:1];
- [metrics setValue:@"metricsCounter1" forKey:@"com.matter.metrics.counter1"];
+ MTRMetricData * metricData1 = [MTRMetricData new];
+ [metrics setMetricData:metricData1 forKey:@"com.matter.metrics.counter1"];
NSArray<NSString *> * keys = [metrics allKeys];
XCTAssertTrue([keys count] == 1);
XCTAssertTrue([keys containsObject:@"com.matter.metrics.counter1"]);
}
-- (void)test003_TestMultipleKeys
-{
- MTRMetrics * metrics = [[MTRMetrics alloc] initWithCapacity:3];
- [metrics setValue:@"metricsCounter1" forKey:@"com.matter.metrics.counter1"];
- [metrics setValue:@"metricsCounter2" forKey:@"com.matter.metrics.counter2"];
- [metrics setValue:[NSError errorWithDomain:MTRErrorDomain code:MTRErrorCodeInvalidState userInfo:nil] forKey:@"com.matter.metrics.counter3"];
-
- NSArray<NSString *> * keys = [metrics allKeys];
- XCTAssertTrue([keys count] == 3);
- XCTAssertTrue([keys containsObject:@"com.matter.metrics.counter1"]);
- XCTAssertTrue([keys containsObject:@"com.matter.metrics.counter2"]);
- XCTAssertTrue([keys containsObject:@"com.matter.metrics.counter3"]);
- XCTAssertFalse([keys containsObject:@"com.matter.metrics.counter4"]);
-}
-
-- (void)test004_TestValueForKey
+- (void)test003_TestValueForKey
{
MTRMetrics * metrics = [[MTRMetrics alloc] initWithCapacity:1];
- [metrics setValue:@"metricsCounter1" forKey:@"com.matter.metrics.counter1"];
+ MTRMetricData * metricData1 = [MTRMetricData new];
+ [metrics setMetricData:metricData1 forKey:@"com.matter.metrics.counter1"];
- XCTAssertEqualObjects([metrics valueForKey:@"com.matter.metrics.counter1"], @"metricsCounter1");
+ XCTAssertEqualObjects([metrics metricDataForKey:@"com.matter.metrics.counter1"], metricData1);
}
-- (void)test005_TestMultipleValueForKeys
+- (void)test004_TestMultipleValueForKeys
{
MTRMetrics * metrics = [[MTRMetrics alloc] initWithCapacity:3];
- [metrics setValue:@"metricsCounter1" forKey:@"com.matter.metrics.counter1"];
- [metrics setValue:@"metricsCounter2" forKey:@"com.matter.metrics.counter2"];
- [metrics setValue:[NSError errorWithDomain:MTRErrorDomain code:MTRErrorCodeInvalidState userInfo:nil] forKey:@"com.matter.metrics.counter3"];
+ MTRMetricData * metricData1 = [MTRMetricData new];
+ MTRMetricData * metricData2 = [MTRMetricData new];
+ MTRMetricData * metricData3 = [MTRMetricData new];
+ [metrics setMetricData:metricData1 forKey:@"com.matter.metrics.counter1"];
+ [metrics setMetricData:metricData2 forKey:@"com.matter.metrics.counter2"];
+ [metrics setMetricData:metricData3 forKey:@"com.matter.metrics.counter3"];
- XCTAssertEqualObjects([metrics valueForKey:@"com.matter.metrics.counter1"], @"metricsCounter1");
- XCTAssertEqualObjects([metrics valueForKey:@"com.matter.metrics.counter2"], @"metricsCounter2");
- XCTAssertEqualObjects([metrics valueForKey:@"com.matter.metrics.counter3"], [NSError errorWithDomain:MTRErrorDomain code:MTRErrorCodeInvalidState userInfo:nil]);
+ XCTAssertEqualObjects([metrics metricDataForKey:@"com.matter.metrics.counter1"], metricData1);
+ XCTAssertEqualObjects([metrics metricDataForKey:@"com.matter.metrics.counter2"], metricData2);
+ XCTAssertEqualObjects([metrics metricDataForKey:@"com.matter.metrics.counter3"], metricData3);
+ XCTAssertNotEqualObjects([metrics metricDataForKey:@"com.matter.metrics.counter3"], metricData2);
}
-- (void)test006_TestValueRemoval
+- (void)test005_TestValueRemoval
{
MTRMetrics * metrics = [[MTRMetrics alloc] initWithCapacity:2];
- [metrics setValue:@"metricsCounter1" forKey:@"com.matter.metrics.counter1"];
- [metrics setValue:@"metricsCounter2" forKey:@"com.matter.metrics.counter2"];
+ MTRMetricData * metricData1 = [MTRMetricData new];
+ MTRMetricData * metricData2 = [MTRMetricData new];
+ [metrics setMetricData:metricData1 forKey:@"com.matter.metrics.counter1"];
+ [metrics setMetricData:metricData2 forKey:@"com.matter.metrics.counter2"];
NSArray<NSString *> * keys = [metrics allKeys];
XCTAssertTrue([keys count] == 2);
- [metrics setValue:nil forKey:@"com.matter.metrics.counter2"];
+ [metrics setMetricData:nil forKey:@"com.matter.metrics.counter2"];
keys = [metrics allKeys];
XCTAssertTrue([keys count] == 1);
XCTAssertTrue([keys containsObject:@"com.matter.metrics.counter1"]);
XCTAssertFalse([keys containsObject:@"com.matter.metrics.counter2"]);
+ XCTAssertEqualObjects([metrics metricDataForKey:@"com.matter.metrics.counter1"], metricData1);
+ XCTAssertNotEqualObjects([metrics metricDataForKey:@"com.matter.metrics.counter1"], metricData2);
- [metrics setValue:nil forKey:@"com.matter.metrics.counter1"];
+ [metrics setMetricData:nil forKey:@"com.matter.metrics.counter1"];
keys = [metrics allKeys];
XCTAssertTrue([keys count] == 0);
}
diff --git a/src/tracing/metric_keys.h b/src/tracing/metric_keys.h
index f52b2ab..2f80af9 100644
--- a/src/tracing/metric_keys.h
+++ b/src/tracing/metric_keys.h
@@ -29,16 +29,7 @@
/**
* List of supported metric keys
*/
-constexpr MetricKey kMetricDiscoveryOverBLE = "disc-over-ble";
-constexpr MetricKey kMetricDiscoveryOnNetwork = "disc-on-nw";
-constexpr MetricKey kMetricPASESession = "pase-session";
-constexpr MetricKey kMetricPASESessionPair = "pase-session-pair";
-constexpr MetricKey kMetricPASESessionBLE = "pase-session-ble";
-constexpr MetricKey kMetricAttestationResult = "attestation-result";
-constexpr MetricKey kMetricAttestationOverridden = "attestation-overridden";
-constexpr MetricKey kMetricCASESession = "case-session";
-constexpr MetricKey kMetricCASESessionEstState = "case-conn-est";
-constexpr MetricKey kMetricWiFiRSSI = "wifi_rssi";
+constexpr MetricKey kMetricWiFiRSSI = "wifi_rssi";
} // namespace Tracing
} // namespace chip
