Skip to content

fix(opentelemetry): preserve booleans, handle multi-value headers, tighten test#13315

Open
shreemaan-abhishek wants to merge 2 commits intoapache:masterfrom
shreemaan-abhishek:fix/otel-additional-attrs
Open

fix(opentelemetry): preserve booleans, handle multi-value headers, tighten test#13315
shreemaan-abhishek wants to merge 2 commits intoapache:masterfrom
shreemaan-abhishek:fix/otel-additional-attrs

Conversation

@shreemaan-abhishek
Copy link
Copy Markdown
Contributor

Description

Three follow-ups to #13146 (which started coercing additional_attributes values to strings via tostring() before passing them to attr.string()). The patch as merged still has three discrete gaps:

1. Boolean false is silently dropped

inject_attributes() gates insertion on if val then. Lua treats false as falsy, so any additional_attribute whose resolved value is the boolean false (from an nginx variable, or set explicitly by a plugin) never makes it into the span at all. Switching to if val ~= nil then is the standard Lua idiom for "exists, including false".

2. Multi-value headers stringify to "table: 0x..."

When additional_attributes references a header source via prefix-match (X-Foo-*), the value comes from core.request.headers(api_ctx) which wraps ngx.req.get_headers(). That can return a Lua table for multi-value headers (multiple Set-Cookie, multiple X-Forwarded-For, etc.). tostring(some_table) then emits "table: 0x7f..." and that's what ends up as the OTel attribute value — useless.

This PR extracts a coerce_attr_value(val) helper that:

  • Returns nil for nil (caller skips the attribute).
  • Joins table values with ", " (table.concat).
  • Falls through to tostring() for everything else.

Both the prefix-match path and the keyed path go through the helper.

3. TEST 22 is too loose to detect a regression

The new TEST 22's response_body assertion is:

qr/.*opentelemetry-lua.*/

That's the OTel exporter's library-name marker; every OTel export contains it, regardless of whether additional_attributes actually made it into the span. The test would still pass with the regression #13146 was supposed to fix.

Tighten it to assert each of request_time and bytes_sent appears as a "stringValue" in the export:

qr/.*opentelemetry-lua.*"key":"request_time","value":\{"stringValue":"[^"]+"\}.*"key":"bytes_sent","value":\{"stringValue":"[^"]+"\}.*/s

upstream_response_time is intentionally not asserted: TEST 21 calls /opentracing which is served directly without proxy_pass, so $upstream_response_time is nil and the plugin correctly omits the attribute. Asserting it would fail for the wrong reason.

Which issue(s) this PR fixes:

Follow-up to #13146; no separate issue.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change — TEST 22 in t/plugin/opentelemetry.t is now a real regression guard for the string-coercion contract.
  • I have updated the documentation to reflect this change — no doc impact; the plugin's documented contract is unchanged.
  • I have verified that this change is backward compatible

…ghten test

Three follow-ups to apache#13146 (which started coercing additional_attributes
values to strings before sending them to OTel attr.string):

1. inject_attributes() gated insertion on `if val then`, which silently
   drops boolean false (Lua falsy) — regressing any user whose nginx
   variable resolves to a boolean false flag. Switch to
   `if val ~= nil then` so false survives.

2. ngx.req.get_headers() returns a Lua table for multi-value headers
   (e.g. multiple Set-Cookie / X-Forwarded-For entries). tostring()
   over a table emits 'table: 0x...' and that's what was landing in
   the span attribute. Extract a coerce_attr_value() helper that joins
   multi-value entries with ', ' and skips when the source is missing
   entirely.

3. TEST 22's response_body regex was qr/.*opentelemetry-lua.*/ which
   matches the OTLP exporter marker even when the numeric attributes
   are entirely absent from the export — i.e. the test would still
   pass with the regression apache#13146 was supposed to fix. Tighten it to
   assert each of request_time / bytes_sent appears as a stringValue.
   (upstream_response_time is intentionally not asserted: TEST 21
   serves /opentracing directly without proxying, so
   $upstream_response_time is nil and the plugin correctly omits the
   attribute.)

Signed-off-by: Shreemaan Abhishek <shreemaan.abhishek@gmail.com>
Signed-off-by: Abhishek Choudhary <shreemaan.abhishek@gmail.com>
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Apr 29, 2026
AlinsRan
AlinsRan previously approved these changes Apr 30, 2026
membphis
membphis previously approved these changes Apr 30, 2026
…ional-attrs

Signed-off-by: Abhishek Choudhary <shreemaan.abhishek@gmail.com>
@shreemaan-abhishek shreemaan-abhishek dismissed stale reviews from membphis and AlinsRan via 3a70010 April 30, 2026 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants