remove RX as the primary pipe for gRPC communications #8281

mgravell · 2022-04-04T13:53:56Z

Issue describing the changes in this PR

resolves #8280; performance locally is a 5x+ speedup in throughput

the main crux of the new flow is described in src/WebJobs.Script.Grpc/Eventing/GrpcEventExtensions.cs, but in short: we establish a per-worker Channel<InboundGrpcEvent> and Channel<OutboundGrpcEvent> to handle the backlog; instead of publishing to RX, we publish to the writer of the relevant queue. Separately, we have an async worker deque data from the queues, and process accordingly. This is much more direct, avoids a lot of RX machinery, and creates isolation between workers.

Pull request checklist

My changes do not require documentation changes
- Otherwise: Documentation issue linked to PR
My changes should not be added to the release notes for the next release
- Otherwise: I've added my notes to release_notes.md
My changes do not need to be backported to a previous version
- Otherwise: Backport tracked by issue/PR #issue_or_pr
I have added all required tests (Unit tests, E2E tests)

Additional information

Additional PR information

remove RX as the primary pipe for gRPC communications

1: GrpcWorkerChannelTests span up duplicate channels 2: suppress write failures (async void)

… TestFunctionRpcService and GrpcWorkerChannelTests to match

fabiocav · 2022-04-29T21:20:55Z

@mgravell apologies for the delay on this. We'll get eyes on this ASAP. Would it be possible to have this rebased to deal with the conflicts?

Thanks!

mgravell · 2022-04-30T10:06:47Z

Will do

…

On Fri, 29 Apr 2022, 22:21 Fabio Cavalcante, ***@***.***> wrote: @mgravell <https://github.com/mgravell> apologies for the delay on this. We'll get eyes on this ASAP. Would it be possible to have this rebased to deal with the conflicts? Thanks! — Reply to this email directly, view it on GitHub <#8281 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEHMENKREBPYEY25NFYVTVHRHEHANCNFSM5SPTV72Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

pragnagopa · 2022-04-30T23:05:46Z

Tagging @safihamid @paulbatum - fyi

@fabiocav / @brettsam - would be good to get test E2E on private stamp before merging this PR

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannelFactory.cs

# Conflicts: # src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs # test/WebJobs.Script.Tests/Workers/Rpc/GrpcWorkerChannelTests.cs # test/WebJobs.Script.Tests/Workers/Rpc/TestFunctionRpcService.cs

mgravell · 2022-05-06T13:19:58Z

@fabiocav I believe that's merged; it looks like some cheese moved around the topic of "function load collections", which made the tests merge a bit messy, but: should work

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

liliankasem · 2022-05-19T20:25:44Z

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

+            // advertise that we support multiple streams, and hint at a number; with this flag, we allow
+            // clients to connect multiple back-hauls *with the same workerid*, and rely on the internal
+            // plumbing to make sure we don't process everything N times
+            initRequest.Capabilities.Add(RpcWorkerConstants.MultiStream, "10"); // TODO: make this configurable


Can we make the number ("10") a readonly constant for now?

Is the TODO for this PR or future work? If future work, can we create a github issue for this and mention it here?

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannelFactory.cs

src/WebJobs.Script.Grpc/Eventing/IGrpcEventManager.cs

src/WebJobs.Script.Grpc/Server/FunctionRpcService.cs

brettsam

I only have very general questions here. I think we should get this run on a private stamp to make sure we have no cold start regressions.

I also want to take one more pass through to think through things like restarts and failures -- this is tricky!

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

src/WebJobs.Script.Grpc/Eventing/IGrpcEventManager.cs

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannelFactory.cs

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

liliankasem · 2022-06-15T00:48:43Z

@mgravell anything we can do to help you get this PR merged?

fabiocav · 2022-06-16T20:31:50Z

@mgravell indeed, this is high priority on our side, so whatever we can do to help, please let us know.

NickCraver · 2022-06-16T20:33:15Z

I looked at this earlier on a call - it looks like the main pending item is to test regressions in cold start and not code tidy here as the blocker, or am I missing a thread where that's already in play? Happy to help on the code side.

liliankasem · 2022-06-16T20:45:31Z

I looked at this earlier on a call - it looks like the main pending item is to test regressions in cold start and not code tidy here as the blocker, or am I missing a thread where that's already in play? Happy to help on the code side.

I think it's a mix of both, would be great to get PR feedback addressed but not a huge blocker. @safihamid and @pragnagopa can you help with cold start testing?

safihamid · 2022-06-16T22:21:42Z

I looked at this earlier on a call - it looks like the main pending item is to test regressions in cold start and not code tidy here as the blocker, or am I missing a thread where that's already in play? Happy to help on the code side.

I think it's a mix of both, would be great to get PR feedback addressed but not a huge blocker. @safihamid and @pragnagopa can you help with cold start testing?

Ok I will get cold start comparison numbers for this by end of next week. I hope most PR feedbacks are addressed by then.

Co-authored-by: Lilian Kasem <likasem@microsoft.com>

…unctions-host into mgravell/remove-rx

INCOMPLETE - still 2 new failing tests # Conflicts: # src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs # test/WebJobs.Script.Tests/Workers/Rpc/GrpcWorkerChannelTests.cs

mgravell · 2022-06-21T15:35:20Z

Merge is in progress; fighting SendInvocationRequest_ValidateTraceContext and SendInvocationRequest_ValidateTraceContext_SessionId currently

safihamid · 2022-06-22T20:46:24Z

No noticeable cold start changes with this PR in my private stamp as long as we are using the right language worker during placeholder mode. i.e. use Node placeholder for Node app, ...

NickCraver · 2022-07-27T02:28:15Z

Most failures here are due to more merges since the PR - 1 left I couldn't get through tonight:

[xUnit.net 00:03:48.32]     Microsoft.Azure.WebJobs.Script.Tests.WorkerConcurrencyManagerEndToEndTests.WorkerStatus_NewWorkerAdded [FAIL]
  Failed Microsoft.Azure.WebJobs.Script.Tests.WorkerConcurrencyManagerEndToEndTests.WorkerStatus_NewWorkerAdded [2 m]
  Error Message:
   System.ApplicationException : Condition not reached within timeout.
  Stack Trace:
     at Microsoft.Azure.WebJobs.Script.Tests.TestHelpers.Await(Func`1 condition, Int32 timeout, Int32 pollingInterval, Boolean throwWhenDebugging, Func`1 userMessageCallback) in /_/test/WebJobs.Script.Tests.Shared/TestHelpers.cs:line 115
   at Microsoft.Azure.WebJobs.Script.Tests.WorkerConcurrencyManagerEndToEndTests.WorkerStatus_NewWorkerAdded() in /_/test/WebJobs.Script.Tests.Integration/Rpc/WorkerConcurrencyManagerEndToEndTests.cs:line 28
--- End of stack trace from previous location ---
fail: Host.General[515]
      A host error has occurred during startup operation 'fe3a9a1d-e32b-4243-9d9f-2e2d3bcbbfe3'.
      System.ArgumentNullException: Value cannot be null. (Parameter 'provider')
         at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetService[T](IServiceProvider provider)
         at Microsoft.Azure.WebJobs.Script.ScriptHostBuilderExtensions.<>c__DisplayClass6_0.<AddScriptHost>b__2(HostBuilderContext context, IConfigurationBuilder configBuilder) in /_/src/WebJobs.Script/ScriptHostBuilderExtensions.cs:line 114
         at Microsoft.Extensions.Hosting.HostBuilder.BuildAppConfiguration()
         at Microsoft.Extensions.Hosting.HostBuilder.Build()
         at Microsoft.Azure.WebJobs.Script.WebHost.DefaultScriptHostBuilder.BuildHost(Boolean skipHostStartup, Boolean skipHostConfigurationParsing) in /_/src/WebJobs.Script.WebHost/DefaultScriptHostBuilder.cs:line 59
         at Microsoft.Azure.WebJobs.Script.WebHost.WebJobsScriptHostService.BuildHost(Boolean skipHostStartup, Boolean skipHostJsonConfiguration) in /_/src/WebJobs.Script.WebHost/WebJobsScriptHostService.cs:line 581
         at Microsoft.Azure.WebJobs.Script.WebHost.WebJobsScriptHostService.UnsynchronizedStartHostAsync(ScriptHostStartupOperation activeOperation, Int32 attemptCount, JobHostStartupMode startupMode) in /_/src/WebJobs.Script.WebHost/WebJobsScriptHostService.cs:line 276

I'm not setup for end-to-end on laptop so can't debug readily but something is amiss there with services - will poke more tomorrow.

… new system Here we're simulating latency in the outbound pipe was it was intended to when everything was posting through the script manager through RX. Also makes the latency non-static and more test friendly.

liliankasem

just noticed there's a TODO here, should probably resolve before merge! (+ any other TODOs I may have missed). If not addressed, you should create an issue for any items we're not handling in this PR

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

Unsure if this covers #8604 completely or not - hope so!

…SendsInvocationCancelRequest

NickCraver · 2022-09-02T11:58:48Z

Gentle reminder we should try and get this in before more conflicts pile up against the old script manager approach.

brettsam · 2022-09-02T15:23:45Z

Let's get it in -- no objections from me. Although I do see conflicts again...

… tests)

NickCraver · 2022-09-02T15:59:17Z

@brettsam merged once more from dev and addressed one test race (existing issue but hey, improvements!)

fabiocav · 2022-09-02T18:50:19Z

Thanks, @NickCraver.

Good to have this merged once conflicts are resolved.

NickCraver · 2022-09-02T18:59:46Z

@fabiocav I can't see merge conflicts local or PR UX, any idea what it's conflicting with?

fabiocav · 2022-09-02T19:11:48Z

@NickCraver odd, I don't see the usual list, just the conflict warning in GH.

If you can merge locally, go for it!

NickCraver · 2022-09-02T19:12:54Z

I beat the GitHub UI!

mgravell and others added 3 commits April 4, 2022 14:47

remove RX as the primary pipe for gRPC communications

da1932e

remove RX as the primary pipe for gRPC communications

fixup test failures

ea4aee8

1: GrpcWorkerChannelTests span up duplicate channels 2: suppress write failures (async void)

tweak the worker channel process flow to ensure it is testable; fixup…

3739612

… TestFunctionRpcService and GrpcWorkerChannelTests to match

fabiocav requested review from mathewc, pragnagopa, fabiocav and brettsam April 29, 2022 21:21

pragnagopa requested a review from liliankasem April 30, 2022 16:10

safihamid reviewed May 2, 2022

View reviewed changes

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannelFactory.cs Show resolved Hide resolved

mgravell added 3 commits May 5, 2022 15:54

Merge branch 'dev' into mgravell/remove-rx

6f1ea2e

# Conflicts: # src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs # test/WebJobs.Script.Tests/Workers/Rpc/GrpcWorkerChannelTests.cs # test/WebJobs.Script.Tests/Workers/Rpc/TestFunctionRpcService.cs

Merge branch 'dev' into mgravell/remove-rx

e9a2c4d

merge removal of PublishFunctionLoadResponsesEvent work

0daaa10

liliankasem reviewed May 19, 2022

View reviewed changes

brettsam reviewed May 19, 2022

View reviewed changes

mgravell and others added 5 commits June 20, 2022 14:19

Update src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

60e01c8

Co-authored-by: Lilian Kasem <likasem@microsoft.com>

Update src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

a83275f

Co-authored-by: Lilian Kasem <likasem@microsoft.com>

nits and renames from PR review feedback

29540a2

Merge branch 'mgravell/remove-rx' of https://github.com/Azure/azure-f…

85a4265

…unctions-host into mgravell/remove-rx

Merge branch 'dev' into mgravell/remove-rx

990ed5e

INCOMPLETE - still 2 new failing tests # Conflicts: # src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs # test/WebJobs.Script.Tests/Workers/Rpc/GrpcWorkerChannelTests.cs

NickCraver and others added 2 commits July 26, 2022 22:28

Tidy up/PR comments

4a1b0c6

Fix WorkerStatus_NewWorkerAdded by properly simulating latency in the…

914b7d6

… new system Here we're simulating latency in the outbound pipe was it was intended to when everything was posting through the script manager through RX. Also makes the latency non-static and more test friendly.

liliankasem approved these changes Jul 27, 2022

View reviewed changes

paulbatum approved these changes Jul 28, 2022

View reviewed changes

liliankasem reviewed Jul 28, 2022

View reviewed changes

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs Outdated Show resolved Hide resolved

fabiocav reviewed Aug 4, 2022

View reviewed changes

liliankasem mentioned this pull request Aug 4, 2022

Additional logging and cleanup for gRPC RX #8604

Closed

2 tasks

NickCraver added 3 commits August 30, 2022 08:49

Merge remote-tracking branch 'origin/dev' into mgravell/remove-rx

39385ad

Address PR comments

4764d1c

Unsure if this covers #8604 completely or not - hope so!

Test fix for SendInvocationRequest_SignalCancellation_WithCapability_…

f0233b0

…SendsInvocationCancelRequest

NickCraver added perf perf: language-worker labels Aug 30, 2022

NickCraver added 2 commits August 30, 2022 09:45

Fix PR comments - ensure _outbound and _inbound are never null

8f6ca1a

Remove cruft

6b06dc9

brettsam approved these changes Sep 2, 2022

View reviewed changes

NickCraver added 2 commits September 2, 2022 11:26

Merge remote-tracking branch 'origin/dev' into mgravell/remove-rx

015d385

Capture ObjectDisposedException in disposal race (occasional issue in…

6f9f11e

… tests)

brettsam approved these changes Sep 2, 2022

View reviewed changes

fabiocav approved these changes Sep 2, 2022

View reviewed changes

NickCraver merged commit c112509 into dev Sep 2, 2022

NickCraver deleted the mgravell/remove-rx branch September 2, 2022 19:12

brettsam mentioned this pull request Nov 2, 2023

[pack] Enable ordering of RpcLog and InvocationResponse messages #9657

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove RX as the primary pipe for gRPC communications #8281

remove RX as the primary pipe for gRPC communications #8281

mgravell commented Apr 4, 2022 •

edited

Loading

fabiocav commented Apr 29, 2022

mgravell commented Apr 30, 2022 via email

pragnagopa commented Apr 30, 2022

mgravell commented May 6, 2022

liliankasem May 19, 2022

brettsam left a comment

liliankasem commented Jun 15, 2022

fabiocav commented Jun 16, 2022

NickCraver commented Jun 16, 2022

liliankasem commented Jun 16, 2022 •

edited

Loading

safihamid commented Jun 16, 2022

mgravell commented Jun 21, 2022

safihamid commented Jun 22, 2022

NickCraver commented Jul 27, 2022

liliankasem left a comment •

edited

Loading

NickCraver commented Sep 2, 2022

brettsam commented Sep 2, 2022

NickCraver commented Sep 2, 2022

fabiocav commented Sep 2, 2022

NickCraver commented Sep 2, 2022

fabiocav commented Sep 2, 2022

NickCraver commented Sep 2, 2022

remove RX as the primary pipe for gRPC communications #8281

remove RX as the primary pipe for gRPC communications #8281

Conversation

mgravell commented Apr 4, 2022 • edited Loading

Issue describing the changes in this PR

Pull request checklist

Additional information

fabiocav commented Apr 29, 2022

mgravell commented Apr 30, 2022 via email

pragnagopa commented Apr 30, 2022

mgravell commented May 6, 2022

liliankasem May 19, 2022

Choose a reason for hiding this comment

brettsam left a comment

Choose a reason for hiding this comment

liliankasem commented Jun 15, 2022

fabiocav commented Jun 16, 2022

NickCraver commented Jun 16, 2022

liliankasem commented Jun 16, 2022 • edited Loading

safihamid commented Jun 16, 2022

mgravell commented Jun 21, 2022

safihamid commented Jun 22, 2022

NickCraver commented Jul 27, 2022

liliankasem left a comment • edited Loading

Choose a reason for hiding this comment

NickCraver commented Sep 2, 2022

brettsam commented Sep 2, 2022

NickCraver commented Sep 2, 2022

fabiocav commented Sep 2, 2022

NickCraver commented Sep 2, 2022

fabiocav commented Sep 2, 2022

NickCraver commented Sep 2, 2022

mgravell commented Apr 4, 2022 •

edited

Loading

liliankasem commented Jun 16, 2022 •

edited

Loading

liliankasem left a comment •

edited

Loading