metrics, bank, pack, quic: use MACRO_CNT in def #6543
Conversation
Use the MACRO_CNT variables in the definition of the metrics arrays. This is to provide drift in the future which could result in over/ under reads/writes.
intrigus-lgtm
requested review from
nbridge-jump,
ripatel-fd,
akhinvasara-jumptrading,
ptaffet-jump and
mmcgee-jump
as code owners
| ulong txn_load_address_lookup_tables[ 6 ]; | ||
| ulong transaction_result[ 41 ]; | ||
| ulong txn_load_address_lookup_tables[ FD_METRICS_COUNTER_BANK_TRANSACTION_LOAD_ADDRESS_TABLES_CNT ]; | ||
| ulong transaction_result[ FD_METRICS_COUNTER_BANK_TRANSACTION_RESULT_CNT ]; |
There was a problem hiding this comment.
This one is actually TRANSACTION_ERROR_CNT I believe
There was a problem hiding this comment.
Both FD_METRICS_COUNTER_BANK_TRANSACTION_RESULT_CNT and FD_METRICS_ENUM_TRANSACTION_ERROR_CNT map to the correct value, 41.
transaction_result is used in FD_MCNT_ENUM_COPY( BANK, TRANSACTION_RESULT, ctx->metrics.transaction_result ); so FD_METRICS_COUNTER_BANK_TRANSACTION_RESULT_CNT definitely aligns with that.
I do agree that it's confusing to have ..._RESULT as well as ..._ERROR, but that's how it's defined in metrics.xml.
I don't think it would make sense to change the enum name to TransactionResult as that would deviate from agave and clash with TransactionError from bankf tile.
Should I instead rename the field to transaction_err and the counter name in metrics.xml to TransactionError?
| ulong pkt_quic_hdr_err_cnt; /* number of packets dropped due to weird QUIC header */ | ||
| ulong pkt_undersz_cnt; /* number of QUIC packets dropped due to being too small */ | ||
| ulong pkt_oversz_cnt; /* number of QUIC packets dropped due to being too large */ | ||
| ulong pkt_decrypt_fail_cnt[ FD_METRICS_ENUM_QUIC_ENC_LEVEL_CNT ]; /* number of packets that failed decryption due to auth tag */ |
There was a problem hiding this comment.
Number of encryption levels shouldn't come from metrics. Use FD_QUIC_NUM_ENC_LEVELS
There was a problem hiding this comment.
Maybe we should ASSERT somewhere that they are equal
There was a problem hiding this comment.
Actually - FD_METRICS_ENUM_QUIC_ENC_LEVEL_CNT is fine. Ignore me
There was a problem hiding this comment.
i like the idea of adding an ASSERT to check they are equal
would be gr8 to also ASSERT that for FD_METRICS_ENUM_QUIC_CONN_STATE_CNT and FD_QUIC_CONN_STATE_CNT (in fd_quic_conn.h)
Use the MACRO_CNT variables in the definition of the metrics arrays.
This is to avoid drift in the future which could result in over/ under reads/writes.
(Apologies for all the review requests...)