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: (7 comments) File config/sys.config: https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/0c8815a5_9177… : PS8, Line 24: %% {enft_kpi_enable, true}, %% whether to enable the NFT KPI module (default: false) where does the "e" from "enft" come from? At first sight looks like "nft" would be much more comprehensive for users. File include/s1gw_metrics.hrl: https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/2ede2696_17ee… : PS8, Line 40: %% NOTE: these counters shall not be listed in ?S1GW_COUNTERS, So simply this can be announced as "Per-eNB counters"? File rebar.config: https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/f9b9d83c_c1ed… : PS8, Line 13: {git, "https://gitea.osmocom.org/vyanitskiy/enftables.git", {branch, "fixeria/json"}}}, can we have this in a "osmocom/master" or "master" branch instead? File src/enft_kpi.erl: https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/56cb0edb_175c… : PS8, Line 142: R1 = [nft_expr_match_ip_proto("udp", ?OP_NEQ), nft_expr_accept()], sounds like you may want to introduce atoms in the nft lib to express OP_NEQ and OP_EQ instead. https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/7cb1115d_f696… : PS8, Line 145: nft_cmd_add_chain(TName, "gtpu-ul", "prerouting"), may make sense to have this strings as defines, to avoid potential typos. https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/cd3ecb1f_23cf… : PS8, Line 341: case enb_add_nft_counters(ES0#{addr => Addr}, Cfg) of fyi this can take some time, and afaict you are doing it synchronously (handle_call), which means the caller (probably handling SCTP traffic) will become stuck for a while waiting for this. It's fine for now but can become a problem later. Also it's fine if the the caller of this call is per-session I guess. File src/osmo_s1gw_sup.erl: https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/486338f6_4571… : PS8, Line 81: EnftKpi = {enft_kpi, {enft_kpi, start_link, [enft_kpi_cfg()]}, I?m still wondering why this is called "enft" but above it's called "Pfcp" and not "Epfcp" or "Sctp" vs "Esctp". -- 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 <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Wed, 28 May 2025 11:44:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No