Attention is currently required from: neels.
iedemam 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 19:

(12 comments)

Patchset:

PS18:
> Hi Neels, […]
For now I'm using deciseconds (not decaseconds as in my typo above). I'd like 
to have the underlying stat values be milliseconds for clarity but if this 
isn't possible to have independent timer frequency and increment values, it's 
not a deal-breaker.


Patchset:

PS19:
Hi all,

I think all feedback has now been addressed. I still want to do additional 
testing now that we're really close so please do not merge, even if everything 
looks good.

Thanks,
-Michael


File include/osmocom/bsc/bts.h:

https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/e2f91611_256e6d9e
PS18, Line 61:  BTS_CTR_CHAN_SDCCH_ACTIVE_MILLISECONDS_TOTAL,
> (1)
Done


File include/osmocom/bsc/gsm_data.h:

https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/61943723_41ef4818
PS18, Line 890:         /* Interval timing to capture duration per activation 
and cummulative active time */
> "cumulative"
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/15f55d51_92d8594f
PS18, Line 891:         struct osmo_time_cc active_ms;
> (1) […]
Done


File src/osmo-bsc/bts.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/134c40f4_93716e8e
PS18, Line 1038:                  "Cumulative number of milliseconds of SDCCH 
channel activity" },
> (1)
Done


File src/osmo-bsc/bts_trx_vty.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/31ba6abc_315990ef
PS18, Line 576:                         VTY_NEWLINE);
> (3) […]
This specific VTY output is the primary motivation for this entire patch. I've 
re-added the osmo_time_cc_reset() so each lchan activation can be tracked and 
output.

I started with option b but you argued against it so I refactored everything to 
only use a single osmo_time_cc. I agree this is cleaner and now believe it 
matches my functional goal and your implementation approach.


File src/osmo-bsc/bts_vty.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/1959db04_7d21fd8b
PS18, Line 3844:        vty_out(vty, "%s", VTY_NEWLINE);
> (2) […]
Agreed. I've opted to show total activations and then avg lifespan over past 
hour.


File src/osmo-bsc/lchan_fsm.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/d6bb8d10_2c515814
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 ab […]
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/a09ec898_31d87a43
PS18, Line 224:         if (lchan->active_ms.cfg.rate_ctr) {
> (could just set the flag no matter if rate_ctr is set or not. […]
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/56f4b207_a0e53313 
PS18, Line 546:                 .active_ms = lchan->active_ms
> please add a trailing comma. […]
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/27081/comment/e385d5ae_3588b8d7
PS18, Line 556:         if (lchan->active_ms.cfg.rate_ctr) {
> (could just set the flag no matter if rate_ctr is set or not. […]
Done



--
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: 19
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: neels <[email protected]>
Gerrit-Comment-Date: Wed, 06 Apr 2022 12:59:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: iedemam <[email protected]>
Comment-In-Reply-To: neels <[email protected]>
Gerrit-MessageType: comment

Reply via email to