Use AsyncLocal for RequestContext #2961

dVakulen · 2017-04-20T20:51:04Z

As target framework version was upgraded to .Net 4.6.1 now it's possible to remove NETSTANDARD ifdefs from RequestContext.

jdom · 2017-04-21T17:55:07Z

I think this is a good idea. Nevertheless, I'm not so sure about no longer honoring Trace.CorrelationManager.ActivityId since that one is not just an implementation detail, and people set that directly.
What are your thoughts about it?

dVakulen · 2017-04-21T20:08:05Z

As AsyncLocal activity id is the only way of storing it while running on .Net core - it should become the default, and on full .Net we can use Trace.CorrelationManager.ActivityId in case of empty AsyncLocal one.

…cLocal one

dVakulen · 2017-04-24T17:29:10Z

@jdom Added commit that does fallback to Trace.CorrelationManager.ActivityId in case of empty AsyncLocal one : c4b726b.
Will add test if this is acceptable approach.

jdom

Yup, I think this one is a safer approach.
Once we convert the vNext tree to the main tree (post 1.5.0 I think), we can move this fallback logic into PlatformServices, so when running Orleans 2.0 in full .NET, this behavior is preserved, even if Orleans itself is NETSTANDARD.

jdom · 2017-04-24T17:58:16Z

src/Orleans/Runtime/RequestContext.cs

+
+                object legacyActivityIdObj;
+#if !NETSTANDARD
+                if ((Guid)activityIdObj == Guid.Empty && contextData.TryGetValue(E2_E_TRACING_LEGACY_ACTIVITY_ID_HEADER, out legacyActivityIdObj))


I would do this inside the previous if, before assigning it to Guid.Empty

jdom · 2017-04-24T18:53:36Z

Awesome, thanks! BTW, please add a test for it, as you suggested

jdom · 2017-04-24T21:28:52Z

src/Orleans/Runtime/RequestContext.cs

-                Trace.CorrelationManager.ActivityId = (Guid)activityIdObj;
+                    object legacyActivityIdObj;
+#if !NETSTANDARD
+                    if (contextData.TryGetValue(E2_E_TRACING_LEGACY_ACTIVITY_ID_HEADER, out legacyActivityIdObj))


The reason for the test failures is that contextData can be null, so this throws

jdom · 2017-04-24T21:30:46Z

src/Orleans/Runtime/RequestContext.cs

@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
+using System.Threading;
 #if !NETSTANDARD
 using System.Runtime.Remoting.Messaging;
 #else


check this ifdefing (you are adding System.Threading two times, one outside the if, and one inside the else)

jdom · 2017-04-24T21:38:57Z

src/Orleans/Runtime/RequestContext.cs

 #endif
+                }
+
+                ActivityId.Value = (Guid)activityIdObj;


Most likely this is a non-issue, but could this cause unnecessary perf impact if the user is never setting the ActivityId in his code (but we set it always to Guid.Empty)? Probably not a concern in terms of memory pressure, but not sure about how expensive is to lift this value when scheduling tasks.
/cc @ReubenBond

It's not expensive from memory standpoint, but ExecutionContext is optimized for no values case, so flowing of that value will indeed take some CPU time, but I doubt that it could be seen on performance tests. Anyway, fixed.

jdom · 2017-04-24T23:48:39Z

src/Orleans/Runtime/RequestContext.cs

-#else
-                Trace.CorrelationManager.ActivityId = (Guid)activityIdObj;
+                    object legacyActivityIdObj = null;
+#if !NETSTANDARD


super-nit: I assume this generates a warning in NETSTANDARD, since you are initializing legacyActivityIdObj but never using it. Just move it inside the #if

jdom · 2017-04-24T23:51:06Z

src/Orleans/Runtime/RequestContext.cs

+                }
+                else
+                {
+                    ActivityId.Value = (Guid)activityIdObj;


I suppose this is enough not to set ActivityId if unused. Do we want to set it to Guid.Empty if ActivityId.Value is already set to something else though?
I believe the answer is no, and we should keep the code as-is, but wanted to ask you in case you did not consider that case and you actually have a different opinion about it.

I strongly suspect that it will be empty in that cases, as local values ExecutionContext does not flow up to parent task as can be seen at https://github.com/dotnet/corefx/blob/b04757c307eee37dcc2b887f0b5ba08d7e866cc1/src/System.Threading/tests/AsyncLocalTests.cs#L255 and RequestContext is being imported only in tasks that is created by Orleans per request, thus ensuring that non-empty value can not end up in that place. (+ in some places in streaming, which I didn't reviewed thoroughly, but they also seems to be safe). But to be on completely safe side, as retrieving of empty value does not incurs much cost (value retrieving code, values container class that is optimized for
common case of no values) I've just added check for non-empty async local activity id value to set it to default value.

Hehe, we think alike. I did the same analysis as you and draw the same conclusion: it's probably empty in most cases since it doesn't flow up, but still it wouldn't harm to do the check (although I would have been fine if we didn't do the check too, since it's a very rare edge condition).

jdom · 2017-04-25T17:48:19Z

LGTM, I'll merge when the tests complete

jdom · 2017-04-25T21:18:53Z

src/Orleans/Runtime/RequestContext.cs

@@ -36,13 +32,13 @@ public static class RequestContext

        internal const string CALL_CHAIN_REQUEST_CONTEXT_HEADER = "#RC_CCH";
        internal const string E2_E_TRACING_ACTIVITY_ID_HEADER = "#RC_AI";
+        internal const string E2_E_TRACING_LEGACY_ACTIVITY_ID_HEADER = "#L_RC_AI";
        internal const string ORLEANS_REQUEST_CONTEXT_KEY = "#ORL_RC";


This is no longer used. But no issue, I'll delete it afterwards, merging for now

jdom · 2017-04-25T21:22:47Z

test/Tester/TestUtils.cs

@@ -114,20 +114,12 @@ public static class RequestContextTestUtils
    {
        public static void SetActivityId(Guid id)
        {
-#if NETSTANDARD
            RequestContext.ActivityId.Value = id;


Since we are now officializing this approach (previously it was just a temp workaround for .NET Standard, so I didn't nit pick on it), it would be nice to not expose the AsyncLocal field, but instead have a wrapper property, so this would look like:
RequestContext.ActivityId = id

Same with the CallContextData field.

BTW, looking closer, should we just dump the ActivityId as a separate AsyncLocal field? Looks like we are putting it in the same dictionary as CallContextData when flowing it, and there is no inherent reason to have it separate. The reason some of it was separate, is that we were also augmenting and flowing the Trace.CorrelationManager.ActivityId, which is an external property that people might have already been using.

We can of course have a property in RequestContext so that people don't have to use a well-known header for it, such as:

/// <summary>Gets or sets an activity ID that can be used for correlation</summary> public static Guid ActivityId { get { return (Guid)(Get(E2_E_TRACING_ACTIVITY_ID_HEADER) ?? Guid.Empty); } set { Set(E2_E_TRACING_ACTIVITY_ID_HEADER, value); } }

Created a PR against your branch (dVakulen#8), let me know what you think. I believe it greatly simplifies the ActivityID API (and impl), especially for end users of both .NET and .NET Standard.

Yeah, the reason it was extracted into separate variable was to save us from more conditional compilation statements + no dictionary allocations if ActivityId is the only value to be stored there.

Simplify handling and fallbacks of ActivityIDs to something similar to how it was before the move to AsyncLocal. Add RequestContext.ActivityId property for ease of flowing an end-to-end activity ID even in .NET Standard, where Trace.CorrelationManager.ActivityId does not exist. Avoid setting an explicit null value if the AsyncLocal is already null when clearing.

Remove separate AsyncLocal for ActivityId

jdom · 2017-04-27T16:33:07Z

Thanks a lot @dVakulen!

dVakulen · 2017-04-27T18:50:34Z

Thank you too, @jdom.

As side note - while it's indeed cool to receive thumb ups, I don't quite get why this one has got so much of them (compared to other PR's).

ReubenBond · 2017-04-27T22:13:38Z

@dVakulen the results from, eg #2060 look amazing (up to 81% perf improvement!), I think the thumbs up here are because we've each personally dealt with differences between .NET Standard & full .NET - there were some hard times there... so we're just so happy to see convergence.

jdom · 2017-04-27T22:27:17Z

yup, thumbs up are because we are starting to converge to a single source tree for 2.0. The 3 of us that gave thumbs up are the ones that have being dealing with the differences in the codebases for several months. So after the 4.6.1 merge we were already planning to do this work (and other NETSTANDARD_TODO ones), so you coming and fixing it for us is all goodness.
All PRs are useful. Some are not in our main priority, so while good, they still add work for us in the form of reviews and testing, that we have to prioritize high to keep the contributions coming. This one is good, but also aligns with our priority, so it removes work from us. Sure, we have to review it, but you did take the time to do most of the heavy lifting and testing.

dVakulen added 3 commits April 20, 2017 22:19

Merge remote-tracking branch 'refs/remotes/dotnet/master' into HEAD

e10330f

Use async local for request context

4c1521b

Formatting

2d79525

dnfclas added the cla-already-signed label Apr 20, 2017

Fix tests

5ce52c7

dVakulen changed the title ~~Use async local for request context~~ [WIP] Use async local for request context Apr 21, 2017

Remove NetStandart ifdefs from test grains

e129544

Fallback to Trace.CorrelationManager.ActivityId in case of empty Asyn…

c4b726b

…cLocal one

jdom reviewed Apr 24, 2017

View reviewed changes

dVakulen added 2 commits April 24, 2017 21:45

Address feedback

9f07692

Address feedback

cb2a0f4

dVakulen added 2 commits April 24, 2017 21:59

Add legacy activityId test

5b0e628

Add BTV test category

dc6a2cd

jdom reviewed Apr 24, 2017

View reviewed changes

jdom changed the title ~~[WIP] Use async local for request context~~ Use AsyncLocal for RequestContext Apr 24, 2017

dVakulen added 2 commits April 25, 2017 01:23

Fix null reference exception in accessing of contextData

5c2f2b7

Fix test grain

1d54954

jdom reviewed Apr 24, 2017

View reviewed changes

jdom approved these changes Apr 24, 2017

View reviewed changes

Address super-nits

32a8c92

jdom reviewed Apr 25, 2017

View reviewed changes

jdom and others added 2 commits April 25, 2017 18:22

Merge pull request #8 from jdom/request-context-async-local

b55f42b

Remove separate AsyncLocal for ActivityId

jdom merged commit 647ae06 into dotnet:master Apr 27, 2017

dVakulen deleted the request-context-async-local branch April 27, 2017 18:50

jdom mentioned this pull request Jun 7, 2017

Implementing DiagnosticsSource for cross silo tracing #3093

Closed

github-actions bot locked and limited conversation to collaborators Dec 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use AsyncLocal for RequestContext #2961

Use AsyncLocal for RequestContext #2961

dVakulen commented Apr 20, 2017 •

edited

Loading

jdom commented Apr 21, 2017

dVakulen commented Apr 21, 2017

dVakulen commented Apr 24, 2017 •

edited

Loading

jdom left a comment

jdom Apr 24, 2017

dVakulen Apr 24, 2017 •

edited

Loading

jdom commented Apr 24, 2017

jdom Apr 24, 2017

dVakulen Apr 24, 2017 •

edited

Loading

jdom Apr 24, 2017

dVakulen Apr 24, 2017

jdom Apr 24, 2017

dVakulen Apr 24, 2017

jdom Apr 24, 2017

dVakulen Apr 25, 2017

jdom Apr 24, 2017

dVakulen Apr 25, 2017 •

edited

Loading

jdom Apr 25, 2017

jdom commented Apr 25, 2017

jdom Apr 25, 2017

jdom Apr 25, 2017

jdom Apr 26, 2017 •

edited

Loading

jdom Apr 26, 2017

dVakulen Apr 26, 2017 •

edited

Loading

jdom commented Apr 27, 2017

dVakulen commented Apr 27, 2017

ReubenBond commented Apr 27, 2017

jdom commented Apr 27, 2017

Use AsyncLocal for RequestContext #2961

Use AsyncLocal for RequestContext #2961

Conversation

dVakulen commented Apr 20, 2017 • edited Loading

jdom commented Apr 21, 2017

dVakulen commented Apr 21, 2017

dVakulen commented Apr 24, 2017 • edited Loading

jdom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dVakulen Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

jdom commented Apr 24, 2017

Choose a reason for hiding this comment

dVakulen Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dVakulen Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdom commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdom Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dVakulen Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

jdom commented Apr 27, 2017

dVakulen commented Apr 27, 2017

ReubenBond commented Apr 27, 2017

jdom commented Apr 27, 2017

dVakulen commented Apr 20, 2017 •

edited

Loading

dVakulen commented Apr 24, 2017 •

edited

Loading

dVakulen Apr 24, 2017 •

edited

Loading

dVakulen Apr 24, 2017 •

edited

Loading

dVakulen Apr 25, 2017 •

edited

Loading

jdom Apr 26, 2017 •

edited

Loading

dVakulen Apr 26, 2017 •

edited

Loading