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.

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

Fix activity tracing to track entire execution time of inbound calls #763

Merged
merged 1 commit into from
Jan 29, 2022

Conversation

AArnott
Copy link
Member

@AArnott AArnott commented Jan 28, 2022

The incoming RPC was getting an activity that bound only the time to invoke the server method and get its result back. But if the method were async, terminating the activity at that point meant the timestamp on the end of the activity would be premature. Instead, an async server method should mean the activity does not end until the returned Task completes. This makes it clear that child activities and other messages logged by the server method belong to that activity.

As shown in the Service Trace Viewer:

Before
image

After
image

@AArnott AArnott added the bug label Jan 28, 2022
@AArnott AArnott added this to the v2.11 milestone Jan 28, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #763 (0017e26) into main (5e740fc) will increase coverage by 0.20%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #763      +/-   ##
==========================================
+ Coverage   91.54%   91.75%   +0.20%     
==========================================
  Files          60       61       +1     
  Lines        5182     5323     +141     
==========================================
+ Hits         4744     4884     +140     
- Misses        438      439       +1     
Impacted Files Coverage Δ
src/StreamJsonRpc/JsonRpc.cs 94.03% <ø> (-0.10%) ⬇️
src/StreamJsonRpc/ActivityTracingStrategy.cs 95.45% <0.00%> (-4.55%) ⬇️
...nRpc/Reflection/MessageFormatterProgressTracker.cs 93.54% <0.00%> (-1.26%) ⬇️
src/StreamJsonRpc/Reflection/RpcTargetInfo.cs 92.57% <0.00%> (-0.20%) ⬇️
src/StreamJsonRpc/JsonRpcTargetOptions.cs 100.00% <0.00%> (ø)
...StreamJsonRpc/Reflection/JsonRpcMethodAttribute.cs 100.00% <0.00%> (ø)
src/StreamJsonRpc/ExceptionSettings.cs 100.00% <0.00%> (ø)
src/StreamJsonRpc/MessagePackFormatter.cs 92.51% <0.00%> (+0.30%) ⬆️
src/StreamJsonRpc/Resources.Designer.cs 67.50% <0.00%> (+0.41%) ⬆️
... and 4 more

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 5e740fc...0017e26. Read the comment docs.

@AArnott AArnott merged commit 8529cac into microsoft:main Jan 29, 2022
@AArnott AArnott deleted the activitytracing branch January 29, 2022 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants