stats: Ensure that each osmo_stat_item only reports once per interval

We should never report multiple values for a metric. It is confusing for
the log reporter and wrong for statsd. Statsd will record only one value,
but will it be the first, last, ...?
This can happen if an osmo_stat_item changes more than once within the
same reporting interval.

With this patch only one aggregate value is sent to the log reporters.
The value reported is the maximum during this interval. Other
aggregations could be possible (min, last), but reporting a (useful)
average is not because the values don't include a timestamp and most
osmo_stat_items change at unregular intervals.

Change-Id: I366ab1c66f4ae6363111ea4e41b66b7d5bcade9c
Related: SYS#4877

3 files changed, 38 insertions, 18 deletions

diff --git a/src/stats.c b/src/stats.c
index a0834d29..f2820a45 100644
--- a/src/stats.c
+++ b/src/stats.c
@@ -699,31 +699,31 @@ static int osmo_stat_item_handler(
 	int have_value;
 
 	have_value = osmo_stat_item_get_next(item, &idx, &value) > 0;
-	if (!have_value)
+	if (!have_value) {
 		/* Send the last value in case a flush is requested */
 		value = osmo_stat_item_get_last(item);
+	} else {
+		int32_t next_val;
+		/* If we have multiple values only send the max */
+		while (osmo_stat_item_get_next(item, &idx, &next_val) > 0)
+			value = OSMO_MAX(value, next_val);
+	}
 
-	do {
-		llist_for_each_entry(srep, &osmo_stats_reporter_list, list) {
-			if (!srep->running)
-				continue;
-
-			if (!have_value && !srep->force_single_flush)
-				continue;
+	llist_for_each_entry(srep, &osmo_stats_reporter_list, list) {
+		if (!srep->running)
+			continue;
 
-			if (!osmo_stats_reporter_check_config(srep,
-					statg->idx, statg->desc->class_id))
-				continue;
+		if (!have_value && !srep->force_single_flush)
+			continue;
 
-			osmo_stats_reporter_send_item(srep, statg,
-				item->desc, value);
-		}
+		if (!osmo_stats_reporter_check_config(srep,
+				statg->idx, statg->desc->class_id))
+			continue;
 
-		if (!have_value)
-			break;
+		osmo_stats_reporter_send_item(srep, statg,
+			item->desc, value);
 
-		have_value = osmo_stat_item_get_next(item, &idx, &value) > 0;
-	} while (have_value);
+	}
 
 	return 0;
 }
diff --git a/tests/stats/stats_test.c b/tests/stats/stats_test.c
index 71f710a9..05caf982 100644
--- a/tests/stats/stats_test.c
+++ b/tests/stats/stats_test.c
@@ -440,6 +440,20 @@ static void test_reporting()
 	osmo_stats_report();
 	OSMO_ASSERT(send_count == 2);
 
+	printf("report (group 1, item 1 update twice):\n");
+	osmo_stat_item_set(statg1->items[TEST_A_ITEM], 10);
+	osmo_stat_item_set(statg1->items[TEST_A_ITEM], 10);
+	send_count = 0;
+	osmo_stats_report();
+	OSMO_ASSERT(send_count == 2);
+
+	printf("report (group 1, item 1 update twice, check max):\n");
+	osmo_stat_item_set(statg1->items[TEST_A_ITEM], 20);
+	osmo_stat_item_set(statg1->items[TEST_A_ITEM], 10);
+	send_count = 0;
+	osmo_stats_report();
+	OSMO_ASSERT(send_count == 2);
+
 	printf("report (remove statg1, ctrg1):\n");
 	/* force single flush */
 	srep1->force_single_flush = 1;
diff --git a/tests/stats/stats_test.ok b/tests/stats/stats_test.ok
index 8628adb7..26d9e38d 100644
--- a/tests/stats/stats_test.ok
+++ b/tests/stats/stats_test.ok
@@ -100,6 +100,12 @@ report (group 1, counter 1 update):
 report (group 1, item 1 update):
   test2: item p= g=test.one i=1 n=item.a v=10 u=ma
   test1: item p= g=test.one i=1 n=item.a v=10 u=ma
+report (group 1, item 1 update twice):
+  test2: item p= g=test.one i=1 n=item.a v=10 u=ma
+  test1: item p= g=test.one i=1 n=item.a v=10 u=ma
+report (group 1, item 1 update twice, check max):
+  test2: item p= g=test.one i=1 n=item.a v=20 u=ma
+  test1: item p= g=test.one i=1 n=item.a v=20 u=ma
 report (remove statg1, ctrg1):
   test2: counter p= g=ctr-test:one_dot i=3 n=ctr:a v=0 d=0
   test1: counter p= g=ctr-test:one_dot i=3 n=ctr:a v=0 d=0