NH-24786 x-trace-options custom-* kvs, bugfix, and lots more tests #85
Conversation
There was a problem hiding this comment.
Really awesome enhancement of the integration and unit testing @tammy-baylis-swi! I look a closer look at the integration tests and just skimmed the unit tests, mainly because the integration rules hinge on the far from clear x-trace-options (aka trigger trace) specification -- which this review helped highlight (and so yes, made some more tweaks on the spec ;p).
Feel free to address the logic around setting the x-trace-options-response header in a subsequent PR.
| # while value can contain more equals signs | ||
| if len(option_kv) > 1: | ||
| self.custom_kvs[option_key] = "=".join( | ||
| okv.strip() for okv in option_kv[1:] |
There was a problem hiding this comment.
Minor curiosity: just realized in line 53 it is option_kv = option.split("=", 2) which allows splitting beyond the first equal sign. Any reason why it can't be option_kv = option.split("=", 1) which should ensure further equal signs in the option value would not be split out, so is there still the need to rejoin split items here? e.g.
a = 'custom-something=value_thing=4'
>>> a.split('=',2)
['custom-something', 'value_thing', '4']
>>> a.split('=',1)
['custom-something', 'value_thing=4']
There was a problem hiding this comment.
No reason, yes it can be simplified! Addressed in 1ee0466, which will be in another PR.
| assert span_server.attributes["TriggeredTrace"] == True | ||
| assert "this-will-be-ignored" not in span_server.attributes | ||
|
|
||
| # Check root span tracestate has `sw` and `xtrace_options_response` keys |
There was a problem hiding this comment.
Very OCD nit: should be Check client span tracestate... instead?
There was a problem hiding this comment.
Yes! Addressing in b75c652
| 2. Some traceparent and tracestate are injected into service's outgoing request | ||
| (done by OTel TraceContextTextMapPropagator). | ||
| 3. No x-trace-options-response header is calculated because the extracted | ||
| x-trace-options does not include trigger-trace. |
There was a problem hiding this comment.
I realized the spec was quite unclear on this, so tweaked it more. But if an x-trace-options header was given, we should set an x-trace-options-response (which would indicate that "trigger trace was not requested" in this particular scenario).
Even though these x-trace-options* headers are primarily for trigger tracing, in effect they are (perhaps a bit over?)designed to be a generic way to pass "options" to influence the tracing behaviour, whether it is sampling (trigger trace) or setting specific attributes (custom-*) on a resulting trace. So request header x-trace-options should be matched with response header x-trace-options-response to tell the user what happened.
There was a problem hiding this comment.
Addressed in b75c652 (in another PR)
| spans = self.memory_exporter.get_finished_spans() | ||
| assert len(spans) == 0 | ||
|
|
||
| def test_signed_missing_xtraceoptions_header(self): |
There was a problem hiding this comment.
This would be the scenario where x-trace-options-response response header should not be injected.
|
Thanks @cheempz !
I will take you up on this offer since there are more PRs floating about anyway 😄 . Merging this one. |
Second attempt after closing #79 without merging.
There are a few small src code changes in this PR, while the rest is adding/updating/restructuring unit and integration tests.
Code changes:
custom-*KVs in extracted x-trace-options headers are written to entry spans as attributescustom-*are only ignored if there is no value, e.g.custom-key-but-no-value;(different from NH-24786 xtraceoptions custom kvs #79 which ignored if too many=signs)TriggeredTrace: trueis written to entry spans as attribute iftriggered-trace;is inx-trace-options.extractmethod so it still extractsx-trace-options-signatureeven if nox-trace-optionsheader.XTraceOptionsstruct so it still inits withx-trace-options-signaturevalue even if nox-trace-optionsvalue given.Manual test traces in this updated doc: https://swicloud.atlassian.net/wiki/spaces/NIT/pages/3294199981/2022-11-16+x-trace-options+set+custom-
Integration test changes:
test_scenario_6.pytotest_unsigned_tt.py.test_signed_tt.py.test_unsigned_tt.pyandtest_signed_tt.py(hopefully) test scenarios 6-12 in the Trigger Trace Request/Response Matrix, plus the basic indented cases under Handling and Response.test_scenario_8.pyto test thecustom-*andTriggeredTrace: truesetting for whentraceparentandtracestateare used for regular sampling, but any KVs inx-trace-optionsshould still be written in entry span attributes.Unit test changes:
XTraceOptionsunit tests to accommodate updates and bugfixes.tests/unit/test_sampler/containstest_sampler_calculate_attributes.pywhich is dedicated to testingcalculate_attributes, while the rest of the methods are tested intest_sampler.py.tests/unit/test_sampler/fixture/has fixtures/mocks shared by the Sampler tests.Does not include tests for: the Req/Resp matrix's invalid values, unexpected usage, input validation. Those are for future PRs because this one is already out of hand sorry.
Please let me know what you think! 😃 I hope my summary makes sense but this is a long one, so let me know if you'd like to huddle-review anything.