refactor stat_item: get rid of FIFO and "skipped" error

Intead of attempting to store all distinct values of a reporting period,
just store min, max, last as well as a sum and N of each reporting
period.

This gets rid of error messages like

  DLSTATS ERROR stat_item.c:285 num_bts:oml_connected: 44 stats values skipped

while at the same time more accurately reporting the max value for each
reporting period. (So far stats_item only reports the max value; keep
that part unchanged, as shown in stats_test.c.)

With the other so far unused values (min, sum), we are ready to also
report the minimum value as well as an average value per reporting
period in the future, if/when our stats reporter allows for it.

Store the complete record of the previous reporting period. So far we
only compare the 'max' value, but like this we are ready to also see
changes in min, last and average value between reporting periods.

This patch breaks API by removing:
- struct members osmo_stats_item.stats_next_id, .last_offs and .values[]
- struct osmo_stats_item_value
- osmo_stat_item_get_next()
- osmo_stat_item_discard()
- osmo_stat_item_discard_all()
and by making struct osmo_stats_item opaque.
In libosmocore, we do have a policy of never breaking API. But since the
above should never be accessed by users of the osmo_stats_item API -- or
if they are, would no longer yield useful results, we decided to make an
exception in this case. The alternative would be to introduce a new
osmo_stats_item2 API and maintaining an unused legacy osmo_stats_item
forever, but we decided that the effort is not worth it. There are no
known users of the removed items.

Related: SYS#5542
Change-Id: I137992a5479fc39bbceb6c6c2af9c227bd33b39b

3 files changed, 124 insertions, 106 deletions

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 22591fb9..0880561c 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -87,6 +87,7 @@ utils_utils_test_LDADD = $(LDADD) $(top_builddir)/src/gsm/libosmogsm.la
 
 stats_stats_test_SOURCES = stats/stats_test.c
 stats_stats_test_LDADD = $(LDADD) $(top_builddir)/src/gsm/libosmogsm.la
+stats_stats_test_CPPFLAGS = $(AM_CPPFLAGS) -I$(top_srcdir)/src
 
 a5_a5_test_SOURCES = a5/a5_test.c
 a5_a5_test_LDADD = $(LDADD) $(top_builddir)/src/gsm/libgsmint.la
diff --git a/tests/stats/stats_test.c b/tests/stats/stats_test.c
index 9489e603..dea2bf7c 100644
--- a/tests/stats/stats_test.c
+++ b/tests/stats/stats_test.c
@@ -29,6 +29,8 @@
 #include <osmocom/core/rate_ctr.h>
 #include <osmocom/core/stats.h>
 
+#include <stat_item_internal.h>
+
 #include <stdio.h>
 #include <inttypes.h>
 
@@ -88,11 +90,10 @@ static void stat_test(void)
 
 	struct osmo_stat_item_group *sgrp2;
 	const struct osmo_stat_item *sitem1, *sitem2;
-	int rc;
 	int32_t value;
-	int32_t next_id_a = 1;
-	int32_t next_id_b = 1;
 	int i;
+	int64_t sum1;
+	int64_t sum2;
 
 	OSMO_ASSERT(statg != NULL);
 
@@ -117,124 +118,144 @@ static void stat_test(void)
 	OSMO_ASSERT(sitem2 != sitem1);
 	OSMO_ASSERT(sitem2 == osmo_stat_item_group_get_item(statg, TEST_B_ITEM));
 
+	/* No value set yet, expecting default value from osmo_stat_item_desc definition above. */
 	value = osmo_stat_item_get_last(osmo_stat_item_group_get_item(statg, TEST_A_ITEM));
 	OSMO_ASSERT(value == -1);
 
-	rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a, &value);
-	OSMO_ASSERT(rc == 0);
-
+	/* No value set yet, expecting new value in all fields */
 	osmo_stat_item_set(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), 1);
-
+	sum1 = 1;
 	value = osmo_stat_item_get_last(osmo_stat_item_group_get_item(statg, TEST_A_ITEM));
 	OSMO_ASSERT(value == 1);
+	OSMO_ASSERT(sitem1->value.n == 1);
+	OSMO_ASSERT(sitem1->value.min == 1);
+	OSMO_ASSERT(sitem1->value.last == 1);
+	OSMO_ASSERT(sitem1->value.max == 1);
+	OSMO_ASSERT(sitem1->value.sum == 1);
 
-	rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a, &value);
-	OSMO_ASSERT(rc == 1);
-	OSMO_ASSERT(value == 1);
-
-	rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a, &value);
-	OSMO_ASSERT(rc == 0);
-
+	sum2 = 0;
 	for (i = 2; i <= 32; i++) {
 		osmo_stat_item_set(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), i);
+		sum1 += i;
 		osmo_stat_item_set(osmo_stat_item_group_get_item(statg, TEST_B_ITEM), 1000 + i);
-
-		rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a, &value);
-		OSMO_ASSERT(rc == 1);
-		OSMO_ASSERT(value == i);
-
-		rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_B_ITEM), &next_id_b, &value);
-		OSMO_ASSERT(rc == 1);
-		OSMO_ASSERT(value == 1000 + i);
+		sum2 += 1000 + i;
 	}
+	OSMO_ASSERT(sitem1->value.n == 32);
+	OSMO_ASSERT(sitem1->value.min == 1);
+	OSMO_ASSERT(sitem1->value.last == 32);
+	OSMO_ASSERT(sitem1->value.max == 32);
+	OSMO_ASSERT(sitem1->value.sum == sum1);
+
+	OSMO_ASSERT(sitem2->value.n == 31);
+	OSMO_ASSERT(sitem2->value.min == 1002);
+	OSMO_ASSERT(sitem2->value.last == 1032);
+	OSMO_ASSERT(sitem2->value.max == 1032);
+	OSMO_ASSERT(sitem2->value.sum == sum2);
 
 	/* check if dec & inc is working */
 	osmo_stat_item_set(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), 42);
-	rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a, &value);
-	OSMO_ASSERT(rc == 1);
-	OSMO_ASSERT(value == 42);
+	sum1 += 42;
+	OSMO_ASSERT(sitem1->value.n == 33);
+	OSMO_ASSERT(sitem1->value.min == 1);
+	OSMO_ASSERT(sitem1->value.last == 42);
+	OSMO_ASSERT(osmo_stat_item_get_last(osmo_stat_item_group_get_item(statg, TEST_A_ITEM)) == 42);
+	OSMO_ASSERT(sitem1->value.max == 42);
+	OSMO_ASSERT(sitem1->value.sum == sum1);
 
 	osmo_stat_item_dec(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), 21);
-	rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a, &value);
-	OSMO_ASSERT(rc == 1);
-	OSMO_ASSERT(value == 21);
+	sum1 += 42 - 21;
+	OSMO_ASSERT(sitem1->value.n == 34);
+	OSMO_ASSERT(sitem1->value.min == 1);
+	OSMO_ASSERT(sitem1->value.last == 21);
+	OSMO_ASSERT(osmo_stat_item_get_last(osmo_stat_item_group_get_item(statg, TEST_A_ITEM)) == 21);
+	OSMO_ASSERT(sitem1->value.max == 42);
+	OSMO_ASSERT(sitem1->value.sum == sum1);
 
 	osmo_stat_item_inc(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), 21);
-	rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a, &value);
-	OSMO_ASSERT(rc == 1);
-	OSMO_ASSERT(value == 42);
-
-	/* Keep 2 in FIFO */
-	osmo_stat_item_set(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), 33);
-	osmo_stat_item_set(osmo_stat_item_group_get_item(statg, TEST_B_ITEM), 1000 + 33);
-
-	for (i = 34; i <= 64; i++) {
-		osmo_stat_item_set(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), i);
-		osmo_stat_item_set(osmo_stat_item_group_get_item(statg, TEST_B_ITEM), 1000 + i);
-
-		rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a, &value);
-		OSMO_ASSERT(rc == 1);
-		OSMO_ASSERT(value == i-1);
-
-		rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_B_ITEM), &next_id_b, &value);
-		OSMO_ASSERT(rc == 1);
-		OSMO_ASSERT(value == 1000 + i-1);
-	}
-
-	rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a, &value);
-	OSMO_ASSERT(rc == 1);
-	OSMO_ASSERT(value == 64);
-
-	rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_B_ITEM), &next_id_b, &value);
-	OSMO_ASSERT(rc == 1);
-	OSMO_ASSERT(value == 1000 + 64);
-
-	/* Overrun FIFOs */
-	for (i = 65; i <= 96; i++) {
-		osmo_stat_item_set(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), i);
-		osmo_stat_item_set(osmo_stat_item_group_get_item(statg, TEST_B_ITEM), 1000 + i);
-	}
-
-	fprintf(stderr, "Skipping %d values\n", 93 - 65);
-	rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a, &value);
-	OSMO_ASSERT(rc == 93 - 65 + 1);
-	OSMO_ASSERT(value == 93);
-
-	for (i = 94; i <= 96; i++) {
-		rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a, &value);
-		OSMO_ASSERT(rc == 1);
-		OSMO_ASSERT(value == i);
-	}
-
-	fprintf(stderr, "Skipping %d values\n", 90 - 65);
-	rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_B_ITEM), &next_id_b, &value);
-	OSMO_ASSERT(rc == 90 - 65 + 1);
-	OSMO_ASSERT(value == 1000 + 90);
-
-	for (i = 91; i <= 96; i++) {
-		rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_B_ITEM), &next_id_b, &value);
-		OSMO_ASSERT(rc == 1);
-		OSMO_ASSERT(value == 1000 + i);
-	}
-
-	/* Test Discard (single item) */
+	sum1 += 42;
+	OSMO_ASSERT(sitem1->value.n == 35);
+	OSMO_ASSERT(sitem1->value.min == 1);
+	OSMO_ASSERT(sitem1->value.last == 42);
+	OSMO_ASSERT(osmo_stat_item_get_last(osmo_stat_item_group_get_item(statg, TEST_A_ITEM)) == 42);
+	OSMO_ASSERT(sitem1->value.max == 42);
+	OSMO_ASSERT(sitem1->value.sum == sum1);
+
+	/* Test item flush, reporting period elapsing */
+	osmo_stat_item_flush(osmo_stat_item_group_get_item(statg, TEST_A_ITEM));
+	OSMO_ASSERT(sitem1->value.n == 0);
+	OSMO_ASSERT(sitem1->value.min == 42);
+	OSMO_ASSERT(sitem1->value.last == 42);
+	OSMO_ASSERT(osmo_stat_item_get_last(osmo_stat_item_group_get_item(statg, TEST_A_ITEM)) == 42);
+	OSMO_ASSERT(sitem1->value.max == 42);
+	OSMO_ASSERT(sitem1->value.sum == 0);
+
+	/* Still see the previous reporting period in reported.* */
+	OSMO_ASSERT(sitem1->reported.n == 35);
+	OSMO_ASSERT(sitem1->reported.min == 1);
+	OSMO_ASSERT(sitem1->reported.last == 42);
+	OSMO_ASSERT(sitem1->reported.max == 42);
+	OSMO_ASSERT(sitem1->reported.sum == sum1);
+
+	/* After a flush, the first item replaces the last, min and max */
 	osmo_stat_item_set(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), 97);
-	rc = osmo_stat_item_discard(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a);
-	OSMO_ASSERT(rc == 1);
-
-	rc = osmo_stat_item_discard(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a);
-	OSMO_ASSERT(rc == 0);
-
-	rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a, &value);
-	OSMO_ASSERT(rc == 0);
-
-	osmo_stat_item_set(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), 98);
-	rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a, &value);
-	OSMO_ASSERT(rc == 1);
-	OSMO_ASSERT(value == 98);
-
-	rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a, &value);
-	OSMO_ASSERT(rc == 0);
+	OSMO_ASSERT(sitem1->value.n == 1);
+	OSMO_ASSERT(sitem1->value.min == 97);
+	OSMO_ASSERT(sitem1->value.last == 97);
+	OSMO_ASSERT(osmo_stat_item_get_last(osmo_stat_item_group_get_item(statg, TEST_A_ITEM)) == 97);
+	OSMO_ASSERT(sitem1->value.max == 97);
+	OSMO_ASSERT(sitem1->value.sum == 97);
+
+	/* ...and still see the previous reporting period in reported.* */
+	OSMO_ASSERT(sitem1->reported.n == 35);
+	OSMO_ASSERT(sitem1->reported.min == 1);
+	OSMO_ASSERT(sitem1->reported.last == 42);
+	OSMO_ASSERT(sitem1->reported.max == 42);
+	OSMO_ASSERT(sitem1->reported.sum == sum1);
+
+	/* If an entire reporting period elapses without a new value, the last seen value remains. */
+	osmo_stat_item_flush(osmo_stat_item_group_get_item(statg, TEST_A_ITEM));
+	osmo_stat_item_flush(osmo_stat_item_group_get_item(statg, TEST_A_ITEM));
+	OSMO_ASSERT(sitem1->value.n == 0);
+	OSMO_ASSERT(sitem1->value.min == 97);
+	OSMO_ASSERT(sitem1->value.last == 97);
+	OSMO_ASSERT(osmo_stat_item_get_last(osmo_stat_item_group_get_item(statg, TEST_A_ITEM)) == 97);
+	OSMO_ASSERT(sitem1->value.max == 97);
+	OSMO_ASSERT(sitem1->value.sum == 0);
+
+	/* now the previous reporting period got turned around */
+	OSMO_ASSERT(sitem1->reported.n == 0);
+	OSMO_ASSERT(sitem1->reported.min == 97);
+	OSMO_ASSERT(sitem1->reported.last == 97);
+	OSMO_ASSERT(sitem1->reported.max == 97);
+	OSMO_ASSERT(sitem1->reported.sum == 0);
+
+	/* Another empty reporting period, everything remained the same. */
+	osmo_stat_item_flush(osmo_stat_item_group_get_item(statg, TEST_A_ITEM));
+	OSMO_ASSERT(sitem1->value.n == 0);
+	OSMO_ASSERT(sitem1->value.min == 97);
+	OSMO_ASSERT(sitem1->value.last == 97);
+	OSMO_ASSERT(osmo_stat_item_get_last(osmo_stat_item_group_get_item(statg, TEST_A_ITEM)) == 97);
+	OSMO_ASSERT(sitem1->value.max == 97);
+	OSMO_ASSERT(sitem1->value.sum == 0);
+	OSMO_ASSERT(sitem1->reported.n == 0);
+	OSMO_ASSERT(sitem1->reported.min == 97);
+	OSMO_ASSERT(sitem1->reported.last == 97);
+	OSMO_ASSERT(sitem1->reported.max == 97);
+	OSMO_ASSERT(sitem1->reported.sum == 0);
+
+	/* Test Reset, place back to default value. The previously reported value remains the same. */
+	osmo_stat_item_reset(osmo_stat_item_group_get_item(statg, TEST_A_ITEM));
+	OSMO_ASSERT(sitem1->value.n == 0);
+	OSMO_ASSERT(sitem1->value.min == -1);
+	OSMO_ASSERT(sitem1->value.last == -1);
+	OSMO_ASSERT(osmo_stat_item_get_last(osmo_stat_item_group_get_item(statg, TEST_A_ITEM)) == -1);
+	OSMO_ASSERT(sitem1->value.max == -1);
+	OSMO_ASSERT(sitem1->value.sum == 0);
+	OSMO_ASSERT(sitem1->reported.n == 0);
+	OSMO_ASSERT(sitem1->reported.min == 97);
+	OSMO_ASSERT(sitem1->reported.last == 97);
+	OSMO_ASSERT(sitem1->reported.max == 97);
+	OSMO_ASSERT(sitem1->reported.sum == 0);
 
 	osmo_stat_item_group_free(statg);
 
diff --git a/tests/stats/stats_test.err b/tests/stats/stats_test.err
index 92d6ce16..a890e0fa 100644
--- a/tests/stats/stats_test.err
+++ b/tests/stats/stats_test.err
@@ -1,7 +1,3 @@
-Skipping 28 values
-DLSTATS ERROR item.a: 28 stats values skipped
-Skipping 25 values
-DLSTATS ERROR item.b: 25 stats values skipped
 Start test: test_reporting
 DLGLOBAL ERROR counter group 'ctr-test:one' already exists for index 2, instead using index 3. This is a software bug that needs fixing.
 DLGLOBAL ERROR 'ctr-test.one_dot' is not a valid counter group identifier