NH-24786 Fix and test x-trace-options input validation #87
Conversation
There was a problem hiding this comment.
Thanks @tammy-baylis-swi! LGTM except one main thing I somehow missed on the other reviews 😓, which feel free to address in the next PR (it'll touch multiple tests).
| # In this test we know there is `sw` and `xtrace_options_response` | ||
| # in tracestate where value of former will be new_span_id and new_trace_flags | ||
| assert "tracestate" in resp_json | ||
| assert resp_json["tracestate"] == "sw={},xtrace_options_response=trigger-trace####ok;ignored####foo".format( |
There was a problem hiding this comment.
Oops I missed this on reviewing the previous PRs -- the actual tracestate header injected into outbound requests should NOT contain any of the xtrace options information, we should only set the sw= value per our W3C handling rules.
We should only use the SpanContext TraceState as a place to "stash", in memory, these x-trace-options values in order to make them available at the point of x-trace-options-response header injection, aka at the end of the request.
| ("sw", "0000000000000000-01"), | ||
| ("xtrace_options_response", "trigger-trace####ok;ignored####foo"), | ||
| ]) | ||
| assert span_server.context.trace_state == expected_trace_state |
There was a problem hiding this comment.
So yes, here I do expect to see the SpanContext TraceState to have the stashed value (with the little #### "encoding hack").
|
Thank you @cheempz ! Yep, I'll create another PR to fix that propagation. |
Fixes
x-trace-optionsheader input validation so thatsw-keys/custom-*/tsvalues are used if multiple inx-trace-optionsheader (fb06d23 and 9a77a25)=are allowed insw-keystoo, likecustom-*(a28215e)The other src code update here is to simplify splitting of header values (1ee0466) as per previous PR comment.
Through writing tests it seems the other cases under Input Validation are already being handled correctly. The rest of this PR is more unit tests and adding a new integration tests suite,
test_xtraceoptions_validation.py.This PR does not include the
x-trace-options-responsefix. That will be in another PR. So for now, there are response headers missing when there should be for some tests.Please let me know what you think! 😺