Change how EndSegment is called for Tasks

[bgrainger]

Description

If the Task has already completed, we call EndSegment (synchronously) immediately. Otherwise, we always continue with TaskContinuationOptions.ExecuteSynchronously on the default TaskScheduler, which runs our continuation as soon as possible (on the thread that completed the task).

This avoids task continuations being delayed until much later in the ASP.NET pipeline (due to when AspNetSynchronizationContext posts them), which causes inaccurate timings.

Testing

Tested in prod as per #85 (comment).

[newrelicbot]

Thank you for contributing to the .NET Agent!

A member of our team will be around to take a look shortly and kick off a build.

[bgrainger]

Should this PR also revert this code?

newrelic-dotnet-agent/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/StackExchangeRedis/ExecuteAsyncImplWrapper.cs

Lines 41 to 49 in 03d6ca8

	//We're not using Delegates.GetAsyncDelegateFor(agent, segment) because if an async redis call is made from an asp.net mvc action,
	//the continuation may not run until that mvc action has finished executing, or has yielded execution, because the synchronization context
	//will only allow one thread to execute at a time. To work around this limitation, since we don't need access to HttpContext in our continuation,
	//we can just provide the TaskContinuationOptions.HideScheduler flag so that we will use the default ThreadPool scheduler to schedule our
	//continuation. Using the ThreadPool scheduler allows our continuation to run without needing to wait for the mvc action to finish executing or
	//yielding its execution. We're not applying this change across the board, because we still need to better understand the impact of making this
	//change more broadly vs just fixing a known customer issue.
	return Delegates.GetAsyncDelegateFor<Task>(agent, segment, TaskContinuationOptions.ExecuteSynchronously | TaskContinuationOptions.HideScheduler);

[nrcventura]

Should this PR also revert this code?

newrelic-dotnet-agent/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/StackExchangeRedis/ExecuteAsyncImplWrapper.cs

Lines 41 to 49 in 03d6ca8

	//We're not using Delegates.GetAsyncDelegateFor(agent, segment) because if an async redis call is made from an asp.net mvc action,
	//the continuation may not run until that mvc action has finished executing, or has yielded execution, because the synchronization context
	//will only allow one thread to execute at a time. To work around this limitation, since we don't need access to HttpContext in our continuation,
	//we can just provide the TaskContinuationOptions.HideScheduler flag so that we will use the default ThreadPool scheduler to schedule our
	//continuation. Using the ThreadPool scheduler allows our continuation to run without needing to wait for the mvc action to finish executing or
	//yielding its execution. We're not applying this change across the board, because we still need to better understand the impact of making this
	//change more broadly vs just fixing a known customer issue.
	return Delegates.GetAsyncDelegateFor<Task>(agent, segment, TaskContinuationOptions.ExecuteSynchronously | TaskContinuationOptions.HideScheduler);

That's a good question. I think that I still have the test app that I used to verify that fix so I can include that in the additional timing tests we want to do to try to confirm that some previously problematic scenarios are still working as expected. Unfortunately, we haven't come up with a good way to automate these types of timing tests yet.

[nrcventura]

I'm still working my way through testing the timing of some of our previously problematic async scenarios, but I was able to test that the Redis instrumentation, with the changes in this PR, can go back to using return Delegates.GetAsyncDelegateFor<Task>(agent, segment);. Although, my preference is to have that done in a separate clean up PR, because I suspect that there are a few other changes that can be made to better standardize the way we handle these continuations. I think that we might eventually be able to get rid of the else if (options == TaskContinueWithOption.None) block, and then reduce the number of GetAsyncDelegateFor overloads that we have.

[nrcventura]

ci test this please

[nrcventura]

The timings for many of our async scenarios look good. Not only will this fix a few problems, but it helps pave the way forward for simplifying/standardizing our async timing. Great job identifying this!

[bgrainger]

This affects our DB timings in production, so I'm anxious to get this fix. However, I'd prefer to run the "official" NR Agent on our infrastructure rather than a version I built locally.

Do you have a rough expectation of when this updated code might ship? (I'm not asking for a firm promise of a release date, but just want to understand when it's likely we'd be able to upgrade to this. I don't even know if you've shipped an agent built from this OSS code base yet, so I understand that things might take longer than expected.)

[nrcventura]

We have a few things queued up for a release, and were waiting to kick off our release process until this was merged to better support your use case. We plan on possibly kicking off our release process tomorrow (9/16). This is definitely an area where we can improve our transparency, and can learn from other open source projects.

[nrcventura]

We had a minor issue come up during our .NET 5 RC 1 validation that's delaying the release a little, but we still plan on running our release process soon.