feat(stream): per-listen metrics zone and $stream_session_reason - #123
Open
AlinsRan wants to merge 6 commits into
Open
feat(stream): per-listen metrics zone and $stream_session_reason#123AlinsRan wants to merge 6 commits into
AlinsRan wants to merge 6 commits into
Conversation
nginx keeps the stream $status at 200 for every failure that happens after the upstream connection is established, and it exposes no way to read the byte counters of a live session, so a TCP proxy cannot report connection outcomes or real-time throughput. Add ngx_stream_apisix_metrics_module: - apisix_stream_metrics_zone reserves a shared memory zone holding, per stream listening address, the active session count and the bytes moved in the four directions. Each session accumulates locally and merges into the zone at most once per second, plus a final flush, so long-lived connections keep the counters moving without an atomic operation per read or write. - $stream_session_reason reports why a session ended: normal close, client or upstream reset, connect/receive/send timeout, worker shutdown. - resty.apisix.stream.metrics exposes the zone to Lua over FFI. The ngx_stream_proxy_module patch only records what nginx destroys on its own: it clears ev->timedout before finalizing, and it frees u->peer.connection before the log phase runs, which is also why the last byte delta towards the upstream has to be taken in the finalize hook. Everything else, including reset detection, is derived from the connection flags.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds shared-memory stream metrics, Lua metrics access, session termination variables, listening-address reporting, proxy lifecycle hooks for supported Nginx versions, documentation, and integration tests. ChangesStream metrics and session reasons
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant StreamProxy
participant MetricsModule
participant SharedMetricsZone
participant LuaMetrics
StreamProxy->>MetricsModule: update session counters
MetricsModule->>SharedMetricsZone: flush address metrics
StreamProxy->>MetricsModule: finalize session
MetricsModule->>SharedMetricsZone: store final counters
LuaMetrics->>MetricsModule: dump metrics entries
MetricsModule-->>LuaMetrics: return address and counter totals
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Aug 3, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
t/stream/metrics.t (1)
65-75: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the reason for the connect timeout.
TEST 3 checks the 502 status but not
$stream_session_reason.connect_timeoutis the one reason produced by thectx->connect_timeoutplusNGX_STREAM_BAD_GATEWAYpath inngx_stream_apisix_derive_reason, and no test covers it. Add the reason to the log line and to the expected output.💚 Proposed coverage addition
log_by_lua_block { - ngx.log(ngx.WARN, "status: ", ngx.var.status) + ngx.log(ngx.WARN, "status: ", ngx.var.status, + ", reason: ", ngx.var.stream_session_reason) } --- stream_request eval "GET /" --- error_log -status: 502 +status: 502, reason: connect_timeout🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@t/stream/metrics.t` around lines 65 - 75, Update TEST 3’s log_by_lua_block to log both ngx.var.status and ngx.var.stream_session_reason, then extend the expected error_log output to assert the 502 status includes the connect_timeout reason. Keep the existing unreachable-upstream setup and native 502 assertion unchanged.src/stream/ngx_stream_apisix_metrics_module.c (3)
294-299: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPublish the new slot with a write barrier.
On reload the master appends slots while the old workers still read the same zone. A reader can observe the incremented
sh->nusedbefore it observes theaddrbytes of that slot. Addngx_memory_barrier()before the increment so the slot contents are visible first.♻️ Proposed ordering fix
slot->addr_len = (uint32_t) addr->len; ngx_memcpy(slot->addr, addr->data, addr->len); + ngx_memory_barrier(); + sh->nused++;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stream/ngx_stream_apisix_metrics_module.c` around lines 294 - 299, In the slot append logic, add ngx_memory_barrier() after copying the new slot’s address and immediately before incrementing sh->nused, ensuring readers see fully initialized slot contents before the published count increases.
330-332: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the skipped listening addresses.
An address longer than
NGX_STREAM_APISIX_METRICS_ADDR_LENproduces no metrics and no diagnostic. A unix socket path can exceed 128 bytes. Emit a warning, as the adjacent too-small-zone branch does.♻️ Proposed diagnostic
if (addr.len == 0 || addr.len > NGX_STREAM_APISIX_METRICS_ADDR_LEN) { + ngx_log_error(NGX_LOG_WARN, cycle->log, 0, + "apisix stream metrics skips listening address " + "\"%V\": length %uz exceeds %d", + &addr, addr.len, + NGX_STREAM_APISIX_METRICS_ADDR_LEN); continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stream/ngx_stream_apisix_metrics_module.c` around lines 330 - 332, Update the addr validation branch in the stream metrics address-processing logic to emit a warning before continuing when addr.len exceeds NGX_STREAM_APISIX_METRICS_ADDR_LEN, matching the diagnostic behavior of the adjacent too-small-zone branch; preserve the existing handling for zero-length addresses and the continue behavior.
691-709: 🩺 Stability & Availability | 🔵 TrivialThe decrement path is correct. One operational note.
ctx->countedguarantees exactly one decrement per session, and it impliesctx->slotis non-NULL.
activelives in shared memory that survives reloads. If a worker is killed while sessions are live, the log phase never runs for those sessions and the gauge stays inflated for the lifetime of the zone. Consider documenting this, or tracking active counts per worker so a dead worker's contribution can be dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stream/ngx_stream_apisix_metrics_module.c` around lines 691 - 709, Document the shared-memory lifecycle limitation near ngx_stream_apisix_metrics_log_handler: sessions belonging to a worker killed before log phase execution do not decrement slot->active, so the gauge may remain inflated for the lifetime of the zone. Do not alter the existing ctx->counted decrement path.lib/resty/apisix/stream/metrics.lua (1)
26-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the slot cap into the public header.
Line 26 says this value must stay in sync with
ngx_stream_apisix_metrics_module.h, butNGX_STREAM_APISIX_METRICS_MAX_SLOTSis defined inngx_stream_apisix_metrics_module.c. If the C cap grows later,dump()truncates silently. Move the macro next toNGX_STREAM_APISIX_METRICS_ADDR_LENandNGX_STREAM_APISIX_METRICS_DIRECTIONSin the header, so the three constants the Lua side mirrors live in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/resty/apisix/stream/metrics.lua` around lines 26 - 33, Move NGX_STREAM_APISIX_METRICS_MAX_SLOTS from ngx_stream_apisix_metrics_module.c into the public ngx_stream_apisix_metrics_module.h beside NGX_STREAM_APISIX_METRICS_ADDR_LEN and NGX_STREAM_APISIX_METRICS_DIRECTIONS, removing the duplicate C definition. Update the Lua MAX_ENTRIES mirror to use the header’s synchronized value so dump() reflects the shared slot cap.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/stream/ngx_stream_apisix_metrics_module.c`:
- Around line 501-516: Validate the reason argument at the start of
ngx_stream_apisix_set_session_reason, rejecting values outside the valid
NGX_STREAM_APISIX_REASON enum range before storing them in ctx->reason. Preserve
the existing null-context handling and first-terminating-event-wins behavior for
valid reasons.
---
Nitpick comments:
In `@lib/resty/apisix/stream/metrics.lua`:
- Around line 26-33: Move NGX_STREAM_APISIX_METRICS_MAX_SLOTS from
ngx_stream_apisix_metrics_module.c into the public
ngx_stream_apisix_metrics_module.h beside NGX_STREAM_APISIX_METRICS_ADDR_LEN and
NGX_STREAM_APISIX_METRICS_DIRECTIONS, removing the duplicate C definition.
Update the Lua MAX_ENTRIES mirror to use the header’s synchronized value so
dump() reflects the shared slot cap.
In `@src/stream/ngx_stream_apisix_metrics_module.c`:
- Around line 294-299: In the slot append logic, add ngx_memory_barrier() after
copying the new slot’s address and immediately before incrementing sh->nused,
ensuring readers see fully initialized slot contents before the published count
increases.
- Around line 330-332: Update the addr validation branch in the stream metrics
address-processing logic to emit a warning before continuing when addr.len
exceeds NGX_STREAM_APISIX_METRICS_ADDR_LEN, matching the diagnostic behavior of
the adjacent too-small-zone branch; preserve the existing handling for
zero-length addresses and the continue behavior.
- Around line 691-709: Document the shared-memory lifecycle limitation near
ngx_stream_apisix_metrics_log_handler: sessions belonging to a worker killed
before log phase execution do not decrement slot->active, so the gauge may
remain inflated for the lifetime of the zone. Do not alter the existing
ctx->counted decrement path.
In `@t/stream/metrics.t`:
- Around line 65-75: Update TEST 3’s log_by_lua_block to log both ngx.var.status
and ngx.var.stream_session_reason, then extend the expected error_log output to
assert the 502 status includes the connect_timeout reason. Keep the existing
unreachable-upstream setup and native 502 assertion unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de85b043-48d5-45d9-a175-6ec1a9f050eb
📒 Files selected for processing (11)
README.mdlib/resty/apisix/stream/metrics.luapatch/1.21.4/nginx-stream_metrics.patchpatch/1.25.3.1/nginx-stream_metrics.patchpatch/1.27.1.1/nginx-stream_metrics.patchpatch/1.29.2.4/nginx-stream_metrics.patchsrc/stream/configsrc/stream/ngx_stream_apisix_metrics_module.csrc/stream/ngx_stream_apisix_metrics_module.hsrc/stream/ngx_stream_apisix_module.ht/stream/metrics.t
…t correctly
Review follow-ups on the stream metrics module:
- The zone pointer was cached when the zone was created and never cleared. A
reload that removed the directive (or the whole stream block) left it
pointing at shared memory ngx_init_cycle had already unmapped, so the next
worker segfaulted on startup. Resolve it from the module main conf of the
current cycle instead, which is NULL in exactly those cases.
- proxy_next_upstream swaps the peer connection, so pc->sent restarts from
zero while the flushed mark still held the previous peer's total. The
decrease was read as 'nothing to do' and every retried byte was lost. Treat
a decrease as a counter restart.
- The interval clock was stamped even when nothing was written. The proxy
module calls in once before any data has moved, so the first bytes of a
session stayed invisible for a whole second. Only an actual write starts
the interval.
- A UDP session has no FIN to observe, so one that simply ran to completion
reported no reason at all. Fall back to a normal close when the session
ended on NGX_STREAM_OK, and record the reason on the UDP shutdown path too.
- Unix sockets inside stream{} are internal plumbing, not proxy ports.
Counting them reported gateway control traffic as proxied bytes and left a
permanent floor of worker connections in the active gauge.
- A listening address too long for a slot was dropped silently; warn instead.
resty.apisix.stream.metrics no longer restricts itself to the stream
subsystem: the zone is process global and the reader touches no session, so
http can read it too.
Tests now cover what the requirements actually asked for: counters moving
while a session is still open (with exact byte counts), UDP accounting and
its reason, and the exclusion of internal unix sockets.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
80-90: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClarify the
closedmapping for UDP sessions.A successful UDP session maps to
closed, even though UDP has no FIN. State thatclosedmeans normal completion for TCP and UDP. Retainupstream_timeoutfor a UDP upstream that never answers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 80 - 90, Update the README status-table description for closed to state that it represents normal completion for both TCP and UDP sessions, without implying UDP uses FIN. Keep the upstream_timeout entry unchanged for UDP upstreams that never answer.
🧹 Nitpick comments (1)
t/stream/metrics.t (1)
79-103: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse asymmetric payload sizes for directional-byte assertions.
Test 4 only verifies nonzero values. Test 8 uses four bytes in both directions. A request/response counter mapping error can pass both tests.
t/stream/metrics.t#L79-L103: Use a deterministic TCP upstream with different request and response payload sizes. Assert exact values for the proxied listener.t/stream/metrics.t#L184-L231: Send a payload with a length different from"pong". Assert the four expected values, such asdi=7 de=4 ue=7 ui=4.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@t/stream/metrics.t` around lines 79 - 103, The directional-byte tests need asymmetric payloads and exact assertions to detect counter mapping errors. In t/stream/metrics.t lines 79-103, update TEST 4 to use a deterministic TCP upstream with different request and response payload sizes, then assert exact byte values for the proxied listener instead of only checking nonzero flags; in lines 184-231, change the sent payload to a length different from “pong” and assert all four expected directional values, such as di=7, de=4, ue=7, and ui=4.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 38-40: Update the README text describing Unix sockets inside
stream{} to explicitly state that the AF_UNIX exclusion also omits
user-configured Unix proxy listeners, or revise the documented filter behavior
to exclude only APISIX’s internal worker event channel while retaining
user-configured Unix listeners.
- Around line 53-56: Update the proxy_next_upstream documentation to state that
the module’s counters aggregate bytes across all attempts, while
$upstream_bytes_sent and $upstream_bytes_received are per-attempt values;
instruct users to sum the corresponding per-attempt values before comparing, and
remove the claim that the aggregate is more accurate.
---
Outside diff comments:
In `@README.md`:
- Around line 80-90: Update the README status-table description for closed to
state that it represents normal completion for both TCP and UDP sessions,
without implying UDP uses FIN. Keep the upstream_timeout entry unchanged for UDP
upstreams that never answer.
---
Nitpick comments:
In `@t/stream/metrics.t`:
- Around line 79-103: The directional-byte tests need asymmetric payloads and
exact assertions to detect counter mapping errors. In t/stream/metrics.t lines
79-103, update TEST 4 to use a deterministic TCP upstream with different request
and response payload sizes, then assert exact byte values for the proxied
listener instead of only checking nonzero flags; in lines 184-231, change the
sent payload to a length different from “pong” and assert all four expected
directional values, such as di=7, de=4, ue=7, and ui=4.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1db6d97c-a34b-4443-88a0-09ff955ae8b0
📒 Files selected for processing (8)
README.mdlib/resty/apisix/stream/metrics.luapatch/1.21.4/nginx-stream_metrics.patchpatch/1.25.3.1/nginx-stream_metrics.patchpatch/1.27.1.1/nginx-stream_metrics.patchpatch/1.29.2.4/nginx-stream_metrics.patchsrc/stream/ngx_stream_apisix_metrics_module.ct/stream/metrics.t
🚧 Files skipped from review as they are similar to previous changes (6)
- patch/1.27.1.1/nginx-stream_metrics.patch
- patch/1.25.3.1/nginx-stream_metrics.patch
- lib/resty/apisix/stream/metrics.lua
- patch/1.29.2.4/nginx-stream_metrics.patch
- patch/1.21.4/nginx-stream_metrics.patch
- src/stream/ngx_stream_apisix_metrics_module.c
…t the README Second review round: - proxy_next_upstream drops the peer and pc->sent with it. Treating a decrease as a restart only recovered the bytes if the new peer's counter had not yet overtaken the old mark; otherwise the failed attempt was still lost. Flush and reset the upstream mark before the peer goes away instead, which makes the total exact rather than nearly exact. The loss was small in practice -- every next_upstream call site runs before u->connected, so only PROXY protocol and TLS handshake bytes were at stake -- but the requirement is exactness. - Slot publication had no write barrier. Claiming is master-only, but a reload reuses the zone while the previous generation of workers is still reading, so on a weakly ordered architecture a reader could see the incremented count before the address it exposes. The old comment claimed fork made this safe, which is only true for a cold start. - Guard the AF_UNIX check with NGX_HAVE_UNIX_DOMAIN, as nginx does everywhere else, and clamp the address length the FFI reader copies. - resty.apisix.stream.metrics threw on a build without the stream addon rather than returning the documented nil, err. The README promised totals that were 'exact' and upstream counters 'more accurate' than nginx's own variables; neither held on the retry path. It also omitted that removing the directive releases the zone, and that slots are never reclaimed. TEST 9 asserted only that no unix slot exists, which a long enough servroot path would satisfy by tripping the address length check instead. It now asserts the exact slot set.
There was a problem hiding this comment.
Pull request overview
This PR adds TCP/UDP stream proxy observability to the APISIX nginx-module runtime by introducing a per-listening-address shared metrics zone (active sessions + byte counters) and a new $stream_session_reason stream variable, with Lua FFI access for live scraping.
Changes:
- Add
ngx_stream_apisix_metrics_moduleimplementingapisix_stream_metrics_zone,$stream_session_reason, and$stream_listen_addr. - Patch multiple supported nginx stream proxy module versions to record termination reasons and to update/flush byte counters during a live session.
- Add Lua FFI reader (
resty.apisix.stream.metrics) plus Test::Nginx coverage and README documentation for the new directive/variables.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| t/stream/metrics.t | Adds stream-level tests for reason classification and per-listen live counters (TCP + UDP) via FFI. |
| src/stream/ngx_stream_apisix_module.h | Makes the metrics API reachable from the proxy-module patch include entrypoint. |
| src/stream/ngx_stream_apisix_metrics_module.h | Declares reason enum, FFI-visible entry layout, and proxy-module hooks. |
| src/stream/ngx_stream_apisix_metrics_module.c | Implements the metrics zone, slot mapping, flush logic, stream variables, and FFI dump. |
| src/stream/config | Registers the new stream module as a separate addon module in the build. |
| README.md | Documents apisix_stream_metrics_zone, $stream_session_reason, and $stream_listen_addr, including behavior notes/limitations. |
| patch/1.21.4/nginx-stream_metrics.patch | Hooks stream proxy module to set reasons and flush/update counters. |
| patch/1.25.3.1/nginx-stream_metrics.patch | Same as above for nginx 1.25.3.1 patchset. |
| patch/1.27.1.1/nginx-stream_metrics.patch | Same as above for nginx 1.27.1.1 patchset. |
| patch/1.29.2.4/nginx-stream_metrics.patch | Same as above for nginx 1.29.2.4 patchset. |
| lib/resty/apisix/stream/metrics.lua | Adds Lua FFI reader for dumping per-listen stream metrics from shared memory. |
Suppressed comments (1)
src/stream/ngx_stream_apisix_metrics_module.c:833
ngx_stream_apisix_metrics_dump()readssh->nusedand then immediately readssh->slots[]. Since the master may append slots during reload (with only a writer-side barrier), add a reader-sidengx_memory_barrier()after readingnusedto ensure slot contents are visible before copying them.
n = ngx_min(sh->nused, max);
for (i = 0; i < n; i++) {
slot = &sh->slots[i];
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…e README PR review comments: - ngx_stream_apisix_set_session_reason indexes the reason table with a value that arrives from the ngx_stream_proxy_module patches, which are separate files free to drift from this enum. Refuse an out-of-range value instead of reading past the table. - The unix socket note read as though only internal sockets are skipped. The filter is on the address family, so a unix listener configured for proxying is skipped too; say so. - $upstream_bytes_sent and $upstream_bytes_received report one value per attempt rather than a total, so the retry note now says they have to be summed before they can be compared with these counters.
A store barrier alone only orders the writer. A reader that loads nused and then the slot it exposes still lets a weakly ordered CPU hoist the slot load above the count load, which is the very reordering the writer barrier exists to prevent. Snapshot nused, barrier, then walk the slots, in both the lookup and the FFI dump.
Final review round. The once-a-second throttle did not bound staleness, it removed it. A skipped delta has no later trigger: nothing calls back into the forwarding path until more data arrives or the session ends. For request/response traffic on a long lived connection -- the common L4 shape -- a whole response could stay invisible for as long as the connection then stayed idle, which is exactly what keeping the counters in nginx was supposed to avoid, and the README claim of 'up to a second' was simply wrong. The hook already sits where nginx has drained everything readable, so the session-local accumulation still collapses the inner read/write loop into one atomic per direction. The throttle was buying almost nothing for that. Giving up before any peer answers -- connection refused, no live upstream, a failed or timed out upstream handshake -- left no mark on the connection flags and so reported no reason at all. It now reports connect_failed, and the README no longer describes '-' as only meaning a pre-proxy rejection. TEST 7 could not have caught the throttle bug: the flush mark starts at zero, so the first burst always published. It now sends a second burst on the still open session, which is precisely what a deferred flush would hide. TEST 3 gained the reason assertion for the path it was already walking.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the runtime half of rfcs#245 (TCP stream proxy observability).
Why
A TCP proxy cannot be monitored from Lua alone:
$statusat 200 for every failure that happens after the upstream connection is established. Idle timeouts, upstream resets and client resets are all indistinguishable from a clean close (ngx_stream_proxy_module.c, all post-connect paths end inngx_stream_proxy_finalize(s, NGX_STREAM_OK)).$bytes_receivedand friends only exist at log time, so a long-lived connection reports nothing until it ends.What
A new
ngx_stream_apisix_metrics_module:apisix_stream_metrics_zone <size>reserves a shared memory zone holding, per stream listening address, the active session count and bytes moved in the four directions (downstream/upstream × ingress/egress). A session accumulates locally and merges into the zone at most once per second, plus a final flush, so long-lived connections keep the counters moving without an atomic operation per read or write. Slots are claimed ininit_modulefromcycle->listening, so the hot path is an index lookup and an atomic add — no locking.$stream_session_reasonreports why a session ended:closed,client_rst,client_error,upstream_rst,upstream_error,connect_timeout,recv_timeout,send_timeout,upstream_timeout,shutdown.$stream_listen_addris the configured listening address, which is what the zone keys its slots by.$server_addris the address the connection was accepted on and differs on a wildcard listen.resty.apisix.stream.metricsexposes the zone to Lua over FFI.Both are inert unless configured: without the zone directive nothing is collected, and
$stream_session_reasonstill works on its own.About the nginx patch
The patch stays deliberately small — it only records what nginx destroys on its own:
ngx_stream_proxy_process_connectionclearsev->timedoutbefore finalizing, so aproxy_timeoutis unrecoverable afterwards.ngx_stream_proxy_finalizefreesu->peer.connectionbefore the log phase runs, so both the upstream-side reason and the last byte delta towards the upstream have to be taken at its entry.Everything else is derived from the connection flags: a reset leaves
read->errorset even though the proxy module rewrites it into an EOF, and a failed write leaveserrorset on the destination connection.The patch adds no include hunk —
nginx-tcp_over_tls.patchowns the include block of that file and pulls inngx_stream_apisix_module.h, which now brings in the metrics header. Touching those lines from a second patch would break its context.Testing
-Werroragainst nginx 1.21.4, 1.25.3, 1.27.1 and 1.29.2; patches added for all fourpatch/directories.t/stream/metrics.t: 16 subtests covering normal close, receive timeout, native 502, the four byte directions and the active gauge over FFI, plus the no-zone-configured paths.closed,recv_timeout,client_rst,upstream_rstand the native 502 are each distinguished correctly.Follow-ups
Needs a
1.19.9tag; api7/apisix-build-tools then bumpsapisix_nginx_module_veron both runtime lines, and the gateway consumes it through the released runtime.Summary by CodeRabbit
New Features
Documentation