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

ScopeContext - Allow child task to propagate context properties to parent #4643

Open
snakefoot opened this issue Oct 28, 2021 · 3 comments
Open

Comments

@snakefoot
Copy link
Contributor

snakefoot commented Oct 28, 2021

Maybe something like this, where it possible to do this:

using var propertyCollector = ScopeContext.PushPropertyCollector();
ScopeContext.CollectProperty("parentproperty", "value");
ScopeContext.CollectProperty("sharedproperty", "value");
await ChildTaskAsync();   // Pushes properties to ScopeContext
Logger.Info("Includes properties from child task");

async void ChildTaskAsync()
{
     ScopeContext.CollectProperty("childproperty", "value");
     await Task.Delay(1);
     ScopeContext.CollectProperty("sharedproperty", "value2");   // overwrites
}

The properties included in the property-collector should be accessible when using ${scopeproperty} or IncludeScopeProperties = true.

Guess one could manually inject a ConcurrentDictionary as scope-property at the parent-task-scopecontext, and then child-tasks can resolve that scope-property and insert their own propagating-properties into the ConcurrentDictionary. Thus parent-task-scopecontext will see that the scope-property with ConcurrentDictionary has the propagating-values from the child-tasks.

@snakefoot snakefoot changed the title ScopeContext - PushPropertyCollector ScopeContext - Allow child task to propagate context properties to parent Oct 28, 2021
@marcanpilami
Copy link

I'm interested in something like this as well. I see two issues here.

First one is to define the API scope. Both in your sample in in my requirement, we need the API to work in async/await environments, i.e. in a hierarchical tree of execution contexts. That means we must not mix up data from different tasks or threads, and we cannot simply go back until we get to the default root context. So we would likely require an API XXX.BeginScope() (which would return a Disposable to be usable in a using block).

Second issue is implementation. Usually this "no mixup" means an AsyncLocal object - exactly like the MDLC is doing, but AsyncLocal objects only propagate down, not up. And since AsyncLocal objects are copied on capture, there is no way to simply keep a reference to the base context and have new values propagate - this won't update copied snapshots. Finaly, the MDLC actually uses an immutable dictionary, so it's not simply a "pass a reference in the AsyncLocal". My idea here would be to extend the MDLC with a new AsyncLocal dictionary, which would not be immutable. Basically the same dictionary would be kept for all sub contexts - contrary to the normal MDLC dictionary which is different for each sub context. Items from normal MLDC dictionary & new dictionary would be merged when used by layouts.

This would also allow to have nested scopes. In that case, existing scope should be copied. In this end this is allowing control on MDLC snapshots - today they are taken once per value set (which means at least once per context where a value changes), this new API would allow to take the snapshot at will.

This could result in:

using(MappedDiagnosticsLogicalContext.BeginSharedScope())
{
	MappedDiagnosticsLogicalContext.SetSharedScope("key1", "value1");
	Console.WriteLine(MappedDiagnosticsLogicalContext.Get("key1")); // => value1
	await Task.Run(() => MappedDiagnosticsLogicalContext.SetSharedScope("key1", "value2"));
	Console.WriteLine(MappedDiagnosticsLogicalContext.Get("key1")); // => value2
}

I am not an nlog committer at all, but I would be interested in creating a PR should someone in the know agree/discuss/amend... my proposition.

@snakefoot
Copy link
Contributor Author

snakefoot commented Oct 29, 2021

Sounds like we are on the same page, and have recognized the issue with having multiple-child-tasks to the same parent-task, and the issues with concurrency.

using(MappedDiagnosticsLogicalContext.BeginSharedScope())

Is the same as my ScopeContext.PushPropertyCollector();

MappedDiagnosticsLogicalContext.SetSharedScope("key1", "value2")

Is the same as my ScopeContext.CollectProperty("sharedproperty", "value2");

but I would be interested in creating a PR

Sounds great. Notice that MappedDiagnosticsLogicalContext and NestedDiagnosticsLogicalContext has been merged into
ScopeContext with NLog 5.0 (To better match MEL BeginScope). But I think it is best that you create your own isolated AsyncLocal for this feature, and when the API is done and the unit-test are green, then we can look at merging it into the existing ScopeContext AsyncLocal. You should only focus on the AsyncLocal implementation and ignore NET35 + NET45 that uses remoting.

@marcanpilami
Copy link

marcanpilami commented Nov 2, 2021

OK, thanks for the input. I had indeed not seen all the changes in version 5 and would have missed why you were taking about ScopeContext. I'll submit something this week I hope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants