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.orgneels has submitted this change. ( 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.
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
---
M TODO-RELEASE
M include/osmocom/core/stat_item.h
M src/Makefile.am
M src/stat_item.c
A src/stat_item_internal.h
M src/stats.c
M src/vty/stats_vty.c
M src/vty/utils.c
M tests/Makefile.am
M tests/stats/stats_test.c
M tests/stats/stats_test.err
11 files changed, 364 insertions(+), 328 deletions(-)
Approvals:
laforge: Looks good to me, approved
daniel: Looks good to me, but someone else must approve
pespin: Looks good to me, but someone else must approve
Jenkins Builder: Verified
diff --git a/TODO-RELEASE b/TODO-RELEASE
index c8966cd..0f202e3 100644
--- a/TODO-RELEASE
+++ b/TODO-RELEASE
@@ -15,4 +15,8 @@
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 breakage: remove members stats_next_id, last_offs and values[], no users should exist.
+libosmocore osmo_stat_item API breakage: remove functions osmo_stat_item_get_next(), osmo_stat_item_discard(), osmo_stat_item_discard_all(), no users should exist.
+libosmocore osmo_stat_item_value API breakage: struct definition removed, because no known users exist / no users should exist.
+libosmocore osmo_stat_item ABI breakage: struct osmo_stat_item made opaque.
+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 e600ecd..7f6857c 100644
--- a/include/osmocom/core/stat_item.h
+++ b/include/osmocom/core/stat_item.h
@@ -14,36 +14,16 @@
#define OSMO_STAT_ITEM_NOVALUE_ID 0
#define OSMO_STAT_ITEM_NO_UNIT NULL
-/*! Individual entry in value FIFO */
-struct osmo_stat_item_value {
- int32_t id; /*!< identifier of value */
- int32_t value; /*!< actual value */
-};
-
-/*! 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];
-};
+/*! data we keep for each actual item. Access via API functions only.
+ * (This struct was made opaque after libosmocore 1.5.1)*/
+struct osmo_stat_item;
/*! Statistics item description */
struct osmo_stat_item_desc {
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 */
};
@@ -102,15 +82,9 @@
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);
+int32_t osmo_stat_item_get_last(const struct osmo_stat_item *item);
-/*! Get the last (freshest) value */
-static int32_t osmo_stat_item_get_last(const 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_all(int32_t *next_id)
- OSMO_DEPRECATED("Use osmo_stat_item_discard with item-specific next_id instead");
+void osmo_stat_item_flush(struct osmo_stat_item *item);
typedef int (*osmo_stat_item_handler_t)(
struct osmo_stat_item_group *, struct osmo_stat_item *, void *);
@@ -122,12 +96,20 @@
int osmo_stat_item_for_each_group(osmo_stat_item_group_handler_t handle_group, void *data);
-static inline int32_t osmo_stat_item_get_last(const struct osmo_stat_item *item)
-{
- return item->values[item->last_offs].value;
-}
-
void osmo_stat_item_reset(struct osmo_stat_item *item);
void osmo_stat_item_group_reset(struct osmo_stat_item_group *statg);
+const struct osmo_stat_item_desc *osmo_stat_item_get_desc(struct osmo_stat_item *item);
+
+/* DEPRECATION: up until libosmocore 1.5.1, this API also defined
+ * - struct osmo_stat_item_value
+ * - osmo_stat_item_get_next()
+ * - osmo_stat_item_discard()
+ * - osmo_stat_item_discard_all()
+ * Despite our principle of never breaking API compatibility, we have decided to remove these, because there are no
+ * known users. These items were never practically used/usable outside of libosmocore since the generic stats reporter
+ * (stats.c) was introduced.
+ * We also decided to make struct osmo_stat_item opaque to allow future changes of the struct without API breakage.
+ */
+
/*! @} */
diff --git a/src/Makefile.am b/src/Makefile.am
index 3c589e6..328d2c7 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -58,7 +58,13 @@
endif
BUILT_SOURCES = crc8gen.c crc16gen.c crc32gen.c crc64gen.c
-EXTRA_DIST = conv_acc_sse_impl.h conv_acc_neon_impl.h crcXXgen.c.tpl
+
+EXTRA_DIST = \
+ conv_acc_sse_impl.h \
+ conv_acc_neon_impl.h \
+ crcXXgen.c.tpl \
+ stat_item_internal.h \
+ $(NULL)
libosmocore_la_LDFLAGS = -version-info $(LIBVERSION) -no-undefined
diff --git a/src/stat_item.c b/src/stat_item.c
index 1788746..59f5935 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>
@@ -80,6 +165,8 @@
#include <osmocom/core/timer.h>
#include <osmocom/core/stat_item.h>
+#include <stat_item_internal.h>
+
/*! global list of stat_item groups */
static LLIST_HEAD(osmo_stat_item_groups);
@@ -98,9 +185,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 +203,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 +260,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 +271,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,89 +282,37 @@
*/
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 the next value from the osmo_stat_item object.
- * If a new value has been set, it is returned. The next_id is used to decide
- * which value to return.
- * On success, *next_id is updated to refer to the next unread value. If
- * values have been missed due to FIFO overflow, *next_id is incremented by
- * (1 + num_lost).
- * This way, the osmo_stat_item object can be kept stateless from the reader's
- * perspective and therefore be used by several backends simultaneously.
- *
- * \param val the osmo_stat_item object
- * \param next_id identifies the next value to be read
- * \param value a pointer to store the value
- * \returns the increment of the index (0: no value has been read,
- * 1: one value has been taken,
- * (1+n): n values have been skipped, one has been taken)
+/*! 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.
*/
-int osmo_stat_item_get_next(const struct osmo_stat_item *item, int32_t *next_id,
- int32_t *value)
+void osmo_stat_item_flush(struct osmo_stat_item *item)
{
- const struct osmo_stat_item_value *next_value;
- const struct osmo_stat_item_value *item_value = NULL;
- int id_delta;
- int next_offs;
+ item->reported = item->value;
- next_offs = item->last_offs;
- next_value = &item->values[next_offs];
+ /* 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;
- while (next_value->id - *next_id >= 0 &&
- next_value->id != OSMO_STAT_ITEM_NOVALUE_ID)
- {
- item_value = next_value;
-
- 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;
-}
-
-/*! 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;
-}
-
-/*! Skip all values of all items and update \a next_id accordingly */
-int osmo_stat_item_discard_all(int32_t *next_id)
-{
- LOGP(DLSTATS, LOGL_ERROR, "osmo_stat_item_discard_all is deprecated (no-op)\n");
- return 0;
+ /* 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;
}
/*! Initialize the stat item module. Call this once from your program.
@@ -415,21 +426,20 @@
return rc;
}
+/*! Get the last (freshest) value. */
+int32_t osmo_stat_item_get_last(const struct osmo_stat_item *item)
+{
+ return item->value.last;
+}
/*! Remove all values of a stat item
* \param[in] item stat item to reset
*/
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
@@ -444,4 +454,11 @@
osmo_stat_item_reset(item);
}
}
+
+/*! Return the description for an osmo_stat_item. */
+const struct osmo_stat_item_desc *osmo_stat_item_get_desc(struct osmo_stat_item *item)
+{
+ return item->desc;
+}
+
/*! @} */
diff --git a/src/stat_item_internal.h b/src/stat_item_internal.h
new file mode 100644
index 0000000..9ede8c4
--- /dev/null
+++ b/src/stat_item_internal.h
@@ -0,0 +1,35 @@
+/*! \file stat_item_internal.h
+ * internal definitions for the osmo_stat_item API */
+#pragma once
+
+/*! \addtogroup osmo_stat_item
+ * @{
+ */
+
+struct osmo_stat_item_period {
+ /*! Number of osmo_stat_item_set() that occurred 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;
+
+ /*! 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;
+};
+
+/*! @} */
diff --git a/src/stats.c b/src/stats.c
index f06515d..5dac242 100644
--- a/src/stats.c
+++ b/src/stats.c
@@ -101,6 +101,8 @@
#define TRACE_ENABLED(probe) (0)
#endif /* HAVE_SYSTEMTAP */
+#include <stat_item_internal.h>
+
#define STATS_DEFAULT_INTERVAL 5 /* secs */
#define STATS_DEFAULT_BUFLEN 256
@@ -235,28 +237,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 +693,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 +793,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/src/vty/stats_vty.c b/src/vty/stats_vty.c
index c9ae0fb..17e7190 100644
--- a/src/vty/stats_vty.c
+++ b/src/vty/stats_vty.c
@@ -475,10 +475,11 @@
{
struct vty *vty = sctx_;
- char *name = osmo_asciidoc_escape(item->desc->name);
- char *description = osmo_asciidoc_escape(item->desc->description);
+ const struct osmo_stat_item_desc *desc = osmo_stat_item_get_desc(item);
+ char *name = osmo_asciidoc_escape(desc->name);
+ char *description = osmo_asciidoc_escape(desc->description);
char *group_name_prefix = osmo_asciidoc_escape(statg->desc->group_name_prefix);
- char *unit = osmo_asciidoc_escape(item->desc->unit);
+ char *unit = osmo_asciidoc_escape(desc->unit);
/* | Name | Reference | Description | Unit | */
vty_out(vty, "| %s | <<%s_%s>> | %s | %s%s",
diff --git a/src/vty/utils.c b/src/vty/utils.c
index 1137f2b..4501c66 100644
--- a/src/vty/utils.c
+++ b/src/vty/utils.c
@@ -247,12 +247,11 @@
{
struct vty_out_context *vctx = vctx_;
struct vty *vty = vctx->vty;
- const char *unit =
- item->desc->unit != OSMO_STAT_ITEM_NO_UNIT ?
- item->desc->unit : "";
+ const struct osmo_stat_item_desc *desc = osmo_stat_item_get_desc(item);
+ const char *unit = (desc->unit != OSMO_STAT_ITEM_NO_UNIT) ? desc->unit : "";
vty_out(vty, " %s%s: %8" PRIi32 " %s%s",
- vctx->prefix, item->desc->description,
+ vctx->prefix, desc->description,
osmo_stat_item_get_last(item),
unit, VTY_NEWLINE);
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 22591fb..0880561 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -87,6 +87,7 @@
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 9489e60..dea2bf7 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 @@
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 @@
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);
+ 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);
- /* 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_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);
- 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);
-
- 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) */
+ /* 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);
+ 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);
- rc = osmo_stat_item_discard(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a);
- OSMO_ASSERT(rc == 0);
+ /* ...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 == 0);
+ /* 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);
- 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);
+ /* 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);
- rc = osmo_stat_item_get_next(osmo_stat_item_group_get_item(statg, TEST_A_ITEM), &next_id_a, &value);
- OSMO_ASSERT(rc == 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 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: 7
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy at sysmocom.de>
Gerrit-CC: osmith <osmith at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210930/912fd7af/attachment.htm>