Skip to content

Gate W3C trace support to request phases with ngx.var access#345

Open
mtrop-godaddy wants to merge 1 commit into
ledgetech:masterfrom
mtrop-godaddy:fix/traceparent-otel-gating
Open

Gate W3C trace support to request phases with ngx.var access#345
mtrop-godaddy wants to merge 1 commit into
ledgetech:masterfrom
mtrop-godaddy:fix/traceparent-otel-gating

Conversation

@mtrop-godaddy

Copy link
Copy Markdown

The http library reads ngx.var.http_traceparent in send_request to propagate W3C trace context. ngx.var is only readable in request-bearing phases, so this crashes code running in init/init_worker.

Gate the read on ngx.get_phase() being in a request-bearing phase via a whitelist, so request-less phases (init, init_worker, timer, etc.) are excluded by default. Behavior is unchanged for real requests.

The http library reads ngx.var.http_traceparent in send_request to
propagate W3C trace context. ngx.var is only readable in request-bearing
phases, so this crashes code running in init/init_worker.

Gate the read on ngx.get_phase() being in a request-bearing phase via a
whitelist, so request-less phases (init, init_worker, timer, etc.) are
excluded by default. Behavior is unchanged for real requests.
@mtrop-godaddy mtrop-godaddy force-pushed the fix/traceparent-otel-gating branch from 5217ac3 to 9eff4ce Compare June 11, 2026 11:29
@mtrop-godaddy mtrop-godaddy marked this pull request as ready for review June 11, 2026 11:33
@sriemer sriemer self-requested a review June 15, 2026 07:26
@sriemer sriemer self-assigned this Jun 15, 2026
@sriemer sriemer added the bugfix label Jun 15, 2026

@sriemer sriemer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good. Thank you very much! 😃
Just the phases should be revisited. TIA

Comment thread lib/resty/http.lua
Comment thread lib/resty/http.lua
header_filter = true,
body_filter = true,
log = true,
balancer = true,

@sriemer sriemer Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phases reviewed. Those look mostly good. Thanks.
But what about adding "precontent" as seen in t/089-phase.t#L227?
I'd like to drop balancer and preread, because balancer has no full API support and preread is unlikely to be triggered.

@sriemer

sriemer commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Reviewed documentation and code:

@sriemer

sriemer commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Phases not listed as supported in https://openresty-reference.readthedocs.io/en/latest/Lua_Nginx_API/#ngxvarvariable:

  • server_rewrite
  • preread
  • balancer
  • precontent

Accessible via *by_lua*: server_rewrite, precontent, balancer

@sriemer

sriemer commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

server_rewrite and precontent may do full API calls in a sandbox. balancer has a limited subset. But preread is unlikely to be triggered.
So I would like to go for dropping balancer and preread.
I would like to add precontent as well.
@mtrop-godaddy Is this plan fine for you? TIA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants