fix(opentelemetry): preserve booleans, handle multi-value headers, tighten test#13315
Open
shreemaan-abhishek wants to merge 2 commits intoapache:masterfrom
Open
fix(opentelemetry): preserve booleans, handle multi-value headers, tighten test#13315shreemaan-abhishek wants to merge 2 commits intoapache:masterfrom
shreemaan-abhishek wants to merge 2 commits intoapache:masterfrom
Conversation
…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>
AlinsRan
previously approved these changes
Apr 30, 2026
membphis
previously approved these changes
Apr 30, 2026
…ional-attrs Signed-off-by: Abhishek Choudhary <shreemaan.abhishek@gmail.com>
3a70010
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.
Description
Three follow-ups to #13146 (which started coercing
additional_attributesvalues to strings viatostring()before passing them toattr.string()). The patch as merged still has three discrete gaps:1. Boolean
falseis silently droppedinject_attributes()gates insertion onif val then. Lua treatsfalseas falsy, so anyadditional_attributewhose resolved value is the booleanfalse(from annginxvariable, or set explicitly by a plugin) never makes it into the span at all. Switching toif val ~= nil thenis the standard Lua idiom for "exists, including false".2. Multi-value headers stringify to
"table: 0x..."When
additional_attributesreferences a header source via prefix-match (X-Foo-*), the value comes fromcore.request.headers(api_ctx)which wrapsngx.req.get_headers(). That can return a Lua table for multi-value headers (multipleSet-Cookie, multipleX-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:nilfornil(caller skips the attribute).", "(table.concat).tostring()for everything else.Both the prefix-match path and the keyed path go through the helper.
3.
TEST 22is too loose to detect a regressionThe 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_attributesactually 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_timeandbytes_sentappears as a"stringValue"in the export:upstream_response_timeis intentionally not asserted: TEST 21 calls/opentracingwhich is served directly withoutproxy_pass, so$upstream_response_timeis 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
t/plugin/opentelemetry.tis now a real regression guard for the string-coercion contract.