From e785a3ace8442bfaf11cf7f8fe964d99fb68ce22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Kie=C5=82kowicz?= Date: Wed, 4 Oct 2023 20:11:28 +0200 Subject: [PATCH 1/3] Fix test_tracestate_duplicated_keys --- src/OpenTelemetry.Api/CHANGELOG.md | 3 +++ .../Context/Propagation/TraceContextPropagator.cs | 12 +++--------- .../W3CTraceContextTests.cs | 4 ++-- .../Trace/Propagation/TraceContextPropagatorTest.cs | 8 ++++---- 4 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/OpenTelemetry.Api/CHANGELOG.md b/src/OpenTelemetry.Api/CHANGELOG.md index 6d47071c7e3..4fdffec74ce 100644 --- a/src/OpenTelemetry.Api/CHANGELOG.md +++ b/src/OpenTelemetry.Api/CHANGELOG.md @@ -14,6 +14,9 @@ `trace-flags` field of the `traceparent` header. ([#4893](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4893)) +* Fix `TraceContextPropagator` by propagating duplicated `tracestate` entries. + ([#4937](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4937)) + ## 1.6.0 Released 2023-Sep-05 diff --git a/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs b/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs index 16c2456ad4a..0cdc03e5f7d 100644 --- a/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs +++ b/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs @@ -235,7 +235,7 @@ internal static bool TryExtractTracestate(string[] tracestateCollection, out str if (tracestateCollection != null) { - var keySet = new HashSet(); + int validEntries = 0; var result = new StringBuilder(); for (int i = 0; i < tracestateCollection.Length; ++i) { @@ -264,7 +264,7 @@ internal static bool TryExtractTracestate(string[] tracestateCollection, out str continue; } - if (keySet.Count >= 32) + if (validEntries >= 32) { // https://github.com/w3c/trace-context/blob/master/spec/20-http_request_header_format.md#list // test_tracestate_member_count_limit @@ -294,19 +294,13 @@ internal static bool TryExtractTracestate(string[] tracestateCollection, out str return false; } - // ValidateKey() call above has ensured the key does not contain upper case letters. - if (!keySet.Add(key.ToString())) - { - // test_tracestate_duplicated_keys - return false; - } - if (result.Length > 0) { result.Append(','); } result.Append(listMember.ToString()); + validEntries++; } } diff --git a/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/W3CTraceContextTests.cs b/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/W3CTraceContextTests.cs index 58ec321d600..1effb256171 100644 --- a/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/W3CTraceContextTests.cs +++ b/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/W3CTraceContextTests.cs @@ -103,11 +103,11 @@ public void W3CTraceContextTestSuiteAsync(string value) if (AspNetCoreHostingVersion.Major <= 6) { - Assert.StartsWith("FAILED (failures=5)", lastLine); + Assert.StartsWith("FAILED (failures=4)", lastLine); } else { - Assert.StartsWith("FAILED (failures=2)", lastLine); + Assert.StartsWith("FAILED (failures=1)", lastLine); } } diff --git a/test/OpenTelemetry.Tests/Trace/Propagation/TraceContextPropagatorTest.cs b/test/OpenTelemetry.Tests/Trace/Propagation/TraceContextPropagatorTest.cs index f7dae35547f..089aa248517 100644 --- a/test/OpenTelemetry.Tests/Trace/Propagation/TraceContextPropagatorTest.cs +++ b/test/OpenTelemetry.Tests/Trace/Propagation/TraceContextPropagatorTest.cs @@ -193,10 +193,10 @@ public void Inject_WithTracestate() public void DuplicateKeys() { // test_tracestate_duplicated_keys - Assert.Empty(CallTraceContextPropagator("foo=1,foo=1")); - Assert.Empty(CallTraceContextPropagator("foo=1,foo=2")); - Assert.Empty(CallTraceContextPropagator(new[] { "foo=1", "foo=1" })); - Assert.Empty(CallTraceContextPropagator(new[] { "foo=1", "foo=2" })); + Assert.Equal("foo=1,foo=1", CallTraceContextPropagator("foo=1,foo=1")); + Assert.Equal("foo=1,foo=2", CallTraceContextPropagator("foo=1,foo=2")); + Assert.Equal("foo=1,foo=1", CallTraceContextPropagator(new[] { "foo=1", "foo=1" })); + Assert.Equal("foo=1,foo=2", CallTraceContextPropagator(new[] { "foo=1", "foo=2" })); } [Fact] From 6788d1652ff7620804ebb104f53e9f860aae9f53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Kie=C5=82kowicz?= Date: Tue, 17 Oct 2023 07:20:48 +0200 Subject: [PATCH 2/3] Update CHANGELOG.md --- src/OpenTelemetry.Api/CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry.Api/CHANGELOG.md b/src/OpenTelemetry.Api/CHANGELOG.md index 67b6d19ecc4..4698d0285e8 100644 --- a/src/OpenTelemetry.Api/CHANGELOG.md +++ b/src/OpenTelemetry.Api/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased +* Fix `TraceContextPropagator` by propagating duplicated `tracestate` entries. + ([#4937](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4937)) + ## 1.7.0-alpha.1 Released 2023-Oct-16 @@ -18,9 +21,6 @@ Released 2023-Oct-16 `trace-flags` field of the `traceparent` header. ([#4893](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4893)) -* Fix `TraceContextPropagator` by propagating duplicated `tracestate` entries. - ([#4937](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4937)) - ## 1.6.0 Released 2023-Sep-05 From f6b2d2a8378c80556529b7cebaeac83048f6145b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Kie=C5=82kowicz?= Date: Fri, 27 Oct 2023 13:15:39 +0200 Subject: [PATCH 3/3] W3Trace propagator tests - switch to level 1 --- src/OpenTelemetry.Api/CHANGELOG.md | 3 --- .../Context/Propagation/TraceContextPropagator.cs | 12 +++++++++--- .../Dockerfile | 2 +- .../W3CTraceContextTests.cs | 4 ++-- .../docker-compose.yml | 1 - .../Trace/Propagation/TraceContextPropagatorTest.cs | 8 ++++---- 6 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/OpenTelemetry.Api/CHANGELOG.md b/src/OpenTelemetry.Api/CHANGELOG.md index 96cca2fc44b..67cf8f478ff 100644 --- a/src/OpenTelemetry.Api/CHANGELOG.md +++ b/src/OpenTelemetry.Api/CHANGELOG.md @@ -6,9 +6,6 @@ `8.0.0-rc.2.23479.6`. ([#4959](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4959)) -* Fix `TraceContextPropagator` by propagating duplicated `tracestate` entries. - ([#4937](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4937)) - ## 1.7.0-alpha.1 Released 2023-Oct-16 diff --git a/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs b/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs index 0cdc03e5f7d..16c2456ad4a 100644 --- a/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs +++ b/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs @@ -235,7 +235,7 @@ internal static bool TryExtractTracestate(string[] tracestateCollection, out str if (tracestateCollection != null) { - int validEntries = 0; + var keySet = new HashSet(); var result = new StringBuilder(); for (int i = 0; i < tracestateCollection.Length; ++i) { @@ -264,7 +264,7 @@ internal static bool TryExtractTracestate(string[] tracestateCollection, out str continue; } - if (validEntries >= 32) + if (keySet.Count >= 32) { // https://github.com/w3c/trace-context/blob/master/spec/20-http_request_header_format.md#list // test_tracestate_member_count_limit @@ -294,13 +294,19 @@ internal static bool TryExtractTracestate(string[] tracestateCollection, out str return false; } + // ValidateKey() call above has ensured the key does not contain upper case letters. + if (!keySet.Add(key.ToString())) + { + // test_tracestate_duplicated_keys + return false; + } + if (result.Length > 0) { result.Append(','); } result.Append(listMember.ToString()); - validEntries++; } } diff --git a/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/Dockerfile b/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/Dockerfile index b0c7a87696e..0508036d3e8 100644 --- a/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/Dockerfile +++ b/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/Dockerfile @@ -9,7 +9,7 @@ FROM ubuntu AS w3c #Install git WORKDIR /w3c RUN apt-get update && apt-get install -y git -RUN git clone https://github.com/w3c/trace-context.git +RUN git clone --branch level-1 https://github.com/w3c/trace-context.git FROM mcr.microsoft.com/dotnet/sdk:${BUILD_SDK_VERSION} AS build ARG PUBLISH_CONFIGURATION=Release diff --git a/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/W3CTraceContextTests.cs b/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/W3CTraceContextTests.cs index 1effb256171..aacc918f67a 100644 --- a/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/W3CTraceContextTests.cs +++ b/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/W3CTraceContextTests.cs @@ -103,11 +103,11 @@ public void W3CTraceContextTestSuiteAsync(string value) if (AspNetCoreHostingVersion.Major <= 6) { - Assert.StartsWith("FAILED (failures=4)", lastLine); + Assert.StartsWith("FAILED (failures=3)", lastLine); } else { - Assert.StartsWith("FAILED (failures=1)", lastLine); + Assert.StartsWith("OK", lastLine); } } diff --git a/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/docker-compose.yml b/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/docker-compose.yml index ae3253078ce..7c421786f5c 100644 --- a/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/docker-compose.yml +++ b/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/docker-compose.yml @@ -11,4 +11,3 @@ services: command: --TestCaseFilter:CategoryName=W3CTraceContextTests environment: - OTEL_W3CTRACECONTEXT=enabled - - SPEC_LEVEL=2 diff --git a/test/OpenTelemetry.Tests/Trace/Propagation/TraceContextPropagatorTest.cs b/test/OpenTelemetry.Tests/Trace/Propagation/TraceContextPropagatorTest.cs index 089aa248517..f7dae35547f 100644 --- a/test/OpenTelemetry.Tests/Trace/Propagation/TraceContextPropagatorTest.cs +++ b/test/OpenTelemetry.Tests/Trace/Propagation/TraceContextPropagatorTest.cs @@ -193,10 +193,10 @@ public void Inject_WithTracestate() public void DuplicateKeys() { // test_tracestate_duplicated_keys - Assert.Equal("foo=1,foo=1", CallTraceContextPropagator("foo=1,foo=1")); - Assert.Equal("foo=1,foo=2", CallTraceContextPropagator("foo=1,foo=2")); - Assert.Equal("foo=1,foo=1", CallTraceContextPropagator(new[] { "foo=1", "foo=1" })); - Assert.Equal("foo=1,foo=2", CallTraceContextPropagator(new[] { "foo=1", "foo=2" })); + Assert.Empty(CallTraceContextPropagator("foo=1,foo=1")); + Assert.Empty(CallTraceContextPropagator("foo=1,foo=2")); + Assert.Empty(CallTraceContextPropagator(new[] { "foo=1", "foo=1" })); + Assert.Empty(CallTraceContextPropagator(new[] { "foo=1", "foo=2" })); } [Fact]