Change in libosmocore[master]: refactor stat_item: get rid of FIFO and "skipped" error

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

neels gerrit-no-reply at lists.osmocom.org
Wed Sep 15 06:40:37 UTC 2021


neels has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/25464 )


Change subject: refactor stat_item: get rid of FIFO and "skipped" error
......................................................................

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.

Related: SYS#5542
Change-Id: I137992a5479fc39bbceb6c6c2af9c227bd33b39b
---
M TODO-RELEASE
M include/osmocom/core/stat_item.h
M src/stat_item.c
M src/stats.c
M tests/stats/stats_test.c
M tests/stats/stats_test.err
6 files changed, 318 insertions(+), 272 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/64/25464/1

diff --git a/TODO-RELEASE b/TODO-RELEASE
index c8966cd..855ceaa 100644
--- a/TODO-RELEASE
+++ b/TODO-RELEASE
@@ -15,4 +15,5 @@
 libosmosim	osim_card_{reset,close}	New API
 libosmocore     struct rate_ctr_group, osmo_stat_item_group_desc ABI breakage due to new struct members
 libosmgsm      kdf functions   New API
-libosmocore	osmo_stat_item		ABI + API breakage due to new struct members
+libosmocore	osmo_stat_item		API change and ABI breakage due to new struct members; some osmo_stat_item API deprecated.
+libosmocore	osmo_stat_item		No FIFO buffer of values used anymore, the "skipped values" error is no longer possible.
diff --git a/include/osmocom/core/stat_item.h b/include/osmocom/core/stat_item.h
index c4e8332..717cfe9 100644
--- a/include/osmocom/core/stat_item.h
+++ b/include/osmocom/core/stat_item.h
@@ -14,28 +14,43 @@
 #define OSMO_STAT_ITEM_NOVALUE_ID 0
 #define OSMO_STAT_ITEM_NO_UNIT NULL
 
-/*! Individual entry in value FIFO */
+/*! DEPRECATED, kept only for backwards API compatibility. */
 struct osmo_stat_item_value {
 	int32_t id;		/*!< identifier of value */
 	int32_t value;		/*!< actual value */
 };
 
+struct osmo_stat_item_period {
+	/* Number of osmo_stat_item_set() that occured during the reporting period, zero if none. */
+	uint32_t n;
+	/* Smallest value seen in a reporting period. */
+	int32_t min;
+	/* Most recent value passed to osmo_stat_item_set(), or the item->desc->default_value if none. */
+	int32_t last;
+	/* Largest value seen in a reporting period. */
+	int32_t max;
+	/* Sum of all values passed to osmo_stat_item_set() in the reporting period. */
+	int64_t sum;
+};
+
 /*! data we keep for each actual item */
 struct osmo_stat_item {
 	/*! back-reference to the item description */
 	const struct osmo_stat_item_desc *desc;
-	/* internal use by stats API (stats.c): the id of the next value to
-	 * be read from the FIFO. If accessing osmo_stat_item directly, without
-	 * the stats API, store this value elsewhere. */
-	int32_t stats_next_id;
-	/* internal use by stats API: indicate if the last value sent to
-	 * reporters was actually the last value in the FIFO. This may not be
-	 * the case, as always a max of 1 or more values gets sent (OS#5215) */
-	bool stats_last_sent_was_max;
-	/*! the index of the last value written to the FIFO */
-	int16_t last_offs;
-	/*! value FIFO */
-	struct osmo_stat_item_value values[0];
+
+	/*! DEPRECATED, kept only for backwards API compatibility. */
+	int32_t not_stats_next_id;
+	/*! DEPRECATED, kept only for backwards API compatibility. */
+	int16_t not_last_offs;
+	/*! DEPRECATED, kept only for backwards API compatibility. */
+	struct osmo_stat_item_value not_values[0];
+
+	/* Current reporting period / current value. */
+	struct osmo_stat_item_period value;
+
+	/* The results of the previous reporting period. According to these, the stats reporter decides whether to
+	 * re-send values or omit an unchanged value from a report. */
+	struct osmo_stat_item_period reported;
 };
 
 /*! Statistics item description */
@@ -43,7 +58,7 @@
 	const char *name;	/*!< name of the item */
 	const char *description;/*!< description of the item */
 	const char *unit;	/*!< unit of a value */
-	unsigned int num_values;/*!< number of values to store in FIFO */
+	unsigned int num_values;/*!< DEPRECATED, this value is ignored after libosmocore version 1.5.1 */
 	int32_t default_value;	/*!< default value */
 };
 
@@ -106,12 +121,13 @@
 const struct osmo_stat_item *osmo_stat_item_get_by_name(
 	const struct osmo_stat_item_group *statg, const char *name);
 
-int osmo_stat_item_get_next(const struct osmo_stat_item *item, int32_t *next_id, int32_t *value);
+int osmo_stat_item_get_next(const struct osmo_stat_item *item, int32_t *next_id, int32_t *value)
+	OSMO_DEPRECATED("Access item->value.last, item->value.min, item->value.max or osmo_stat_item_get_avg() instead");
 
-/*! Get the last (freshest) value */
-static int32_t osmo_stat_item_get_last(const struct osmo_stat_item *item);
+void osmo_stat_item_flush(struct osmo_stat_item *item);
 
-int osmo_stat_item_discard(const struct osmo_stat_item *item, int32_t *next_id);
+int osmo_stat_item_discard(const struct osmo_stat_item *item, int32_t *next_id)
+	OSMO_DEPRECATED("Use osmo_stat_item_flush() instead");
 
 int osmo_stat_item_discard_all(int32_t *next_id)
 	OSMO_DEPRECATED("Use osmo_stat_item_discard with item-specific next_id instead");
@@ -126,9 +142,10 @@
 
 int osmo_stat_item_for_each_group(osmo_stat_item_group_handler_t handle_group, void *data);
 
+/*! Get the last (freshest) value. Simply returns item->value.last. */
 static inline int32_t osmo_stat_item_get_last(const struct osmo_stat_item *item)
 {
-	return item->values[item->last_offs].value;
+	return item->value.last;
 }
 
 void osmo_stat_item_reset(struct osmo_stat_item *item);
diff --git a/src/stat_item.c b/src/stat_item.c
index aca8ba6..6014411 100644
--- a/src/stat_item.c
+++ b/src/stat_item.c
@@ -54,20 +54,105 @@
  */
 
 /* Struct overview:
- * Each osmo_stat_item is attached to an osmo_stat_item_group, which is
- * attached to the global osmo_stat_item_groups list.
  *
- *                       osmo_stat_item_groups
- *                           /           \
- *                        group1        group2
- *                       /      \
- *                    item1    item2
- *                      |
- *                   values
- *                  /      \
- *                 1        2
- *                 |-id     |-id
- *                 '-value  '-value
+ * Group and item descriptions:
+ * Each group description exists once as osmo_stat_item_group_desc,
+ * each such group description lists N osmo_stat_item_desc, i.e. describes N stat items.
+ *
+ * Actual stats:
+ * The global osmo_stat_item_groups llist contains all group instances, each points at a group description.
+ * This list mixes all types of groups in a single llist, where each instance points at its group desc and has an index.
+ * There are one or more instances of each group, each storing stats for a distinct object (for example, one description
+ * for a BTS group, and any number of BTS instances with independent stats). A group is identified by a group index nr
+ * and possibly also a given name for that particular index (e.g. in osmo-mgw, a group instance is named
+ * "virtual-trunk-0" and can be looked up by that name instead of its more or less arbitrary group index number).
+ *
+ * Each group instance contains one osmo_stat_item instance per global stat item description.
+ * Each osmo_stat_item keeps track of the values for the current reporting period (min, last, max, sum, n),
+ * and also stores the set of values reported at the end of the previous reporting period.
+ *
+ *  const osmo_stat_item_group_desc foo
+ *                                   +-- group_name_prefix = "foo"
+ *                                   +-- item_desc[] (array of osmo_stat_item_desc)
+ *                                        +-- osmo_stat_item_desc bar
+ *                                        |    +-- name = "bar"
+ *                                        |    +-- description
+ *                                        |    +-- unit
+ *                                        |    +-- default_value
+ *                                        |
+ *                                        +-- osmo_stat_item_desc: baz
+ *                                             +-- ...
+ *
+ *  const osmo_stat_item_group_desc moo
+ *                                   +-- group_name_prefix = "moo"
+ *                                   +-- item_desc[]
+ *                                        +-- osmo_stat_item_desc goo
+ *                                        |    +-- name = "goo"
+ *                                        |    +-- description
+ *                                        |    +-- unit
+ *                                        |    +-- default_value
+ *                                        |
+ *                                        +-- osmo_stat_item_desc: loo
+ *                                             +-- ...
+ *
+ *  osmo_stat_item_groups (llist of osmo_stat_item_group)
+ *   |
+ *   +-- group: foo[0]
+ *   |    +-- desc --> osmo_stat_item_group_desc foo
+ *   |    +-- idx = 0
+ *   |    +-- name = NULL (no given name for this group instance)
+ *   |    +-- items[]
+ *   |         |
+ *   |        [0] --> osmo_stat_item instance for "bar"
+ *   |         |       +-- desc --> osmo_stat_item_desc bar (see above)
+ *   |         |       +-- value.{min, last, max, n, sum}
+ *   |         |       +-- reported.{min, last, max, n, sum}
+ *   |         |
+ *   |        [1] --> osmo_stat_item instance for "baz"
+ *   |         |       +-- desc --> osmo_stat_item_desc baz
+ *   |         |       +-- value.{min, last, max, n, sum}
+ *   |         |       +-- reported.{min, last, max, n, sum}
+ *   |         .
+ *   |         :
+ *   |
+ *   +-- group: foo[1]
+ *   |    +-- desc --> osmo_stat_item_group_desc foo
+ *   |    +-- idx = 1
+ *   |    +-- name = "special-foo" (instance can be looked up by this index-name)
+ *   |    +-- items[]
+ *   |         |
+ *   |        [0] --> osmo_stat_item instance for "bar"
+ *   |         |       +-- desc --> osmo_stat_item_desc bar
+ *   |         |       +-- value.{min, last, max, n, sum}
+ *   |         |       +-- reported.{min, last, max, n, sum}
+ *   |         |
+ *   |        [1] --> osmo_stat_item instance for "baz"
+ *   |         |       +-- desc --> osmo_stat_item_desc baz
+ *   |         |       +-- value.{min, last, max, n, sum}
+ *   |         |       +-- reported.{min, last, max, n, sum}
+ *   |         .
+ *   |         :
+ *   |
+ *   +-- group: moo[0]
+ *   |    +-- desc --> osmo_stat_item_group_desc moo
+ *   |    +-- idx = 0
+ *   |    +-- name = NULL
+ *   |    +-- items[]
+ *   |         |
+ *   |        [0] --> osmo_stat_item instance for "goo"
+ *   |         |       +-- desc --> osmo_stat_item_desc goo
+ *   |         |       +-- value.{min, last, max, n, sum}
+ *   |         |       +-- reported.{min, last, max, n, sum}
+ *   |         |
+ *   |        [1] --> osmo_stat_item instance for "loo"
+ *   |         |       +-- desc --> osmo_stat_item_desc loo
+ *   |         |       +-- value.{min, last, max, n, sum}
+ *   |         |       +-- reported.{min, last, max, n, sum}
+ *   |         .
+ *   |         :
+ *   .
+ *   :
+ *
  */
 
 #include <stdint.h>
@@ -98,9 +183,8 @@
 					    unsigned int idx)
 {
 	unsigned int group_size;
-	unsigned long items_size = 0;
 	unsigned int item_idx;
-	void *items;
+	struct osmo_stat_item *items;
 
 	struct osmo_stat_item_group *group;
 
@@ -117,46 +201,25 @@
 	group->desc = group_desc;
 	group->idx = idx;
 
-	/* Get combined size of all items */
+	items = talloc_array(group, struct osmo_stat_item, group_desc->num_items);
+	OSMO_ASSERT(items);
 	for (item_idx = 0; item_idx < group_desc->num_items; item_idx++) {
-		unsigned int size;
-		size = sizeof(struct osmo_stat_item) +
-			sizeof(struct osmo_stat_item_value) *
-			group_desc->item_desc[item_idx].num_values;
-		/* Align to pointer size */
-		size = (size + sizeof(void *) - 1) & ~(sizeof(void *) - 1);
-
-		/* Store offsets into the item array */
-		group->items[item_idx] = (void *)items_size;
-
-		items_size += size;
-	}
-
-	items = talloc_zero_size(group, items_size);
-	if (!items) {
-		talloc_free(group);
-		return NULL;
-	}
-
-	/* Update item pointers */
-	for (item_idx = 0; item_idx < group_desc->num_items; item_idx++) {
-		struct osmo_stat_item *item = (struct osmo_stat_item *)
-			((uint8_t *)items + (unsigned long)group->items[item_idx]);
-		unsigned int i;
-
+		struct osmo_stat_item *item = &items[item_idx];
+		const struct osmo_stat_item_desc *item_desc = &group_desc->item_desc[item_idx];
 		group->items[item_idx] = item;
-		item->last_offs = group_desc->item_desc[item_idx].num_values - 1;
-		item->stats_next_id = 1;
-		item->desc = &group_desc->item_desc[item_idx];
-
-		for (i = 0; i <= item->last_offs; i++) {
-			item->values[i].value = group_desc->item_desc[item_idx].default_value;
-			item->values[i].id = OSMO_STAT_ITEM_NOVALUE_ID;
-		}
+		*item = (struct osmo_stat_item){
+			.desc = item_desc,
+			.value = {
+				.n = 0,
+				.last = item_desc->default_value,
+				.min = item_desc->default_value,
+				.max = item_desc->default_value,
+				.sum = 0,
+			},
+		};
 	}
 
 	llist_add(&group->list, &osmo_stat_item_groups);
-
 	return group;
 }
 
@@ -195,8 +258,7 @@
  */
 void osmo_stat_item_inc(struct osmo_stat_item *item, int32_t value)
 {
-	int32_t oldvalue = item->values[item->last_offs].value;
-	osmo_stat_item_set(item, oldvalue + value);
+	osmo_stat_item_set(item, item->value.last + value);
 }
 
 /*! Descrease the stat_item to the given value.
@@ -207,8 +269,7 @@
  */
 void osmo_stat_item_dec(struct osmo_stat_item *item, int32_t value)
 {
-	int32_t oldvalue = item->values[item->last_offs].value;
-	osmo_stat_item_set(item, oldvalue - value);
+	osmo_stat_item_set(item, item->value.last - value);
 }
 
 /*! Set the a given stat_item to the given value.
@@ -219,16 +280,19 @@
  */
 void osmo_stat_item_set(struct osmo_stat_item *item, int32_t value)
 {
-	int32_t id = item->values[item->last_offs].id + 1;
-	if (id == OSMO_STAT_ITEM_NOVALUE_ID)
-		id++;
-
-	item->last_offs += 1;
-	if (item->last_offs >= item->desc->num_values)
-		item->last_offs = 0;
-
-	item->values[item->last_offs].value = value;
-	item->values[item->last_offs].id    = id;
+	item->value.last = value;
+	if (item->value.n == 0) {
+		/* No values recorded yet, clamp min and max to this first value. */
+		item->value.min = item->value.max = value;
+		/* Overwrite any cruft remaining in value.sum */
+		item->value.sum = value;
+		item->value.n = 1;
+	} else {
+		item->value.min = OSMO_MIN(item->value.min, value);
+		item->value.max = OSMO_MAX(item->value.max, value);
+		item->value.sum += value;
+		item->value.n++;
+	}
 }
 
 /*! Retrieve a stat item struct.
@@ -282,51 +346,34 @@
 int osmo_stat_item_get_next(const struct osmo_stat_item *item, int32_t *next_id,
 	int32_t *value)
 {
-	const struct osmo_stat_item_value *next_value;
-	const struct osmo_stat_item_value *item_value = NULL;
-	int id_delta;
-	int next_offs;
+	if (value)
+		*value = item->value.last;
+	return item->value.n;
+}
 
-	next_offs = item->last_offs;
-	next_value = &item->values[next_offs];
+/*! Indicate that a reporting period has elapsed, and prepare the stat item for a new period of collecting min/max/avg.
+ * \param item  Stat item to flush.
+ */
+void osmo_stat_item_flush(struct osmo_stat_item *item)
+{
+	item->reported = item->value;
 
-	while (next_value->id - *next_id >= 0 &&
-		next_value->id != OSMO_STAT_ITEM_NOVALUE_ID)
-	{
-		item_value = next_value;
+	/* Indicate a new reporting period: no values have been received, but the previous value.last remains unchanged
+	 * for the case that an entire period elapses without a new value appearing. */
+	item->value.n = 0;
+	item->value.sum = 0;
 
-		next_offs -= 1;
-		if (next_offs < 0)
-			next_offs = item->desc->num_values - 1;
-		if (next_offs == item->last_offs)
-			break;
-		next_value = &item->values[next_offs];
-	}
-
-	if (!item_value)
-		/* All items have been read */
-		return 0;
-
-	*value = item_value->value;
-
-	id_delta = item_value->id + 1 - *next_id;
-
-	*next_id = item_value->id + 1;
-
-	if (id_delta > 1) {
-		LOGP(DLSTATS, LOGL_ERROR, "%s: %d stats values skipped\n", item->desc->name, id_delta - 1);
-	}
-
-	return id_delta;
+	/* Also for the case that an entire period elapses without any osmo_stat_item_set(), put the min and max to the
+	 * last value. As soon as one osmo_stat_item_set() occurs, these are both set to the new value (when n is still
+	 * zero from above). */
+	item->value.min = item->value.max = item->value.last;
 }
 
 /*! Skip/discard all values of this item and update \a next_id accordingly */
 int osmo_stat_item_discard(const struct osmo_stat_item *item, int32_t *next_id)
 {
-	int discarded = item->values[item->last_offs].id + 1 - *next_id;
-	*next_id = item->values[item->last_offs].id + 1;
-
-	return discarded;
+	LOGP(DLSTATS, LOGL_ERROR, "osmo_stat_item_discard is deprecated (no-op)\n");
+	return 0;
 }
 
 /*! Skip all values of all items and update \a next_id accordingly */
@@ -453,15 +500,9 @@
  */
 void osmo_stat_item_reset(struct osmo_stat_item *item)
 {
-	unsigned int i;
-
-	item->last_offs = item->desc->num_values - 1;
-	item->stats_next_id = 1;
-
-	for (i = 0; i <= item->last_offs; i++) {
-		item->values[i].value = item->desc->default_value;
-		item->values[i].id = OSMO_STAT_ITEM_NOVALUE_ID;
-	}
+	item->value.sum = 0;
+	item->value.n = 0;
+	item->value.last = item->value.min = item->value.max = item->desc->default_value;
 }
 
 /*! Reset all osmo stat items in a group
diff --git a/src/stats.c b/src/stats.c
index f06515d..f9a3bbc 100644
--- a/src/stats.c
+++ b/src/stats.c
@@ -235,28 +235,11 @@
 	talloc_free(srep);
 }
 
-static int osmo_stats_discard_item(struct osmo_stat_item_group *statg, struct osmo_stat_item *item, void *sctx_)
-{
-	return osmo_stat_item_discard(item, &item->stats_next_id);
-}
-
-static int osmo_stats_discard_group(struct osmo_stat_item_group *statg, void *sctx_)
-{
-	return osmo_stat_item_for_each_item(statg, &osmo_stats_discard_item, NULL);
-}
-
-static int osmo_stats_discard_all()
-{
-	return osmo_stat_item_for_each_group(&osmo_stats_discard_group, NULL);
-}
-
-/*! Initilize the stats reporting module; call this once in your program
+/*! Initialize the stats reporting module; call this once in your program.
  *  \param[in] ctx Talloc context from which stats related memory is allocated */
 void osmo_stats_init(void *ctx)
 {
 	osmo_stats_ctx = ctx;
-	osmo_stats_discard_all();
-
 	is_initialised = 1;
 	start_timer();
 }
@@ -708,43 +691,33 @@
 	struct osmo_stat_item_group *statg, struct osmo_stat_item *item, void *sctx_)
 {
 	struct osmo_stats_reporter *srep;
-	int32_t value;
-	bool have_value;
-
-	have_value = osmo_stat_item_get_next(item, &item->stats_next_id, &value) > 0;
-	if (!have_value) {
-		/* Send the last value in case a flush is requested */
-		value = osmo_stat_item_get_last(item);
-
-		/* Also send it in case a different max value was sent
-		 * previously (OS#5215) */
-		if (!item->stats_last_sent_was_max)
-			have_value = 1;
-	} else {
-		int32_t next_val;
-		/* If we have multiple values only send the max */
-		while (osmo_stat_item_get_next(item, &item->stats_next_id, &next_val) > 0)
-			value = OSMO_MAX(value, next_val);
-	}
-
-	item->stats_last_sent_was_max = (osmo_stat_item_get_last(item) == value);
+	int32_t prev_reported_value = item->reported.max;
+	int32_t new_value = item->value.max;
 
 	llist_for_each_entry(srep, &osmo_stats_reporter_list, list) {
 		if (!srep->running)
 			continue;
 
-		if (!have_value && !srep->force_single_flush)
+		/* If no new stat values have been set in the current reporting period, skip resending the value.
+		 * However, if the previously sent value is not the same as the current value, then do send the changed
+		 * value (while n == 0, the last value from the previous reporting period is in item->value.max == .last
+		 * == .min, which was put in new_value above).
+		 * Also if the stats reporter is set to resend all values, also do resend the current value regardless
+		 * of repetitions.
+		 */
+		if ((!item->value.n && new_value == prev_reported_value)
+		    && !srep->force_single_flush)
 			continue;
 
 		if (!osmo_stats_reporter_check_config(srep,
 				statg->idx, statg->desc->class_id))
 			continue;
 
-		osmo_stats_reporter_send_item(srep, statg,
-			item->desc, value);
-
+		osmo_stats_reporter_send_item(srep, statg, item->desc, new_value);
 	}
 
+	osmo_stat_item_flush(item);
+
 	return 0;
 }
 
@@ -818,7 +791,6 @@
 	osmo_stat_item_for_each_group(osmo_stat_item_group_handler, NULL);
 
 	/* global actions */
-	osmo_stats_discard_all();
 	flush_all_reporters();
 	TRACE(LIBOSMOCORE_STATS_DONE());
 
diff --git a/tests/stats/stats_test.c b/tests/stats/stats_test.c
index ccb3f91..23025d1 100644
--- a/tests/stats/stats_test.c
+++ b/tests/stats/stats_test.c
@@ -88,11 +88,10 @@
 
 	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);
 
@@ -125,124 +124,144 @@
 	OSMO_ASSERT(osmo_stat_item_get("test.one", 0, "item.b") == sitem2);
 	OSMO_ASSERT(osmo_stat_item_get_using_idxname("test.one", "name-for-idx-0", "item.b") == sitem2);
 
+	/* 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_get("test.one", 0, "item.a")) == 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_get("test.one", 0, "item.a")) == 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);
+	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_get("test.one", 0, "item.a")) == 42);
+	OSMO_ASSERT(sitem1->value.max == 42);
+	OSMO_ASSERT(sitem1->value.sum == sum1);
 
-	/* 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);
+	/* Test item flush, reporting period elapsing */
+	osmo_stat_item_flush(osmo_stat_item_get("test.one", 0, "item.a"));
+	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_get("test.one", 0, "item.a")) == 42);
+	OSMO_ASSERT(sitem1->value.max == 42);
+	OSMO_ASSERT(sitem1->value.sum == 0);
 
-	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);
+	/* 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);
 
-		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);
+	/* After a flush, the first item replaces the last, min and max */
+	osmo_stat_item_set(osmo_stat_item_get("test.one", 0, "item.a"), 97);
+	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_get("test.one", 0, "item.a")) == 97);
+	OSMO_ASSERT(sitem1->value.max == 97);
+	OSMO_ASSERT(sitem1->value.sum == 97);
 
-		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);
-	}
+	/* ...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);
 
-	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);
+	/* If an entire reporting period elapses without a new value, the last seen value remains. */
+	osmo_stat_item_flush(osmo_stat_item_get("test.one", 0, "item.a"));
+	osmo_stat_item_flush(osmo_stat_item_get("test.one", 0, "item.a"));
+	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_get("test.one", 0, "item.a")) == 97);
+	OSMO_ASSERT(sitem1->value.max == 97);
+	OSMO_ASSERT(sitem1->value.sum == 0);
 
-	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);
+	/* 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);
 
-	/* 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);
-	}
+	/* Another empty reporting period, everything remained the same. */
+	osmo_stat_item_flush(osmo_stat_item_get("test.one", 0, "item.a"));
+	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_get("test.one", 0, "item.a")) == 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);
 
-	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) */
-	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);
+	/* Test Reset, place back to default value. The previously reported value remains the same. */
+	osmo_stat_item_reset(osmo_stat_item_get("test.one", 0, "item.a"));
+	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_get("test.one", 0, "item.a")) == -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 92d6ce1..a890e0f 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

-- 
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/25464
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I137992a5479fc39bbceb6c6c2af9c227bd33b39b
Gerrit-Change-Number: 25464
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210915/4b7f71c0/attachment.htm>


More information about the gerrit-log mailing list