NH-11358 Add integration tests #67

tammy-baylis-swi · 2022-10-25T20:07:03Z

This PR adds working, automated, integration tests! They will run as part of the existing GH workflow for automated test runs that we added in this PR.

This replaces a previous PR that wasn't working out so I closed it without merging.

To run the tests on my local a bit quicker (only one env) and to make sure all the new dependencies install, I've used:

make tox OPTIONS="--recreate -vv -e py37-nh-staging"

Rationale for this setup

This is based heavily on this conversation we had in May. It tests acceptance criteria 1, 4, and 6. EDIT 2022-11-15: and now 8 too.

EDIT 2022-11-15: Ignore the rest of this. It's now totally inaccurate.

Summary of changes

(1)
SolarWindsDistroTestBase does setup shared by the two Test* classes below. To initialize our custom OTel components and have them readily accessible at test time, we call individual SolarWindsConfigurator methods. Our testbase extends OTel's TestBase to prepare the InMemorySpanExporter used to check finished spans. This way, these tests all done in the same process/thread (not sure which) and doesn't go so far as to spin up separate containers like the testbed.

(2)
TestSpanAttributes uses the OTel SDK to manual create spans with our custom OTel components then check their attributes and trace states, all while mocking liboboe decisions.

For scenario 4, we manually extract traceparent and tracestate headers with our custom (composite) propagator. The resulting OTel context needs to be manually passed to the SDK at span creation else it won't find it. (This also wouldn't work with Flask as attempted in the failed previous PR.)

For scenario 6, we do the same as 4 plus extract x-trace-options and x-trace-options-signature for signed TT.

There is an additional method test_child_span_attrs. We mock its remote parent by setting a full span context instead of providing OTel context.

(3)
PropagationTest defines a minimal Flask app that we can instrument. It has a Werkzeug client exposed for making requests in-code instead of to a separate service. Its route /test_trace makes an outgoing request so that we can capture the traceparent and tracestate it sends out and check them.

(4)
TestHeaderPropagation uses the Flask app to test header propagation performed by our custom OTel components, also while mocking liboboe decision. The main reason this is separate from TestSpanAttributes is that I couldn't get the Flask app's spans to export to InMemorySpanExporter (what I attempted in the failed previous PR).

(5)
Bonus: while writing these tests I found a typo bug that's been fixed here in 431bd12.

Not in this PR

I realized while writing this that this doesn't test what happens when invalid values are given, e.g. incorrect traceparent and tracestate. Given the size of this PR already, I'd like to add them in a different PR.

The ignoring of some x-trace-options that shouldn't be will be fixed in
https://swicloud.atlassian.net/browse/NH-24786. It is there that I will add tests for incorrect TT-related values and fix the TODOs I've added.

I'll add this information to this doc soon so it's not lost. EDIT: Done, new section: https://swicloud.atlassian.net/wiki/spaces/NIT/pages/2917272644/NH+Python+Automated+Regression+Testing#Example-implementation

Please let me know what you think! 😺

…led by start_span

…n_attrs

…traceparent_tracestate

tammy-baylis-swi · 2022-11-16T00:38:54Z

Regarding this comment:

So why did I put all that in there?

It's because non-entry spans need an up-to-date tracestate at time of injection, rather than the placeholder set by the sampler!

Redesign integration tests

tammy-baylis-swi · 2022-11-16T01:44:39Z

Hi @cheempz ! Thanks again for all those suggestions. I've addressed all that I can while also making big changes to how these tests are set up. Therefore, the original PR description is now inaccurate but I won't update Confluence in good English until this makes sense to us both 😃

Key updates:

I figured out how to set up all of custom composite OTel propagation for sw and trigger trace tracestate dealings, custom SW sampling logic for attribute setting, and export to InMemorySpanExporter for span inspection!
Redesigns integration tests so that each script contains one of the W3C trace context Acceptance Criteria scenarios 1, 4, 6, or 8 (https://swicloud.atlassian.net/wiki/spaces/NIT/pages/2325479753/W3C+Trace+Context#Acceptance-Criteria).
Each of the scenario scripts extends a single, new shared class TestBaseSwHeadersAndAttributes for configuration of OTel components, test app config, and shared setup/teardown.
All the classes that existed at the start of this PR are gone (SolarWindsDistroTestBase, TestSpanAttributes, TestHeaderPropagation, PropagationTest)
Also merges from main the liboboe bump to 11.0.0 and other changes to stop some version conflicts.

Please let me know what you think! This is a lot of changes; sorry for another mega PR.

cheempz

💯 what an awesome revamp @tammy-baylis-swi! This was great to read, educational, and such a good foundation that you managed to glue all the pieces together (header extraction/injection, resulting span attributes) into a simple to understand structure, kudos!

I left minor comments feel free to ignore or address in subsequent PRs.

cheempz · 2022-11-21T20:52:52Z

tests/integration/test_scenario_1.py

+           (done by OTel TraceContextTextMapPropagator).
+        4. Sampling-related attributes are set for the root/service entry span.
+        5. The span_id of the outgoing request span matches the span_id portion in the
+           tracestate header.


Love this docstring and your point 5 reminded me that this was never made clear in the W3C Trace context spec! I've updated the paragraph about tracestate accordingly.

cheempz · 2022-11-21T21:21:47Z

tests/integration/test_scenario_6.py

+        expected_trace_state = trace_api.TraceState([
+            ("sw", "0000000000000000-01"),
+            ("xtrace_options_response", "trigger-trace####ok"),
+        ])


Really appreciate how you have tests that now document previously opaque parts of our customization :)

cheempz · 2022-11-21T21:32:58Z

tests/integration/test_scenario_6.py

+           entry span (mocked). There is no OTel context extracted from request headers,
+           so this is the root and start of the trace.
+        2. Some traceparent and tracestate are injected into service's outgoing request (done by OTel TraceContextTextMapPropagator).
+        3. The injected x-trace-options header is also propagated.


The inbound x-trace-options header doesn't actually need to be propagated to any outbound calls. But I'm guessing here you are referring to how we propagate the Trigger Trace response value (from the tracing decision call) from span to span, in order to make it available at the end of the request when we inject our custom x-trace-options-response header.

Yes you're right! Fixed the wording in f327539

cheempz · 2022-11-21T21:34:55Z

tests/integration/test_scenario_6.py

+        expected_trace_state = trace_api.TraceState([
+            ("sw", "0000000000000000-01"),
+            ("xtrace_options_response", "trigger-trace####ok"),
+        ])


Same as above comment, this is great test-as-documentation on we use TraceState to stash the trigger trace response so it's available at the time of our custom injecting of the x-trace-options-response header.

Yes! Addressed in cce07b4

cheempz · 2022-11-21T21:44:47Z

tests/integration/test_scenario_8.py

+            new_span_id,
+            new_trace_flags,
+        )
+        # TODO NH-24786 will not ignored foo


Hehe it's actually correct right now, foo should be ignored.

Thank you! My silly brain. Rm those todos in b55984c

cheempz · 2022-11-21T21:48:46Z

tests/integration/test_scenario_8.py

+        assert "trigger-trace=ignored" in resp.headers["x-trace-options-response"]
+        assert "ignored=foo" in resp.headers["x-trace-options-response"]
+        # TODO NH-24786 will not ignored foo
+        # assert "ignored" not in resp.headers["x-trace-options-response"]


The change with NH-24786 is that keys like custom-* should now be set as attributes on the span, so for this case I'd expect these attributes:

TriggeredTrace: true

custom-from: lin

SWKeys: custom-sw-from:tammy,baz:qux

The TT spec does not make this easy to see, I will extra highlight in the Jira and also update the spec to hopefully make it clearer.

Thanks Lin! I've removed this todo in b55984c and will update the PR for NH-24786!

cheempz · 2022-11-21T21:52:50Z

tests/integration/test_scenario_8.py

+        assert new_trace_id in resp.headers["x-trace"]
+
+        # Verify x-trace-options-response response header present
+        # and has same values as tracestate but different delimiters


Very nit: "and has same values as internally cached in TraceState..." would make clearer that we don't expect it to have the same value as the actual tracestate header that was injected in outbound requests.

Good idea! Addressed in 20a8de4

cheempz · 2022-11-21T22:06:45Z

CONTRIBUTING.md

+
+#### Integration tests
+
+The integration tests are defined in `tests/integration/`. They require a compiled C-extension and should be run inside the build container. They are also run under tox as per the unit tests. Here is how to run tests locally:


Hehe, very minor: I gave this a try and was BAD and didn't set any SW_APM_SERVICE_KEY_TOX_* env vars, but the tests passed fine. So it seems that the tox-driven unit and integration tests don't actually need valid SW_APM_ env vars (which makes sense given what they're testing).

Yess!!! This must have been leftover from a very old setup. Removed in ea5d5d8

tammy-baylis-swi · 2022-11-21T23:09:13Z

Thank you again @cheempz ! All of last comments addressed. Please could I get another review? 😺

cheempz

LGTM, thanks @tammy-baylis-swi!

tammy-baylis-swi added 30 commits May 18, 2022 18:21

WIP mess

ead50fc

Add unittest mock of get_current_span() and get_span_context() as cal…

52f3b26

…led by start_span

Add some scenario 4 assertions

cec6d5e

Change mock values for less confusion

9c8626a

Add experimental manual_propagation test

a9f78ef

Add bare-bones Flask app, some S4 asserts

2a8a26b

Simplify scenario4 test to compare inject to span contents and rename

7e0cb2e

Clean up TestFunctionalSpanAttributes experiment

8da6daf

Rm unknown (un)instrument calls in test

3c004b0

Merge branch 'main' into NH-11358-functional-tests

4eba91a

Move functional tests to own dir

744de2c

Rename two tests

9ebd0d9

Fix InMemorySpanExporter setup and use

aae43f5

Add test_non_root_attrs_only_do_sample

36d94cd

Add test_child_span_attrs

79bf019

Add test_root_span_attrs_with_traceparent_and_tracestate

bb64e5b

Rm confusing test_non_root_attrs_only_do_sample and test_internal_spa…

f681b91

…n_attrs

Add stub todo

9f09b2b

Bugfix: typo attributeerror

431bd12

Add test_root_span_attrs_with_signed_trigger_trace

54aafb3

Rename test class, update comment

bf9e6c8

TestHeaderPropagation redesigned. Added test_injection_with_existing_…

f0dc9ea

…traceparent_tracestate

Add test_injection_with_existing_traceparent_tracestate_not_sampled

77d18ba

Add test_injection_new_decision

5f85adc

Add test_injection_signed_tt

ba5f997

Rm unused imports, update comment

af1c9b5

Add SolarWindsDistroTestBase and refactor

b73ac49

Fix mock decision retvals, add test_root_span_attrs_not_sampled

9beb8d2

Add helper_test_root_span_attrs and refactor

c801858

Update comments

94a8754

tammy-baylis-swi added 11 commits November 15, 2022 10:26

Add span tracestate checks

fc3156c

assert is not None

95cd700

Update comments

820e1ae

Simplify regex use

b387d44

Mock signature for tt test scenario

2964d1a

Scenario6 does not extract traceparent, tt unsigned

b74eb3a

Update comment

e7688b1

Add scenario8 tests

06da4b4

Update comment wording for true root spans

7518439

Add checks for other some-header interference

9d90f20

Add continue decision propagation checks

397b877

tammy-baylis-swi and others added 3 commits November 15, 2022 16:46

Merge pull request #77 from appoptics/redesign-integration-tests

aa31cc2

Redesign integration tests

Update comment

1ed26a6

Update comment

3e893bd

tammy-baylis-swi requested a review from cheempz November 16, 2022 01:44

Merge branch 'main' into NH-11358-functional-tests

6ccb8f9

cheempz previously approved these changes Nov 21, 2022

View reviewed changes

Fix scenario_6 docstring on x-trace-options-response

f327539

tammy-baylis-swi dismissed cheempz’s stale review via f327539 November 21, 2022 22:37

tammy-baylis-swi added 4 commits November 21, 2022 14:41

Update comment

cce07b4

Rm unnecessary todos

b55984c

Update more comment

20a8de4

Rm unnecessary tox service key env vars

ea5d5d8

tammy-baylis-swi requested a review from cheempz November 21, 2022 23:08

cheempz approved these changes Nov 22, 2022

View reviewed changes

tammy-baylis-swi merged commit a136522 into main Nov 22, 2022

tammy-baylis-swi deleted the NH-11358-functional-tests branch November 22, 2022 01:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NH-11358 Add integration tests #67

NH-11358 Add integration tests #67

tammy-baylis-swi commented Oct 25, 2022 •

edited

Loading

tammy-baylis-swi commented Nov 16, 2022

tammy-baylis-swi commented Nov 16, 2022

cheempz left a comment

cheempz Nov 21, 2022

cheempz Nov 21, 2022

cheempz Nov 21, 2022

tammy-baylis-swi Nov 21, 2022

cheempz Nov 21, 2022

tammy-baylis-swi Nov 21, 2022

cheempz Nov 21, 2022

tammy-baylis-swi Nov 21, 2022

cheempz Nov 21, 2022

tammy-baylis-swi Nov 21, 2022

cheempz Nov 21, 2022

tammy-baylis-swi Nov 21, 2022

cheempz Nov 21, 2022

tammy-baylis-swi Nov 21, 2022

tammy-baylis-swi commented Nov 21, 2022

cheempz left a comment


		#### Integration tests

		The integration tests are defined in `tests/integration/`. They require a compiled C-extension and should be run inside the build container. They are also run under tox as per the unit tests. Here is how to run tests locally:

NH-11358 Add integration tests #67

NH-11358 Add integration tests #67

Conversation

tammy-baylis-swi commented Oct 25, 2022 • edited Loading

Rationale for this setup

EDIT 2022-11-15: Ignore the rest of this. It's now totally inaccurate.

Summary of changes

Not in this PR

tammy-baylis-swi commented Nov 16, 2022

tammy-baylis-swi commented Nov 16, 2022

cheempz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tammy-baylis-swi commented Nov 21, 2022

cheempz left a comment

Choose a reason for hiding this comment

tammy-baylis-swi commented Oct 25, 2022 •

edited

Loading