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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: neels <[email protected]> Gerrit-CC: fixeria <[email protected]> Gerrit-CC: pespin <[email protected]> Gerrit-Attention: iedemam <[email protected]> Gerrit-Attention: fixeria <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Comment-Date: Thu, 24 Mar 2022 17:20:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
