Fix tracing regression #707
Conversation
Codecov Report
@@ Coverage Diff @@
## main #707 +/- ##
==========================================
+ Coverage 89.96% 91.56% +1.60%
==========================================
Files 60 60
Lines 5162 5182 +20
==========================================
+ Hits 4644 4745 +101
+ Misses 518 437 -81
Continue to review full report at Codecov.
|
In microsoft#697 we started using formatter-specific JsonRpcRequest objects on the transmitting side. This causes RPC requests with arguments to fail when ETW tracing is enabled because they try to read arguments using code that was only designed to do so with deserialized messages rather than the one created to be transmitted. The fix is to avoid trying to deserialize outbound arguments. We could have done an `if` check within the `JsonRpcRequest`-derived classes, but it seemed more complete and safe to just stop reusing classes that were written only with deserialization in mind by creating a far simpler outbound-only request class.
| [IgnoreDataMember] | ||
| public TopLevelPropertyBag? TopLevelPropertyBag { get; set; } | ||
|
|
||
| public override bool TrySetTopLevelProperty<T>(string name, [MaybeNull] T value) |
There was a problem hiding this comment.
Now that inbound/outbound is separate, do we need to implement TrySetTopLevelProperty on the inbound object?
There was a problem hiding this comment.
Maybe not. But inbound messages become outbound messages in the remote target relay case. That isn't adequately tested, I'm learning, so I didn't want to risk regressing it.
There was a problem hiding this comment.
Right, I remember that case now from unit tests. Does the relay case only work with json messages or do we support with message pack as well? I am now curious if it would work with messagepack and top level properties since we had different inbound/outbound dictionaries in message pack case.
|
|
||
| public TopLevelPropertyBag? TopLevelPropertyBag { get; set; } | ||
|
|
||
| public override bool TrySetTopLevelProperty<T>(string name, [MaybeNull] T value) |
There was a problem hiding this comment.
Same comment as the Json requests messages here regarding TrySetTopLevelProperty implementation in the inbound one
There was a problem hiding this comment.
Same response, but especially true here, since remote RPC target tests don't run on messagepack at all.
In #697 we started using formatter-specific
JsonRpcRequestobjects on the transmitting side. This causes RPC requests with arguments to fail when ETW tracing is enabled because they try to read arguments using code that was only designed to do so with deserialized messages rather than the one created to be transmitted.The fix is to avoid trying to deserialize outbound arguments. We could have done an
ifcheck within theJsonRpcRequest-derived classes, but it seemed more complete and safe to just stop reusing classes that were written only with deserialization in mind by creating a far simpler outbound-only request class.We had a test hole in the ETW case which this PR (and #706) fills.