Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an option to serialize the W3C traceparent field as a plain string (instead of StreamJsonRpc’s compact binary MessagePack encoding) for both MessagePackFormatter and NerdbankMessagePackFormatter, and adds tests asserting the chosen serialization token type.
Changes:
- Add
TraceParentAsW3CStringinit-only option to both MessagePack formatters and route traceparent serialization through string/binary-specific converters/formatters. - Refactor
TraceParentto expose a fixed serialized length constant and aWriteTo(Span<char>)helper. - Add unit tests validating the on-the-wire MessagePack token type for
traceparentunder both settings.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/StreamJsonRpc.Tests/NerdbankMessagePackFormatterTests.cs | Adds a theory asserting traceparent serializes as Array vs String based on TraceParentAsW3CString. |
| test/StreamJsonRpc.Tests/MessagePackFormatterTests.cs | Adds the same serialization-format assertion for MessagePackFormatter. |
| src/StreamJsonRpc/Protocol/TraceParent.cs | Refactors string parsing/serialization helpers; introduces Length and WriteTo. |
| src/StreamJsonRpc/Polyfills.cs | Consolidates polyfills and adds Encoding.TryGetChars extension for non-NET TFMs. |
| src/StreamJsonRpc/PolyfillMethods.cs | Removes now-redundant polyfill type (moved into Polyfills). |
| src/StreamJsonRpc/NerdbankMessagePackFormatter.TraceParentConverter.cs | Splits traceparent converter into binary vs string variants. |
| src/StreamJsonRpc/NerdbankMessagePackFormatter.cs | Adds TraceParentAsW3CString option and uses it for traceparent serialization. |
| src/StreamJsonRpc/MessagePackFormatter.cs | Adds TraceParentAsW3CString option, new traceparent formatters, and delegates traceparent serialization through them. |
Comments suppressed due to low confidence (2)
src/StreamJsonRpc/NerdbankMessagePackFormatter.cs:488
- Deserialization of traceparent/tracestate is still format-dependent: by default TraceParentConverter expects the compact binary array format, so incoming W3C strings (the scenario in #1444) will still fail unless the receiver opts into TraceParentAsW3CString. If this PR is meant to close #1444, these reads should accept both array and string encodings by inspecting reader.NextMessagePackType (and similarly accept string tracestate).
else if (TraceParentPropertyName.TryRead(ref reader))
{
TraceParent traceParent = formatter.TraceParentConverter.Read(ref reader, context);
result.TraceParent = traceParent.ToString();
}
else if (TraceStatePropertyName.TryRead(ref reader))
{
result.TraceState = ReadTraceState(ref reader, context);
}
src/StreamJsonRpc/MessagePackFormatter.cs:1547
- JsonRpcRequest deserialization uses TraceParentFormatter/ReadTraceState unconditionally, so default settings still reject W3C string-encoded traceparent/tracestate (the interoperability issue described in #1444). If this PR closes #1444, these reads should accept both array and string encodings by switching on reader.NextMessagePackType; keep TraceParentAsW3CString as a serialization preference.
else if (TraceParentPropertyName.TryRead(stringKey))
{
TraceParent traceParent = this.formatter.TraceParentFormatter.Deserialize(ref reader, options);
result.TraceParent = traceParent.ToString();
}
else if (TraceStatePropertyName.TryRead(stringKey))
{
result.TraceState = ReadTraceState(ref reader, options);
}
This property causes the msgpack formatters to write out the `traceparent` as a W3C string instead of the more compact binary format we've been using. Closes #1444
drewnoakes
approved these changes
Jun 1, 2026
Member
drewnoakes
left a comment
There was a problem hiding this comment.
Thanks for fixing this so quickly Andrew.
LGTM. Could add some tests covering parsing failures.
RyanToth3
approved these changes
Jun 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1444