NH-24786 fix xtraceoptions response calc #88
Conversation
…traceoptions-response-calc
There was a problem hiding this comment.
Almost there I think @tammy-baylis-swi! There is still one point of misunderstanding(?) about when the x-trace-options-response should be set, and I think it would be worth adding explicit checks of the injected response header itself rather than the indirect check of what's stashed in SpanContext.
tests/integration/test_scenario_8.py
Outdated
| entry span (mocked). Unsigned trigger trace header is ignored. This is | ||
| not the root span because it continues an existing OTel context. | ||
| 2. traceparent and tracestate headers in the request to the test app are | ||
| injected into the outgoing request (done by OTel TraceContextTextMapPropagator). | ||
| 3. The injected x-trace-options header is still also propagated. | ||
| 3. The valid injected x-trace-options header is still also propagated. |
There was a problem hiding this comment.
Very Nit: "header is still also propagated" might give the impression that we inject something into outbound calls. Might be clearer to says something like "valid x-trace-options is still handled and an x-trace-options-response header is injected into the response".
There was a problem hiding this comment.
This one addressed in c872217
tests/integration/test_scenario_8.py
Outdated
| 2. traceparent and tracestate headers in the request to the test app are | ||
| injected into the outgoing request (done by OTel TraceContextTextMapPropagator). | ||
| 3. The valid injected x-trace-options header is not propagated because not TT. | ||
| 4. No xtraceoptions response header because not TT. |
There was a problem hiding this comment.
Hehe. Please don't go nuts, but this is why we all did go a bit crazy sussing out the TT logic edge cases. This is a case where if a valid x-trace-options header is given, we should set an x-trace-options-response response header... it's not talked about in the W3C spec but you can see an example in the Trigger Trace request/response matrix row 21 (note X-Trace-Options-Response: trigger-trace=not-requested).
There was a problem hiding this comment.
Ok right yes. So the src code fix for this is addressed in b75c652. The tests and docstrings/comments around them have been updated in several other commits. Let me know if anything still amiss!
| # though it will have different span ID because of Flask | ||
| # app's outgoing request | ||
| assert "x-trace" in resp.headers | ||
| assert new_trace_id in resp.headers["x-trace"] |
There was a problem hiding this comment.
Nit: should also assert that response header has x-trace-options-response
There was a problem hiding this comment.
Addressed in e60aa96
| expected_trace_state = trace_api.TraceState([ | ||
| ("sw", "0000000000000000-01"), | ||
| ("xtrace_options_response", "auth####ok;trigger-trace####not-requested;ignored####this-will-be-ignored"), |
There was a problem hiding this comment.
I think it's also worth checking the actual resp.headers for x-trace-options-response (maybe close to the x-trace response header check), because that would be the actual expected and customer visible behaviour as part of the TT feature.
It's a side benefit that you are able to check the response header "ingredients" are being stashed in the SpanContext, but it's an implementation detail and should not be used as the only verification that "yes, we're injecting the response header" :)
This applies to several of the subsequent tests.
There was a problem hiding this comment.
Great call! Added in e60aa96!
tammy-baylis-swi
force-pushed
the
NH-24786-fix-xtraceoptions-response-calc
branch
from
1126455 to
e8d147c
Compare
|
Thanks @cheempz for spotting the issues! I've addressed them in commits as commented, and updated the PR description for clarity. As suggested in your absence I've also tagged @molafson on this PR for Please have a look when you have a moment and let me know if you've spotted any other deviations from spec, unexpected values, or even docstrings/comments that might still be conflicting. 😃 |
|
Invalid value cases 15,16 in Request/Response matrix should be fixed in 59bec51 so that |
tammy-baylis-swi
deleted the
NH-24786-fix-xtraceoptions-response-calc
branch
Updated Dec 8.
Summary of changes
x-trace-options-responseheader was not being added to responses ifx-trace-optionsrequest header was present but missingtrigger-trace-- it should be added if anyx-trace-options. Addressed in b75c652 with other commits for tests.xtrace_options_responsewas included in injected/outgoingtracestaterequest header -- it should only be used for stashing before calculation ofx-trace-options-responseresponse header. Addressed in 38e74ef.xtrace_options_responsefromtracestate, instead of Sampler (7b8a993).xtrace_options_responseis an APM constant, instead of class method of W3CTransformer helper (ec3cf3a).