feat: count bytes per cache status

[yaoge123]

Problem

VTS reports cache efficiency only by request count. A cache hit for a 1 MB response and a cache hit for a 1 KB response count the same, so the counters cannot show how much upstream traffic cache statuses actually save.

Fix

Count bytes per cache status, in both directions:

• downstreamOutBytes: bytes sent to clients while the request ended in a given cache status.
• upstreamInBytes: bytes read from the final upstream attempt for a given cache status; hit is normally 0.

The same information is exposed everywhere cache counters already are:

• JSON: serverZones, filterZones, and cacheZones gain parallel downstreamOutBytes / upstreamInBytes objects next to responses, with matching byte keys in overCounts.
• Prometheus: nginx_vts_server_cache_bytes_total{host,status,direction}, nginx_vts_filter_cache_bytes_total{filter,filter_name,status,direction}, nginx_vts_cache_status_bytes_total{cache_zone,status,direction}, with direction = downstream or upstream.
• Variables: $vts_cache_<status>_downstream_out_bytes and $vts_cache_<status>_upstream_in_bytes.
• set_by_filter / limit members: cache<Status>DownstreamOutBytes and cache<Status>UpstreamInBytes; the snake-case names are wired into the central ngx_http_vhost_traffic_status_members[] table so limit_traffic and variables read the same member.

Dependencies and review updates

• Depends on fix: version the dump format before changing node layout #396, which gives the dump a module-owned format version.
• feat: count bytes per cache status #397 changes ngx_http_vhost_traffic_status_node_t, so it bumps NGX_HTTP_VHOST_TRAFFIC_STATUS_DUMP_FORMAT_VERSION to 2.
• Following review 5184798486, the legacy pre-version dump restore is gone. fix: version the dump format before changing node layout #396 now WARN-rejects pre-version dumps; it no longer logs restoring and then lets the timer replace the file. t/048.dump_format.t TEST2 asserts the rejection, and TEST3/TEST4 are adjusted for format version 2.
• add_cc_bytes() is a static inline function, not a bare block macro, with nginx_version < 1011004 keeping upstreamInBytes at 0.
• The cacheZones overCounts argument-order fix is split out to fix: pass cache-zone overCounts arguments in format order #398 (already merged).
• README documents that upstreamInBytes counts only the final upstream attempt, and that replacing the module changes the shared-memory node layout, so this release needs a restart rather than a reload.

Verification

• nginx 1.30.4 (017cf98) with cache: t/021.set_by_filter.t t/025.check_json_schema.t t/042.dump.t t/048.dump_format.t t/049.cache_status_bytes.t -> PASS, Files=5, Tests=87; --without-http-cache build PASS.
• nginx 1.31.4 (39ffdd8) with cache: same focused tests -> PASS, Files=5, Tests=87; --without-http-cache build PASS.

Rebased onto current master c47ddaa with #396 head ffea9ae.

[Copilot]

🔵 Needs a closer look

It changes shared-memory node layout and persistence/restore behavior while adding new metrics across multiple output formats, so it warrants careful final human verification.

Pull request overview

This PR extends the vhost_traffic_status (VTS) module’s cache-status accounting to include byte totals per cache status, enabling cache efficiency analysis by transferred bytes (not just request counts), and updates dump-format versioning to safely accommodate the widened shared-memory node layout.

Changes:

• Add per-cache-status byte counters for downstream (bytes sent to clients) and upstream (bytes read from upstream), surfaced in JSON, Prometheus, and variables/set_by_filter.
• Widen ngx_http_vhost_traffic_status_node_t to store the new counters and include them in overflow (overCounts) handling and display aggregation.
• Version the dump header format and include node_size checks to prevent restoring incompatible dumps after the node layout change.

File summaries

File	Description
t/validate_json_schema.py	Updates schema validation to require/validate new cache-status byte fields.
t/047.cache_status_bytes.t	Adds regression tests for JSON/Prometheus cache-status byte relationships.
t/046.dump_format.t	Adds tests for rejecting legacy/incorrect dump headers (format version + node size).
t/021.set_by_filter.t	Extends set_by_filter tests/log output to include new cache byte accessors.
src/ngx_http_vhost_traffic_status_variables.c	Exposes new $vts_cache_*_{out_bytes,upstream_bytes} variables.
src/ngx_http_vhost_traffic_status_set.c	Adds set_by_filter member access for per-status cache byte fields.
src/ngx_http_vhost_traffic_status_node.h	Extends node structs with per-status downstream/upstream byte counters (+ overflow counters).
src/ngx_http_vhost_traffic_status_node.c	Initializes new fields and updates node accounting to add per-status bytes.
src/ngx_http_vhost_traffic_status_module.h	Adds ngx_http_vhost_traffic_status_add_cc_bytes and integrates new fields into overflow tracking.
src/ngx_http_vhost_traffic_status_dump.h	Defines dump format version and extends header with node_size.
src/ngx_http_vhost_traffic_status_dump.c	Writes/validates new dump header and rejects mismatched version/node size on restore.
src/ngx_http_vhost_traffic_status_display.c	Adjusts sizing heuristics for JSON output buffering given additional counters/labels.
src/ngx_http_vhost_traffic_status_display_prometheus.h	Adds Prometheus metric descriptors and format strings for cache-status bytes.
src/ngx_http_vhost_traffic_status_display_prometheus.c	Emits new cache-status byte metrics and merges per-status byte totals for “sum” nodes.
src/ngx_http_vhost_traffic_status_display_json.h	Extends JSON format macros to include downstreamOutBytes/upstreamInBytes and overCounts keys.
src/ngx_http_vhost_traffic_status_display_json.c	Populates the new JSON fields and aggregates them into module “sum” stats.
README.md	Documents new JSON objects, variables, and set_by_filter members for cache-status bytes.
CHANGELOG.md	Notes the new feature in Unreleased changes.

Review details

• Files reviewed: 18/18 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[u5surf]

Thanks — per-status byte accounting is genuinely useful, and the semantics you describe hold up. u->state is only assigned in ngx_http_upstream_connect() (ngx_http_upstream.c:1569) and a cache hit never connects, so upstreamInBytes.hit really is 0. r->connection->sent is the right source too: ngx_http_set_keepalive() resets c->sent (ngx_http_request.c:3375), so there is no double counting across a keepalive connection, and it matches how stat_out_bytes is already counted.

Both trees compile clean here with -Wall -Wextra, with NGX_HTTP_CACHE on and off. display_node_name_repeat()'s 17 + nsc -> 33 + nsc is right (serverZone 9 + 8 + 16 = 33 is the maximum; cacheZone is 12 + 16 = 28). The multi-line .* patterns in the new test work because Test::Nginx compiles response_body_like with /s (Socket.pm:1731).

Requesting changes on the following.

1. The dump format version is not bumped

Measured sizeof() on a 64-bit build:

                        sizeof(node)   sizeof(dump_header)
origin/master               3512              144
#396                        3512              152
this PR                     3768              152

--without-http-cache:
origin/master               3368              144
#396                        3368              152
this PR                     3368              152

This PR changes the node layout but leaves NGX_HTTP_VHOST_TRAFFIC_STATUS_DUMP_FORMAT_VERSION at 1, so the only thing separating an old dump from a new one is that 3512 happens not to equal 3768. A layout change that preserved the size — a reordering, a same-size field swap — would pass both checks. This is precisely the case #396 added the version field for, so please bump it to 2. It also produces a clearer log line for the operator: "version failed" rather than "node size failed".

One consequence worth stating in the PR: with --without-http-cache the node is unchanged at 3368, so a no-cache dump written under #396 and one written under this PR are byte-identical, and an unconditional bump rejects them unnecessarily. I would still take the unconditional bump — it costs one dump on one upgrade and keeps the rule simple.

Please also merge this together with #396 in a single release. Shipping them separately makes users lose their dump twice.

2. Nothing guards the shared memory zone across a reload

The dump versioning protects the file. It does not protect the zone. Measured offsets:

                offsetof(data)   offsetof(len)   offsetof(stat_upstream)
origin/master        3504             3488               1896
this PR              3760             3744               2152

ngx_http_vhost_traffic_status_init_zone() carries the existing rbtree straight over when octx is set (ngx_http_vhost_traffic_status_module.c:454). Nodes are allocated as offsetof(ngx_rbtree_node_t, color) + offsetof(node, data) + key->len, so a node allocated by the old build has 3504 + key->len bytes after color. Replace the dynamic module .so, reload, and the new build reads and writes those same nodes at the new offsets — len and ignore_status land 256 bytes past what the slab handed out.

I do not think this PR has to solve it, but it must not be discovered in production. Either state in CHANGELOG.md that this version requires a restart rather than a reload, or fold sizeof(ngx_http_vhost_traffic_status_node_t) into the existing ctx->shm->signature mechanism so a mismatched zone is detected instead of reused.

3. The build breaks on nginx < 1.11.4

ngx_http_vhost_traffic_status_add_cc_bytes reads r->upstream->state->bytes_received in both arms of its #if. That field arrived in nginx 1.11.4:

50ba1a2cd Upstream: the $upstream_bytes_received variable.
release-1.11.4~35

The guard is #if !defined(nginx_version) || nginx_version < 1005007, which is the NGX_HTTP_CACHE_REVALIDATED boundary — a different axis entirely. Anything from 1.5.7 up to 1.11.3 takes the #else arm and fails to compile. CI only builds current nginx and freenginx, so this will not be caught there. Please add a nginx_version < 1011004 arm that contributes 0 upstream bytes.

4. The cacheZones overCounts fix is unrelated, and it changes output

Counting conversion specifiers against arguments on origin/master:

origin/master FMT_CACHE specifiers: 24   arguments passed: 30

NGX_HTTP_VHOST_TRAFFIC_STATUS_JSON_FMT_CACHE has ten specifiers after maxIntegerSize, and display_set_cache_node() passes sixteen values. Everything in cacheZones.*.overCounts is shifted: inBytes has been printing stat_request_counter_oc, outBytes has been printing stat_in_bytes_oc, miss has been printing stat_out_bytes_oc, and so on.

This PR removes exactly the six extra arguments (stat_request_counter_oc and stat_1xx_counter_oc through stat_5xx_counter_oc) and the count comes out right. The fix is correct and welcome — but it is an independent bug with nothing to do with #181, and it changes values that existing dashboards are reading. Please split it into its own commit (ideally its own PR, so it can be backported on its own) and give it a Fix entry in CHANGELOG.md.

5. The same quantity has four different names

surface	name
JSON, zones	downstreamOutBytes.miss / upstreamInBytes.miss
JSON, overCounts	missDownstreamOutBytes / missUpstreamInBytes
set_by_filter	cacheMissOutBytes / cacheMissUpstreamBytes
variables	$vts_cache_miss_out_bytes / $vts_cache_miss_upstream_bytes
Prometheus	status="miss", direction="downstream"

OutBytes versus DownstreamOutBytes, and UpstreamBytes versus UpstreamInBytes, are the two that will hurt. Once these ship they cannot be renamed without breaking users, so please settle on one vocabulary first. My suggestion is to keep the direction word everywhere: cacheMissDownstreamOutBytes and $vts_cache_miss_downstream_out_bytes, matching the JSON and the node fields.

6. limit_traffic cannot ask for the new members

set.c and variables.c both gain the sixteen names; ngx_http_vhost_traffic_status_node_member() in node.c does not. So vhost_traffic_status_limit_traffic cacheHitOutBytes 1000 silently does nothing, while set_by_filter and $vts_cache_hit_out_bytes work. Please add them there too, or say in the README that these members are not available to limit_traffic.

Related: this PR adds names in three separate places and then introduces a fourth table, ngx_http_vhost_traffic_status_cache_status_bytes[], for the Prometheus writer only. There is work in flight on master consolidating all three name ladders into a single offset table (ngx_http_vhost_traffic_status_members[]). It would be much less painful to land that first and add sixteen rows here than to merge this and then unpick it.

7. Smaller points

• _oc cost. Half of the 256-byte node growth is the sixteen overflow counters. A 64-bit byte counter is not realistically going to wrap. If you dropped them the node would grow by 128 bytes instead of 256. Keeping them is defensible for consistency with the existing counters — just make it a deliberate choice rather than a default.
• add_cc_bytes is a bare-block macro that declares locals. if (x) ngx_http_vhost_traffic_status_add_cc_bytes(...); else ... will not compile. ngx_http_vhost_traffic_status_add_cc has the same shape, but this one also introduces downstream and upstream into the enclosing scope. do { ... } while (0) costs nothing here.
• u->state->bytes_received is the last upstream attempt only. When proxy_next_upstream retried, the bytes read from the earlier peers are not counted; r->upstream_states holds them all. Worth a sentence in the README next to upstreamInBytes.
• Prometheus line budget. The longest new line, nginx_vts_filter_cache_bytes_total{filter="",filter_name="",status="revalidated",direction="downstream"} <20 digits>, is 126 bytes with the name excluded, against NGX_HTTP_VHOST_TRAFFIC_STATUS_DISPLAY_PROMETHEUS_LINE of 128. It fits, with two bytes to spare. Any longer status name or label would overflow it.
• Section header budget. The _S header macros grow from 3171 to 3516 bytes while NGX_HTTP_VHOST_TRAFFIC_STATUS_DISPLAY_MAIN_LEN stays at 8192, which also has to cover the JSON main, connections and sharedZones text. Still enough headroom, but worth a look.
• Test numbering. t/047.cache_status_bytes.t collides with t/047.member_names_cache.t on a branch heading for master, as t/046 does in #396.
• Test brittleness. Both new tests assert one long .*-chained pattern that depends on the order zones and statuses are emitted in. It passes today; it will fail confusingly the first time anything reorders the output. Separate assertions per metric would be easier to live with.

[yaoge123]

Addressed in ad060e7.

• Dump format version bumped to 2 because node_t layout changes here.
• add_cc_bytes is now a static inline function instead of a bare block macro; nginx < 1.11.4 contributes zero upstream bytes because bytes_received did not exist there.
• The cacheZones overCounts argument-order bug was split out to fix: pass cache-zone overCounts arguments in format order #398.
• Naming is unified around the direction words: JSON downstreamOutBytes / upstreamInBytes, overCounts <status>DownstreamOutBytes / <status>UpstreamInBytes, set_by_filter cache<Status>DownstreamOutBytes / cache<Status>UpstreamInBytes, variables $vts_cache_<status>_downstream_out_bytes / $vts_cache_<status>_upstream_in_bytes, and node_member/limit_traffic snake names.
• node_member() now has the 16 byte members, so limit_traffic can address them.
• CHANGELOG states that the shared-memory node layout changed and replacing the module/binary requires a restart, not a reload.
• README documents that upstreamInBytes counts only the final upstream attempt; earlier proxy_next_upstream attempts are not included.
• The test was renumbered to t/049.cache_status_bytes.t, and the long order-dependent regexes were replaced with separate assertions per JSON field / Prometheus metric. DISPLAY_PROMETHEUS_LINE was raised to 192 for the longer metric lines.

Verified with nginx 1.30.4 and 1.31.4: t/021, t/025, t/042, t/048, t/049 pass, and --without-http-cache builds pass.

[u5surf]

Thanks — everything from the last round is addressed, and I verified each of them rather than taking the list on trust:

• NGX_HTTP_VHOST_TRAFFIC_STATUS_DUMP_FORMAT_VERSION is 2.
• -Wall -Wextra is clean with NGX_HTTP_CACHE on and off; add_cc_bytes is a function now and the nginx_version >= 1011004 arm is there.
• The naming is consistent across all six surfaces (JSON zones, JSON overCounts, set_by_filter, variables, node_member, Prometheus).
• node_member() carries the sixteen byte members, so limit_traffic can address them.
• DISPLAY_PROMETHEUS_LINE at 192 against a longest new line of 126 bytes is comfortable.
• The overCounts fix landed separately as #398, and the format/argument counts balance again.
• CHANGELOG states the restart requirement and README documents the proxy_next_upstream limitation.

Measured on a 64-bit build: sizeof(node) 3512 -> 3768 with cache, 3368 -> 3368 without, header 144 -> 152.

One thing needs to change before this can go in.

The legacy restore path is not safe once the node layout moves

dump_restore() takes the legacy branch on the version alone:

if (file_header.version > 1000000) {
    ngx_log_error(NGX_LOG_NOTICE, ev->log, 0,
                  "dump_restore::dump_header_read() legacy version:%ui, restoring", ...);
    offset = NGX_HTTP_VHOST_TRAFFIC_STATUS_DUMP_LEGACY_HEADER_SIZE;
}

and then reads sizeof(ngx_http_vhost_traffic_status_node_t) per record. A pre-version header is 144 bytes and carries no node_size, so there is nothing in the file to check the record size against — the branch is sound only while the current layout happens to equal the legacy one. That held for the first commit here (3512 -> 3512). The second commit makes it false: with cache the node becomes 3768, and the fields are inserted mid-struct, so everything after the cache block shifts by 256 bytes:

              offsetof(stat_upstream)   offsetof(len)   offsetof(data)
before                 1896                 3488           3504
after                  2152                 3744           3760

I built a real pre-version dump — 144-byte header, 3512-byte records, plausible counter values — and replayed the restore loop against this branch:

legacy dump: header=144, node=3512, 5 records
this build:  header=152, node=3768

iter 1: len=17221764975064776706 > 1024 -> skip ahead
short read at 17221764975064776853 -> break
-> 0 nodes restored in 2 iterations

len is read from offset 3744, which lands inside the following record's counters, so it is always a huge number, the len > BUF_SIZE arm skips past EOF, and the loop ends. Nothing is restored. I ran 12000 variations over different key lengths and record alignments: one of them managed to insert a single node with meaningless counters, the rest restored nothing. So this does not corrupt anything — but it does lose the dump while telling the operator it did not.

That last part is what makes it worse than a plain rejection. dump_update_valid() returns NGX_OK for a legacy header, dump_handler() reads that as "go ahead and rewrite", and dump_execute() opens the file with NGX_FILE_TRUNCATE. The timer is armed before dump_restore() runs (ngx_http_vhost_traffic_status_module.c:1194-1197), so about a second after start the pre-version dump is gone. The only line at the default log level is the NOTICE saying restoring; every failure below it is ngx_log_debug.

--without-http-cache is unaffected — the node stays 3368 there and the legacy restore works correctly — so the behaviour now depends on the build options.

The test cannot catch this

t/048.dump_format.t TEST 2 builds its fixture by rewriting the header of a dump this build just wrote:

my $data = <$in>;                  # 152-byte header, 3768-byte records
my $legacy  = pack("a128 $uint_fmt $uint_fmt", 'ngx_http_vhost_traffic_status', 0, 1031004);
my $current = pack("a128 $uint_fmt $uint_fmt $uint_fmt", 'x', 0, 0, 0);
substr($data, 0, length($current), $legacy);

The result is a 144-byte header followed by current 3768-byte records, which is a file no released version has ever written, and which matches exactly what the restore code assumes. A genuine pre-version dump has 3512-byte records and is never tried. The comment at the top of the file still states the assumption the second commit invalidates:

Those dumps are still readable: the node layout did not change when the header grew a module format version and node size.

What I would like instead

Please drop the legacy path from this PR and go back to rejecting a pre-version dump with NGX_LOG_WARN, as the first commit originally did.

Rejecting is not a regression. The node layout has already changed three times since vhost_traffic_status_dump shipped, and master has never checked anything:

v0.1.15  2017-06-20  node=2408   dump feature ships
v0.1.17  2018-06-20  node=3480   layout changed
v0.2.2   2023-05-26  node=3488   layout changed
v0.2.5   2025-12-28  node=3512   layout changed
v0.2.7   2026-08-09  node=3512

Feeding a v0.2.4 dump to v0.2.7 today gives the same two iterations and the same empty result, with no log line at all, because dump_restore() never reads the version field and every framing failure is ngx_log_debug. So users already lose the dump across a layout change; this PR's value is that the loss becomes visible. A WARN is an improvement over today. A NOTICE that says restoring when nothing was restored is not.

There is also no case left for the migration to serve. Both commits ship together, so a cache-enabled deployment goes straight from a pre-version dump to format 2 and the legacy branch can never produce a correct restore. If you want that migration to be worth something, the first commit would have to ship in a release of its own, ahead of this one — which I am happy to consider as a separate PR if you would prefer that.

Either way t/048.dump_format.t TEST 2 should change: back to asserting that a pre-version dump is rejected, or, if the migration stays in some gated form, built from a genuine 3512-byte-record fixture so that it fails on a cache-enabled build today.

Minor

The branch conflicts with master on CHANGELOG.md only, because #398 added a Fix entry under [Unreleased]. A rebase sorts it out; the sources merge cleanly.

[yaoge123]

Addressed in a034dfc, rebased onto the updated #396 (ffea9ae) on master c47ddaa.

• Removed the legacy pre-version restore. fix: version the dump format before changing node layout #396 no longer restores such dumps either; it logs WARN and rejects them. The NOTICE ... restoring followed by the timer replacing the file is gone.
• Because feat: count bytes per cache status #397 changes the cache-enabled node from 3512 to 3768 bytes and moves stat_upstream/len/data, the old 144-byte-header + 3512-byte-record dump cannot be parsed here. The fixture in t/048 TEST2 now asserts rejection; TEST3/TEST4 are adjusted for format version 2.
• Kept the rest of the review fixes: dump format version 2, static inline add_cc_bytes() with the nginx <1.11.4 upstream-bytes guard, the eight cache<Status>DownstreamOutBytes / cache<Status>UpstreamInBytes members in the central member table for limit_traffic, the unified JSON/Prometheus/variables/set_by_filter naming, DISPLAY_PROMETHEUS_LINE 192, README notes for final-upstream-attempt semantics and restart-after-replace, and the CHANGELOG Changed note.
• Resolved the CHANGELOG.md conflict with merged fix: pass cache-zone overCounts arguments in format order #398 by keeping both the Fix entry and this PR's Feat/Changed entries.

Verification: nginx 1.30.4 and 1.31.4 focused tests t/021/t/025/t/042/t/048/t/049 pass (Files=5, Tests=87), and both --without-http-cache builds pass.