Change in libosmocore[master]: stats: send real last value if no new values come

osmith gerrit-no-reply at lists.osmocom.org
Fri Aug 20 14:04:55 UTC 2021


osmith has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/25206 )

Change subject: stats: send real last value if no new values come
......................................................................

stats: send real last value if no new values come

Background:
* Individual values can be added to osmo_stat_item.values at any time.
* Stats are reported at a fixed interval (see vty 'stats interval'),
  e.g. every 10 seconds.
* In order to report a new stat value, we use the maximum of all
  osmo_stat_item.values added since the last report.
* By default, we do not send new stat values if they did not change
  (see vty 'config-stats' -> 'flush-period' default of 0).

Fix the following bug:
* If 'flush-period' is 0, and no new osmo_stat_item.values are coming
  in, the last value that gets reported is not necessarily the last
  entry in osmo_stat_item.values.
* For attached reporters (statsd), it could then be that the given stat
  stays at the wrong value for a long stretch of time (think of several
  hours/days/forever).

Explanation of how the test shows that it is fixed:
* stats get reported (value is irrelevant)
* osmo_stat_item gets a new value: 20
* osmo_stat_item gets a new value: 10
* stats get reported (value: 20, the maximum of both new values)
* osmo_stat_item gets no new values
* stats get reported (value: 10, this is new because of the bug fix,
  the real last value in osmo_stat_item, different from the 20 sent
  earlier, without the fix it would not send anything here and the last
  sent value would be 20)
* osmo_stat_item gets no new values
* stats get reported (nothing gets sent, since the real last value was
  already sent and 'flush-period' is 0)

Fixes: OS#5215
Change-Id: Ibeefd0e3d1dbe4be454ff05a21df4848b2abfabe
---
M TODO-RELEASE
M include/osmocom/core/stat_item.h
M src/stats.c
M tests/stats/stats_test.c
M tests/stats/stats_test.err
5 files changed, 15 insertions(+), 1 deletion(-)

Approvals:
  neels: Looks good to me, approved
  daniel: Looks good to me, but someone else must approve
  Jenkins Builder: Verified



diff --git a/TODO-RELEASE b/TODO-RELEASE
index 260e1d4..c8966cd 100644
--- a/TODO-RELEASE
+++ b/TODO-RELEASE
@@ -15,3 +15,4 @@
 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
diff --git a/include/osmocom/core/stat_item.h b/include/osmocom/core/stat_item.h
index fbe0433..332bbe9 100644
--- a/include/osmocom/core/stat_item.h
+++ b/include/osmocom/core/stat_item.h
@@ -28,6 +28,10 @@
 	 * 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 */
diff --git a/src/stats.c b/src/stats.c
index 411ecff..f06515d 100644
--- a/src/stats.c
+++ b/src/stats.c
@@ -715,6 +715,11 @@
 	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 */
@@ -722,6 +727,8 @@
 			value = OSMO_MAX(value, next_val);
 	}
 
+	item->stats_last_sent_was_max = (osmo_stat_item_get_last(item) == value);
+
 	llist_for_each_entry(srep, &osmo_stats_reporter_list, list) {
 		if (!srep->running)
 			continue;
diff --git a/tests/stats/stats_test.c b/tests/stats/stats_test.c
index 6505e66..2f2c6ec 100644
--- a/tests/stats/stats_test.c
+++ b/tests/stats/stats_test.c
@@ -445,7 +445,7 @@
 	fprintf(stderr, "report (group 1, item 1 no update, send last item (!= last max), OS#5215):\n");
 	send_count = 0;
 	osmo_stats_report();
-	OSMO_ASSERT(send_count == 0); /* BUG: should be 2! */
+	OSMO_ASSERT(send_count == 2);
 
 	fprintf(stderr, "report (group 1, item 1 no update, nothing to send):\n");
 	send_count = 0;
diff --git a/tests/stats/stats_test.err b/tests/stats/stats_test.err
index 08c2cbc..daa3e5c 100644
--- a/tests/stats/stats_test.err
+++ b/tests/stats/stats_test.err
@@ -116,6 +116,8 @@
   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 (group 1, item 1 no update, send last item (!= last max), OS#5215):
+  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 no update, nothing to send):
 report (remove statg1, ctrg1):
   test2: counter p= g=ctr-test:one_dot i=3 n=ctr:a v=0 d=0

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

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Ibeefd0e3d1dbe4be454ff05a21df4848b2abfabe
Gerrit-Change-Number: 25206
Gerrit-PatchSet: 5
Gerrit-Owner: osmith <osmith at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann at sysmocom.de>
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210820/a3da2501/attachment.htm>


More information about the gerrit-log mailing list