Attention is currently required from: neels, laforge, pespin. iedemam has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/27081 )
Change subject: WIP: New stats for lchan life duration. ......................................................................
Patch Set 5:
(7 comments)
Patchset:
PS5: Hi all,
Thanks for the reviews so far. Latest patchset uploaded which addresses some concerns.
I think our main disagreement is that of precision. The best of both worlds would be for me to store cumulative active time as seconds instead of milliseconds while still being able to show individual channel ages in milliseconds. This would require carrying remainders as state between activations which I think is ugly.
Perhaps it is indeed enough to only track seconds. In the grand scheme of using this value to extrapolate Erlangs per site, it is still quite precise.
Regards, -Michael
File src/osmo-bsc/bts_trx_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/d981f7cc_ded2752b PS4, Line 575: vty_out(vty, " Activated %llu ms ago%s", duration_ms, VTY_NEWLINE);
Probably more interesting to print it in VTY as seconds? Doesn't look like we can glance millisecond […]
SDCCH channels might not ever reach a full second. This was my thinking in using millisecond precision in the VTY.
File src/osmo-bsc/bts_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/88700fb3_6d23878f PS4, Line 3830: vty_out(vty, " Channel Activations : %"PRIu64" TCH", activations_tch);
I'd rather put it: […]
You don't want to show total activations? Same comment as above about ms precision being needed to display SDCCH info in a meaningful way. I could add logic to simply show "<1s" for them. Sounds clean as well.
File src/osmo-bsc/gsm_data.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/9ad59d86_98481bf2 PS4, Line 349: long long
wondering if a signed data type makes sense here, but this is not critical.
true...everything is now unsigned long long
File src/osmo-bsc/lchan_fsm.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/ad77c3bf_d66974a4 PS4, Line 1812: long long duration_ms = gsm_lchan_active_duration_ms(lchan);
Shouldn't this be an unsigned long long everywhere? Maybe simply use time_t?
true...everything is now unsigned long long
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/d9f07147_63e75399 PS4, Line 1814: if (lchan->type == GSM_LCHAN_TCH_F || lchan->type == GSM_LCHAN_TCH_H) {
I'd go for a switch statement here, but not critical.
done
https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/a10937cb_11d0cbc0 PS4, Line 1816: rate_ctr_add(rate_ctr_group_get_ctr(bts_ctrs, BTS_CTR_LCHAN_TCH_TOTAL_ACTIVE_MILLISECONDS), duration_ms);
my gut feeling also goes more towards a stat_item, but I didn't spend a lot of time contemplating it […]
Agreed. Converted to stat_item.