Attention is currently required from: fixeria, laforge. pespin has posted comments on this change by fixeria. ( https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281?usp=email )
Change subject: enft_kpi: retrieve per-eNB traffic counters ...................................................................... Patch Set 8: Code-Review+1 (5 comments) File config/sys.config: https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/e0ead3c8_7f749520?usp=email : PS8, Line 24: %% {enft_kpi_enable, true}, %% whether to enable the NFT KPI module (default: false) > It's because I named the library `enftables` (Erlang NIFs for libnftables), > and this `e` prefix made […] IMHO the fact that you are using enftables is totally irrelevant from user point of view. The user really only knows the counters are obtained over nft rules, hence why I consider the "e" in there to just be adding confusion. The PFCP stuff is also coming from a "pfcplib", and yet they are not named "pfcplib_*". Just my 5 cents, do as you like. File include/s1gw_metrics.hrl: https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/f3b92493_bda702e4?usp=email : PS8, Line 40: %% NOTE: these counters shall not be listed in ?S1GW_COUNTERS, > Right. Other counters will be made per-eNB too at some point. […] It's fine having a comment, just though the comment there kind of created an alamr in my head (as in "BE CAREFUL!"), but in the end it's just that this is a per-enb counter.. File rebar.config: https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/1e31e8ca_e2fcb5b8?usp=email : PS8, Line 13: {git, "https://gitea.osmocom.org/vyanitskiy/enftables.git";, {branch, "fixeria/json"}}}, > The whole repository currently lives in my personal Gitea profile and will be > migrated to https://gi […] IMHO if this is a repo which is not meant to end up in another namespace, it should be already moved wherever needed before being used in this project. File src/enft_kpi.erl: https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/6a47556a_03f98c8d?usp=email : PS8, Line 142: R1 = [nft_expr_match_ip_proto("udp", ?OP_NEQ), nft_expr_accept()], > Actually, a lot of this stuff can be moved to `enftables`. But for now it can > live here. Acknowledged https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/accc2b7d_cb4776a0?usp=email : PS8, Line 341: case enb_add_nft_counters(ES0#{addr => Addr}, Cfg) of > Good point. I'll migrate this to use `cast` instead of `call`. Be careful though because that may need some work/complexity as it becomes async. I'm fine with merging this as is now, just wanted you to be aware of it and make sure it gets fixed at some point. -- To view, visit https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: erlang/osmo-s1gw Gerrit-Branch: master Gerrit-Change-Id: I498d2854447a2d53d2abddd38652f3e2bbb1fbdd Gerrit-Change-Number: 40281 Gerrit-PatchSet: 8 Gerrit-Owner: fixeria <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: fixeria <[email protected]> Gerrit-Comment-Date: Wed, 28 May 2025 12:19:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: fixeria <[email protected]> Comment-In-Reply-To: pespin <[email protected]>
