Attention is currently required from: laforge, pespin.

fixeria 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:

(6 comments)

File config/sys.config:

https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/63319ec0_4b19ae23?usp=email
 :
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 compreh […]
It's because I named the library `enftables` (Erlang NIFs for libnftables), and 
this `e` prefix made it here too (the code is using `enftables`, so the module 
name is `enft_kpi` and thus param names).


File include/s1gw_metrics.hrl:

https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/94c79ab8_139fe8c8?usp=email
 :
PS8, Line 40: %% NOTE: these counters shall not be listed in ?S1GW_COUNTERS,
> So simply this can be announced as "Per-eNB counters"?
Right. Other counters will be made per-eNB too at some point.

I am adding a NOTE here because a NOTE below states that new entries need to be 
added to `s1gw_metric:init()` (actually `?S1GW_COUNTERS` or `?S1GW_GAUGES`). I 
don't think it hurts?


File rebar.config:

https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/5e948f09_fd047d24?usp=email
 :
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?
The whole repository currently lives in my personal Gitea profile and will be 
migrated to https://gitea.osmocom.org/erlang/. I'll also merge this branch into 
`master`. I'll do this once the customer completes initial testing.


File src/enft_kpi.erl:

https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/c9c94a34_c914017b?usp=email
 :
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.
Actually, a lot of this stuff can be moved to `enftables`. But for now it can 
live here.


https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/25dbb0bc_d691d93a?usp=email
 :
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 th […]
Good point. I'll migrate this to use `cast` instead of `call`.


File src/osmo_s1gw_sup.erl:

https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40281/comment/67f245fc_decbc974?usp=email
 :
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" […]
See the other comment.



--
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: pespin <[email protected]>
Gerrit-Comment-Date: Wed, 28 May 2025 12:07:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <[email protected]>

Reply via email to