Skip to content

refactor: let a variable read the kind of the member it was given - #400

Merged
u5surf merged 1 commit into
vozlt:masterfrom
u5surf:fix/variables-member-kind
Sep 12, 2026
Merged

u5surf merged 1 commit into
vozlt:masterfrom
u5surf:fix/variables-member-kind

Conversation

@u5surf

@u5surf u5surf commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

What this changes

The members table added in #395 says of every member whether it is a counter
or a queue, and the two are read differently. The variables are handed the
offset and not the kind, so the handler that has to average a queue recognises
itself by comparing its offset against stat_request_times — the only queue a
variable has ever named:

if (data == offsetof(ngx_http_vhost_traffic_status_node_t, stat_request_times)) {

So the knowledge that a field is a queue lives in two places, and the copy in
variables.c names one particular field. This hands the row instead of the
offset and asks it, which is what set.c already does for the same two kinds.

A second commit writes down why responseMsecCounter and responseMsec are
the two members no variable reads, which the table did not say.

This fixes no bug. Nothing behaves differently and nothing new becomes
possible; it is the implicit coupling that goes away. If that is not worth a
change here, I am happy to close it.

What it does not do

I originally expected this to be what stands between the responseMsec row and
a variable. It is not — there are two reasons that row has none, and this
addresses one:

addressed here
1 the handler is given the offset and not the kind, so only stat_request_times is averaged yes
2 a variable is answered from the server zone of the request, and the upstream times are written to the node of the peer, in shm.c. On a server zone they are the zeroes the node was created with no — and it cannot be

Adding vts_response_time to that row on this branch still reports 0 for every
request. set_by_filter can name those members because it is told which zone
to read and can be told an upstream one. The second commit records this at the
rows themselves, so the next person does not have to find it out.

How it was verified

Built from a clean tree on a8e0215, nginx 1.31.6, -Wall -Wextra clean with
NGX_HTTP_CACHE on and off.

t/043.variables.t ........... ok
t/046.member_names.t ........ ok
t/047.member_names_cache.t .. ok
t/021.set_by_filter.t ....... ok
Files=4, Tests=104,  Result: PASS

Behaviour is unchanged by construction, not only by the suite. Comparing the
old test (offset equals stat_request_times) with the new one (kind is
QUEUE) over every row that carries a variable:

18 rows carry a variable; 0 would take a different branch
EQUIVALENT

Of the eighteen, one is a queue and it is vts_request_time.

Separately, I checked what the old code did when the responseMsec row was
given a variable — a 50 ms backend, five proxied requests, then both queue
variables read from the same server zone:

X-Request-Time: 58      $vts_request_time
X-Response-Time: 0      $vts_response_time, added to the responseMsec row

That 0 is reason 2 above, and it is the same on this branch. It is why the
comment is worth more here than the code is.

Checklist

  • Builds with --without-http-cache as well as with the cache enabled.
  • Dump format version — not applicable, ngx_http_vhost_traffic_status_node_t
    is untouched and sizeof() is unchanged.
  • nginx_version guard — not applicable, no nginx field or function is newly used.
  • Format macro arguments — not applicable, no ..._FMT_... macro is touched.
  • A test under t/ — not applicable, there is no behaviour to test. The
    equivalence is argued above instead.
  • README.md / CHANGELOG.md — not applicable, nothing here is visible to users.

Assistance Disclosure

AI used. I asked Claude to make the handler read the kind from the table rather
than infer it from the offset; it wrote the change, built it, ran the tests, and
ran the branch-equivalence check and the two-variable experiment above. I
reviewed the result. The finding that reason 2 exists — and so that this change
does not do what I first thought it would — came out of that experiment rather
than from reading the code.

The table says of every member whether it is a counter or a queue, and the
two are read differently. The variables were handed the offset and not the
kind, so the one that has to average a queue recognised itself by comparing
its offset against stat_request_times - the only queue a variable has ever
named. The knowledge that a field is a queue sat in two places, and the copy
in variables.c named a single field.

Hand the row instead of the offset, and ask it. The offset is still what the
value is read at, so nothing moves: of the eighteen members a variable can
read, one is a queue and it is that one, and every one of the eighteen takes
the branch it took before.

While here, say why responseMsecCounter and responseMsec are the two members
no variable reads, which the table did not. A variable is answered from the
server zone of the request, and the upstream times are written to the node of
the peer that served it, in shm.c. On a server zone they are the zeroes the
node was created with, so a variable named here would report 0 for every
request, for ever, with nothing to say it was wrong - before this change and
after it alike, since a zeroed queue averages to zero. set_by_filter names
them because it is told which zone to read, and can be told an upstream one.
@u5surf
u5surf force-pushed the fix/variables-member-kind branch from a4a9822 to b6b549a Compare September 12, 2026 07:24
@u5surf
u5surf merged commit c47ddaa into vozlt:master Sep 12, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant