Attention is currently required from: iedemam, fixeria, pespin.
neels has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-bsc/+/27081
)
Change subject: stats: new trackers for lchan life duration
......................................................................
Patch Set 18:
(11 comments)
Patchset:
PS18:
looks like we will merge this soon. Let's sort out these remaining issues:
* milliseconds vs tenth seconds, various places marked (1):
The time_cc updates the rate_ctr every tenth of a second now, which is nice, thumbs up.
However, the naming and doc still suggest that the rate_ctrs are counting milliseconds.
* reporting on vty the average activity duration, see the place marked (2)
* reporting on vty how long a single active lchan has been active, marked (3)
* minor style
File include/osmocom/bsc/bts.h:
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/bd885a57_1fdc10fe
PS18, Line 61: BTS_CTR_CHAN_SDCCH_ACTIVE_MILLISECONDS_TOTAL,
(1)
File include/osmocom/bsc/gsm_data.h:
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/e33c5ad2_cfe40ef9
PS18, Line 890: /* Interval timing to capture duration per activation and cummulative
active time */
"cumulative"
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/13a6d2ba_5633c0e8
PS18, Line 891: struct osmo_time_cc active_ms;
(1)
I think it was named active_ms due to my code review ... maybe just 'active' or
'active_cc' is better after all? sorry about that.
File src/osmo-bsc/bts.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/038dc187_5ac51d5a
PS18, Line 1038: "Cumulative number of milliseconds of SDCCH channel
activity" },
(1)
File src/osmo-bsc/bts_trx_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/33b5979b_43ba30bf
PS18, Line 576: VTY_NEWLINE);
(3)
Before, I gave code review to remove the osmo_time_cc_reset(), saying it was not
necessary. Which is true, when thinking in terms of rate counters and stats; the normal /
intended way to use osmo_time_cc is to always just carry on. I realize there is a
"but" that I didn't see before: of course without a reset the total_sum
accumulates across all activations of an lchan, and we cannot use it to report how long
ago the currently active lchan has been activated. Sorry for not seeing that before.
ways to resolve: one of
a) drop this vty output and the gsm_lchan_active_duration_ms() function. Then it is fine
to not do osmo_time_cc_reset().
b) use a separate simple timestamp to count a single lchan's active time, orthogonal
to the osmo_time_cc that does the "magic" rate_ctr stuff.
c) re-add the osmo_time_cc_reset() with a comment indicating
"/* This reset allows showing on vty how long ago a single active lchan was
activated. This reset does not affect rate_ctr statistics. */".
The osmo_time_cc still works fine when it is reset at a time when the flag is false.
I think either way it would be nicer to first do this patch about rate_ctr and averages
only, and add the "activated x seconds ago" in a separate patch -- it is a
separate feature. We can then discuss there whether that x should also be exposed to
statistics somehow.
File src/osmo-bsc/bts_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/f2439f5e_3166fad9
PS18, Line 3844: vty_out(vty, "%s", VTY_NEWLINE);
(2)
the usefulness of this output is disputable because it averages over the entire lifetime
of the cell. so if we have a month of normal operation followed by an hour of short-lived
lchans, the avg lifespan reported here does not reflect reality in a useful way.
We can instead (or in addition to above output?) use the rate in the last interval
instead: in rate_ctr->intv, we keep values for the last second, minute, hour and day.
For example:
const struct rate_ctr *active_time_tch = rate_ctr_group_get_ctr(bts->bts_ctrs,
BTS_CTR_CHAN_TCH_ACTIVE_TENTH_SECONDS);
const struct rate_ctr *activations_tch = rate_ctr_group_get_ctr(bts->bts_ctrs,
BTS_CTR_CHAN_ACT_TCH);
vty_out(vty, "avg active duration in the last hour: TCH: %s",
/* active_time_tch counts tenths of seconds, so *100 gives an avg in ms */
osmo_int_to_float_str_c(OTC_SELECT,
100 * active_time_tch->intv[RATE_CTR_INTV_HOUR].rate
/ activations_tch->[RATE_CTR_INTV_HOUR].rate, 3));
Same can be done per minute, per day, and per each lchan type, as you see fit...
File src/osmo-bsc/lchan_fsm.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/6a9972bf_12de536a
PS18, Line 208: struct rate_ctr *rate_ctr;
osmocom style requires that this local variable be defined at the start of the scope, at
line 203 above
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/67698bca_fdd73598
PS18, Line 224: if (lchan->active_ms.cfg.rate_ctr) {
(could just set the flag no matter if rate_ctr is set or not.)
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/d3012b5b_7d4dbfe5
PS18, Line 546: .active_ms = lchan->active_ms
please add a trailing comma. it allows future patches to add a single line, less
merge-conflict prone
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/5c2f549f_fb724a13
PS18, Line 556: if (lchan->active_ms.cfg.rate_ctr) {
(could just set the flag no matter if rate_ctr is set or not.)
--
To view, visit
https://gerrit.osmocom.org/c/osmo-bsc/+/27081
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I1b0670c47cb5e0b7776eda89d1e71545ba0e3347
Gerrit-Change-Number: 27081
Gerrit-PatchSet: 18
Gerrit-Owner: iedemam <michael(a)kapsulate.com>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: iedemam <michael(a)kapsulate.com>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 24 Mar 2022 17:20:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment