Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

W3C Trace propagator tests - switch to level 1 #4937

Merged
merged 12 commits into from
Nov 1, 2023
+3 −4
Merged

W3C Trace propagator tests - switch to level 1 #4937

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e785a3a
Fix test_tracestate_duplicated_keys
Kielek Oct 4, 2023
f07f636
Merge branch 'main' into fix-test_tracestate_duplicated_keys
Kielek Oct 12, 2023
cb2ecd8
Merge branch 'main' into fix-test_tracestate_duplicated_keys
utpilla Oct 13, 2023
705cecd
Merge branch 'main' into fix-test_tracestate_duplicated_keys
utpilla Oct 17, 2023
a4221b2
Merge branch 'main' into fix-test_tracestate_duplicated_keys
utpilla Oct 17, 2023
6788d16
Update CHANGELOG.md
Kielek Oct 17, 2023
d7ad2c4
Merge branch 'main' into fix-test_tracestate_duplicated_keys
Kielek Oct 18, 2023
d1ee774
Merge branch 'main' into fix-test_tracestate_duplicated_keys
Kielek Oct 19, 2023
2a02548
Merge branch 'main' into fix-test_tracestate_duplicated_keys
Kielek Oct 27, 2023
b4faa7c
Merge branch 'main' into fix-test_tracestate_duplicated_keys
Kielek Oct 27, 2023
f6b2d2a
W3Trace propagator tests - switch to level 1
Kielek Oct 27, 2023
904935a
Merge branch 'main' into fix-test_tracestate_duplicated_keys
utpilla Nov 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/OpenTelemetry.Api/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ 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
Expand Down
12 changes: 3 additions & 9 deletions src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ internal static bool TryExtractTracestate(string[] tracestateCollection, out str

if (tracestateCollection != null)
{
var keySet = new HashSet<string>();
int validEntries = 0;
var result = new StringBuilder();
for (int i = 0; i < tracestateCollection.Length; ++i)
{
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()))
Copy link
Contributor

@utpilla utpilla Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/w3c/trace-context/blob/551a5b0855171281e98b4c2a814bf9e1f837cd53/test/test.py#L565-L567

expects the tracestate to be inherited, and the duplicated keys to be either kept as-is or one of them to be discarded

Could we discard the duplicate ones instead of keeping them? We are only allowed 32 keys. Why waste them by storing duplicates when we could make room for other unique keys?

Copy link
Contributor Author

@Kielek Kielek Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried to answer in PR description on it what was the reason for the change:

Duplicated keys are fully propagated now. It allows to drop creating HashSet for each propagation execution. Now only counter is created.

If you think that it is worth to keep the checj, I can update this PR.

Copy link
Contributor

@utpilla utpilla Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I think we could just keep the key as-is in favor of performance as this would be exectued in the hot-path.

@alanwest @CodeBlanch @vishweshbankwar It would be great to know your thoughts as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am good to keep this as spec allows it.
Not for this PR - It would be good to check how other languages are doing it (would be good to keep consistent behavior)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java is doing similar things, at the beginning checking if there is more/less than 32 attributes https://github.com/open-telemetry/opentelemetry-java/blob/57d83344178f578eb5d2f043b0ffc5c42b6719a5/api/all/src/main/java/io/opentelemetry/api/trace/propagation/internal/W3CTraceContextEncoding.java#L43-L45

Go-lang has similar issue like here, before changes: https://github.com/open-telemetry/opentelemetry-go/blob/c047088605b454f172765621d53107f80b1e6417/trace/tracestate.go#L123-L126

Python is checking duplicates: https://github.com/open-telemetry/opentelemetry-python/blob/34d11d59e9b37052eb3aa6706288641ecb3056e7/opentelemetry-api/src/opentelemetry/trace/span.py#L231-L240

I do not think that there we will be able to find common solution.

{
// test_tracestate_duplicated_keys
return false;
}

if (result.Length > 0)
{
result.Append(',');
}

result.Append(listMember.ToString());
validEntries++;
}
}

Expand Down
4 changes: 2 additions & 2 deletions test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/W3CTraceContextTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
8 changes: 4 additions & 4 deletions test/OpenTelemetry.Tests/Trace/Propagation/TraceContextPropagatorTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Copy link
Member

@alanwest alanwest Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the correct behavior or is the behavior in @pellared's PR for opentelemetry-go correct? That is:

Assert.Equal("foo=1,foo=1", CallTraceContextPropagator("foo=1"));
Assert.Equal("foo=1,foo=2", CallTraceContextPropagator("foo=1"));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, nevermind. After speaking with @utpilla, sounds like both behaviors are correct and left up to the implementation.

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]
Expand Down