Fix W3C test service #686

yuktea · 2022-05-26T02:19:37Z

Attempts to fix the W3C test service by updating usage patterns to match the refactorings and functional changes that have been applied since it was last updated. The effort remains a work-in-progress with the tests failing due to a reason I've been unable to diagnose yet.

bobstrecansky · 2022-05-26T15:58:45Z

You'll need to run a make style to format your code and get to the next step of CI.

When I pulled down your branch and ran make test to test your code, I was able to see two relatively simple errors. Is that what you see as well?

yuktea · 2022-05-26T23:15:52Z

@bobstrecansky

You'll need to run a make style to format your code and get to the next step of CI.

When I pulled down your branch and ran make test to test your code, I was able to see two relatively simple errors. Is that what you see as well?

I think you'll need to run make w3c-test-service to get the test service to run. What I see on my end: first all the tests from the w3c test.py script are failing.

----------------------------------------------------------------------
Ran 40 tests in 24.305s

FAILED (failures=40)
ERROR: 40
make: *** [Makefile:64: w3c-test-service] Error 40

On investigating the curl outputs from the symfony app, I see an error about an invalid argument to foreach (1) and (2):

<!-- Attempted to load class &quot;AlwaysOnSampler&quot; from namespace &quot;OpenTelemetry\SDK\Trace\Sampler&quot;.
Did you forget a &quot;use&quot; statement for another namespace? (500 Internal Server Error) -->

~~I haven't figured out why (1) is happening because I didn't change the (I think) relevant part in W3CTestService/TestController.php.~~

~~Any clue what could be causing (2)?~~

UPDATE 1: I've resolved both (1) and (2).

codecov · 2022-05-26T23:22:30Z

Codecov Report

Merging #686 (6796211) into main (499c58a) will decrease coverage by 2.22%.
The diff coverage is 55.17%.

@@             Coverage Diff              @@
##               main     #686      +/-   ##
============================================
- Coverage     85.00%   82.77%   -2.23%     
- Complexity     1233     1242       +9     
============================================
  Files           137      139       +2     
  Lines          2980     3036      +56     
============================================
- Hits           2533     2513      -20     
- Misses          447      523      +76

Flag	Coverage Δ
7.4	`82.76% <55.17%> (-2.25%)`	⬇️
8.0	`82.82% <55.17%> (-2.23%)`	⬇️
8.1	`82.82% <55.17%> (-2.23%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/API/Trace/SpanContext.php	`82.92% <0.00%> (-4.26%)`	⬇️
...ation/SanitizeCombinedHeadersPropagationGetter.php	`0.00% <0.00%> (ø)`
...c/API/Trace/Propagation/TraceContextPropagator.php	`100.00% <100.00%> (ø)`
src/API/Trace/TraceState.php	`100.00% <100.00%> (ø)`
src/SDK/Trace/Span.php	`93.69% <0.00%> (-0.23%)`	⬇️
src/SDK/Common/Exception/StackTraceFormatter.php	`0.00% <0.00%> (ø)`
src/SDK/Trace/RandomIdGenerator.php	`85.71% <0.00%> (+5.71%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 499c58a...6796211. Read the comment docs.

yuktea · 2022-05-27T14:07:36Z

At this point, the test service is running as expected. However all 40 w3c tests are failing, output for all failures from the w3c trace-context test script looks like:

<!-- Unsupported carrier type: NULL. Unable to set value associated with key:traceparent (500 Internal Server Error) -->
<!DOCTYPE html>
<html lang="en">
    <head>
        <meta charset="UTF-8" />
        <meta name="robots" content="noindex,nofollow" />
        <meta name="viewport" content="width=device-width,initial-scale=1" />
        <title>Unsupported carrier type: NULL. Unable to set value associated with key:traceparent (500 Internal Server Error)</title>

@bobstrecansky any ideas to what might be causing this?

UPDATE: I recorded the wrong error initially.

yuktea · 2022-05-28T08:25:49Z

I've ironed out issues in the service controller. All tests still fail, I suspect we're doing something wrong within the controller still but I'm not sure. Could use help in diagnosis.

I replaced deprecated SpanContext::generate() with `SpanContext::createInvalid()`. That didn't look right for obvious reasons, so with this commit it's replaced with a `new Context()`. I'm still not sure if this does the job.

bobstrecansky · 2022-05-28T11:03:21Z

I'm trying to replicate this locally. What commands are you running to see your failures?

yuktea · 2022-05-28T16:29:32Z

I'm trying to replicate this locally. What commands are you running to see your failures?

make w3c-test-service

Edit: Sorry, I messed up some syntax in the setup script which got past me since I was running commands directly in a docker container. You should be able to reproduce the issues with the make command now.

tests/TraceContext/W3CTestService/index.php

yuktea · 2022-05-31T06:00:36Z

At of dff26c7 the w3c tests are still failing. I'll try to document my interpretation of what's going wrong with these tests in the hope it helps us figure out what it's lacking.

All 40 w3c tests fail.

Failure diagnosis output looks like:

======================================================================
FAIL: test_tracestate_value_illegal_characters (__main__.TraceContextTest)
harness sends a request with an invalid tracestate header with illegal value format
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/src/myapp/tests/TraceContext/W3CTestService/trace-context/test/test.py", line 819, in test_tracestate_value_illegal_characters
traceparent, tracestate = self.make_single_request_and_get_tracecontext([
File "/usr/src/myapp/tests/TraceContext/W3CTestService/trace-context/test/test.py", line 113, in make_single_request_and_get_tracecontext
return (self.get_traceparent(headers), self.get_tracestate(headers))
File "/usr/src/myapp/tests/TraceContext/W3CTestService/trace-context/test/test.py", line 101, in get_traceparent
self.assertEqual(len(retval), 1, 'expect one traceparent header, got {} {!r}'.format('more' if retval else 'zero', retval))
AssertionError: 0 != 1 : expect one traceparent header, got zero []

Going by the diagnosis message and looking through the TestController, I believe we're not returning the response header as expected. However I have not been able to figure out exactly what it is we should be returning in the
headers/body. Please take a look: https://github.com/yuktea/opentelemetry-php/blob/dff26c7119e4285b0b2ff99c6267645b90cdb853/tests/TraceContext/W3CTestService/TestController.php#L44
To reproduce on your end, please run make w3c-test-service. At this point, I have fixed the script and verified that the failure is reproducible.

/cc @bobstrecansky @tidal @brettmc

tests/TraceContext/W3CTestService/symfony-setup

bobstrecansky · 2022-05-31T12:23:39Z

I'm able to reproduce your error, and I also get 40 errors. It appears you turned the debug log on, that's a good start. What inconsistencies have you found in the traceparent logic that would cause these errors? Remember, this pulls in the w3c test service here:

https://github.com/open-telemetry/opentelemetry-php/blob/main/tests/TraceContext/W3CTestService/symfony-setup#L47

Nevay · 2022-05-31T13:45:45Z

Regarding TestController#L44, this should be:

$traceCtxPropagator->inject($headers, null, $context);

Some causes for the remaining failures:

the webserver concatenates multiple tracestate headers using ';' (see test_tracestate_empty_header)
the testsuite expects that the entire tracestate is discarded on invalid member (currently invalid members are dropped) (not required by spec; "If the tracestate header cannot be parsed the vendor MAY discard the entire header. Invalid tracestate entries MAY also be discarded.")
the tracestate propagator doesnt handle future versions (should allow >= 4 components instead of exactly 4 components if version is not 0x00)

yuktea · 2022-05-31T17:51:15Z

I'm able to reproduce your error, and I also get 40 errors. It appears you turned the debug log on, that's a good start. What inconsistencies have you found in the traceparent logic that would cause these errors? Remember, this pulls in the w3c test service here:

https://github.com/open-telemetry/opentelemetry-php/blob/main/tests/TraceContext/W3CTestService/symfony-setup#L47

I think the biggest consistency was discarding the traces the expected by the tests in our response headers to the test harness. Beyond this we still have some more failures to investigate that @Nevay's assessment seems to cover well.

As of 4cfc68c, we are passing 30/40 tests. I'm currently investigating the other failures with help from the comments above.

yuktea · 2022-06-01T06:15:50Z

I think at this point the W3C test service is working fine. The 10 remaining failures we likely owe to not complying with the W3C trace context spec which we could work on outside of this PR.

@Nevay

the webserver concatenates multiple tracestate headers using ';' (see test_tracestate_empty_header)

I tried getting around this by doing a string replacement ; -> , but that breaks the same amount of tests it fixes. Apparently ; is a valid character in tracestates :/.
Is there any other workaround for this? I haven't been able to figure it out yet.

yuktea · 2022-06-01T08:56:03Z

Once we get this merged I'll open another PR to factor this to vanilla PHP for speed.

yuktea · 2022-06-02T05:55:23Z

After these changes the errors are down to:

* 1x not supporting future versions, this has be resolved as it is required by spec

* 4x not dropping the entire tracestate on invalid member

Now we're down to these 5 failures in this PR. Should we cover them here?

/cc @bobstrecansky @tidal

bobstrecansky · 2022-06-02T13:42:42Z

We should attempt to cover them in this PR if possible.

-4x failures on the w3c trace context test: - Drop empty tracestates. - Drop the entire tracestate if a member is invalid. - Drop the entire tracestate if member count exceeds max.

yuktea · 2022-06-03T14:32:25Z

Should be good to go. Please check that the spec compliance changes are at the right place. Thanks

yuktea · 2022-06-03T14:42:27Z

Some tests breaking. Will take a look and fix in a while.

src/API/Trace/Propagation/TraceContextPropagator.php

yuktea · 2022-06-03T16:03:42Z

Updated unit tests.

edit: and added some additional relevant tests.

Also fixes now deprecated use of hex2bin on potentially invalid hex strings.

yuktea · 2022-06-08T20:12:06Z

lets get this merged?

tests/Unit/API/Trace/TraceStateTest.php

tests/TraceContext/W3CTestService/index.php

Co-authored-by: Brett McBride <brett.a.mcbride@gmail.com>

tests/TraceContext/W3CTestService/index.php

src/API/Trace/TraceState.php

Adds a new decorator to extract header sanitization to propagation getter decorator, for servers that concatenate duplicate headers with `;`. Enables this sanitation logic to be used across propagated state.

Fix codestyle

e5c8867

yuktea force-pushed the fix-w3c-test-service branch from f438e31 to 961e58f Compare May 27, 2022 13:40

yuktea force-pushed the fix-w3c-test-service branch from 961e58f to 0eeada3 Compare May 27, 2022 14:08

yuktea added 2 commits May 27, 2022 19:40

Fix symfony-setup script for W3C test service

0eeada3

Fix issues in W3C service controller.

8e92442

Fix context generation

7ae1646

I replaced deprecated SpanContext::generate() with `SpanContext::createInvalid()`. That didn't look right for obvious reasons, so with this commit it's replaced with a `new Context()`. I'm still not sure if this does the job.

Fix syntax wart in the symfony-setup script

dff26c7

brettmc reviewed May 29, 2022

View reviewed changes

tests/TraceContext/W3CTestService/index.php Outdated Show resolved Hide resolved

bobstrecansky reviewed May 31, 2022

View reviewed changes

tests/TraceContext/W3CTestService/symfony-setup Outdated Show resolved Hide resolved

fix: Inject context from request into response headers

4cfc68c

Update API usage

562b597

yuktea marked this pull request as ready for review June 1, 2022 06:10

yuktea requested review from zsistla and tidal as code owners June 1, 2022 06:10

yuktea force-pushed the fix-w3c-test-service branch from 79dcd16 to 77eb918 Compare June 1, 2022 08:55

chore: clean up controller

77eb918

yuktea added 2 commits June 3, 2022 19:04

fix: drop tracestate on invalid members

30b9fa5

-4x failures on the w3c trace context test: - Drop empty tracestates. - Drop the entire tracestate if a member is invalid. - Drop the entire tracestate if member count exceeds max.

fix: Add tolerance for future traceparent versions

5749582

yuktea force-pushed the fix-w3c-test-service branch from 7a5fb3f to 45974a5 Compare June 3, 2022 14:29

yuktea force-pushed the fix-w3c-test-service branch from 45974a5 to 8b0bf8a Compare June 3, 2022 14:37

fix: enhance version validation regex

8b0bf8a

Nevay reviewed Jun 3, 2022

View reviewed changes

src/API/Trace/Propagation/TraceContextPropagator.php Outdated Show resolved Hide resolved

test: update unit tests

9f4ff77

yuktea added 3 commits June 3, 2022 23:14

refactor: trace version validation

afb5b29

Also fixes now deprecated use of hex2bin on potentially invalid hex strings.

test: add unit tests for trace version validation

deb13c7

test: another trace validation unit test

8db1644

brettmc reviewed Jun 9, 2022

View reviewed changes

tests/Unit/API/Trace/TraceStateTest.php Outdated Show resolved Hide resolved

brettmc reviewed Jun 9, 2022

View reviewed changes

tests/TraceContext/W3CTestService/index.php Outdated Show resolved Hide resolved

brettmc approved these changes Jun 9, 2022

View reviewed changes

yuktea commented Jun 9, 2022

View reviewed changes

tests/TraceContext/W3CTestService/index.php Outdated Show resolved Hide resolved

Apply cleanup suggestions

ad98cdf

Co-authored-by: Brett McBride <brett.a.mcbride@gmail.com>

yuktea commented Jun 9, 2022

View reviewed changes

tests/TraceContext/W3CTestService/index.php Outdated Show resolved Hide resolved

yuktea added 2 commits June 9, 2022 10:47

Remove unused import

89e2de5

fix codestyle

3fc6908

Nevay reviewed Jun 9, 2022

View reviewed changes

src/API/Trace/TraceState.php Outdated Show resolved Hide resolved

Extract header sanitization to propagation getter decorator

5139f91

Nevay mentioned this pull request Jun 10, 2022

Extract header sanitization to propagation getter decorator yuktea/opentelemetry-php#1

Merged

Extract header sanitization logic to propagation-getter decorator

6796211

Adds a new decorator to extract header sanitization to propagation getter decorator, for servers that concatenate duplicate headers with `;`. Enables this sanitation logic to be used across propagated state.

Nevay approved these changes Jun 11, 2022

View reviewed changes

brettmc merged commit 90edbd3 into open-telemetry:main Jun 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix W3C test service #686

Fix W3C test service #686

yuktea commented May 26, 2022

bobstrecansky commented May 26, 2022

yuktea commented May 26, 2022 •

edited

Loading

codecov bot commented May 26, 2022 •

edited

Loading

yuktea commented May 27, 2022 •

edited

Loading

yuktea commented May 28, 2022

bobstrecansky commented May 28, 2022

yuktea commented May 28, 2022 •

edited

Loading

yuktea commented May 31, 2022

bobstrecansky commented May 31, 2022

Nevay commented May 31, 2022

yuktea commented May 31, 2022

yuktea commented Jun 1, 2022 •

edited

Loading

yuktea commented Jun 1, 2022

yuktea commented Jun 2, 2022

bobstrecansky commented Jun 2, 2022

yuktea commented Jun 3, 2022

yuktea commented Jun 3, 2022

yuktea commented Jun 3, 2022 •

edited

Loading

yuktea commented Jun 8, 2022

Fix W3C test service #686

Fix W3C test service #686

Conversation

yuktea commented May 26, 2022

bobstrecansky commented May 26, 2022

yuktea commented May 26, 2022 • edited Loading

codecov bot commented May 26, 2022 • edited Loading

Codecov Report

yuktea commented May 27, 2022 • edited Loading

yuktea commented May 28, 2022

bobstrecansky commented May 28, 2022

yuktea commented May 28, 2022 • edited Loading

yuktea commented May 31, 2022

bobstrecansky commented May 31, 2022

Nevay commented May 31, 2022

yuktea commented May 31, 2022

yuktea commented Jun 1, 2022 • edited Loading

yuktea commented Jun 1, 2022

yuktea commented Jun 2, 2022

bobstrecansky commented Jun 2, 2022

yuktea commented Jun 3, 2022

yuktea commented Jun 3, 2022

yuktea commented Jun 3, 2022 • edited Loading

yuktea commented Jun 8, 2022

yuktea commented May 26, 2022 •

edited

Loading

codecov bot commented May 26, 2022 •

edited

Loading

yuktea commented May 27, 2022 •

edited

Loading

yuktea commented May 28, 2022 •

edited

Loading

yuktea commented Jun 1, 2022 •

edited

Loading

yuktea commented Jun 3, 2022 •

edited

Loading