Add Nexus Worker interceptor #2278

Quinn-With-Two-Ns · 2024-10-17T18:13:41Z

Add Nexus Worker interceptor based on the Nexus Java SDK interceptors proposed here nexus-rpc/sdk-java#14

Quinn-With-Two-Ns · 2024-12-05T00:29:37Z

Note: CI will fail until a new Nexus Java SDK is released

cretz

Looks great, only one minor span naming thing worth discussing

cretz · 2024-12-05T13:35:09Z

temporal-opentracing/src/main/java/io/temporal/opentracing/SpanOperationType.java

+  EXECUTE_NEXUS_OPERATION("ExecuteNexusOperation"),
+  START_NEXUS_OPERATION("StartNexusOperation"),
+  CANCEL_NEXUS_OPERATION("CancelNexusOperation");


To match other span names, I wonder if this should be

Suggested change

EXECUTE_NEXUS_OPERATION("ExecuteNexusOperation"),

START_NEXUS_OPERATION("StartNexusOperation"),

CANCEL_NEXUS_OPERATION("CancelNexusOperation");

START_NEXUS_OPERATION("StartNexusOperation"),

RUN_NEXUS_OPERATION("RunNexusOperation"),

CANCEL_NEXUS_OPERATION("CancelNexusOperation");

It seems we use "start" for outbound things and "run" for inbound things. I would usually not consider this a blocker, but we copy these names in the other SDKs too.

Yes I was inconsistent here since the handler side is called startOperation

in the future we will have types like GetResult or GetInfo so I think the types here should reflect the method being called

Yes I was inconsistent here since the handler side is called startOperation

My concern is more about the user consistency than the method names. We have interceptor names called start for start workflow but execute for starting child workflow, so we are inconsistent anyways there.

But we have outbound spans for starting workflow, starting activity, starting child workflow, and now starting Nexus operation. I think calling all of those "Start" spans is reasonable from the caller side. And we have inbound spans for running workflow, running activity, and now running Nexus operation, so "Run" is reasonable there. I can understand that both child workflows and Nexus operations may not complete their work in the span (this behavior is different in each SDK), but still is a "Run" in the sense that it's an implementation side thing as opposed to "Start" which is a caller side thing.

But unlike activities the Nexus operation is not being "Run" in this span, it is simply being started. if we wanted to keep the Run prefix I would be OK with StartNexusOperation -> RunStartNexusOperation

I think the same thing happens to child workflows though, especially with sticky disabled, and especially in SDKs that do not recreate this span during replay. The operation is being run, it just may say it's not completing yet. I think we would have to use at least RunStartNexusOperation since StartNexusOperation is for the caller side. Not the biggest fan, but it's at least a bit more consistent.

I think it is a bit different because nexus handlers are not trying to look like durable execution

bergundy · 2024-12-06T13:58:19Z

.../main/java/io/temporal/opentracing/internal/OpenTracingWorkflowOutboundCallsInterceptor.java

+      } finally {
+        nexusOperationExecuteSpan.finish();
+      }


Remind me again when this finally block is hit in Java? Is this after the operation completes or after the request to schedule it has been sent?

After the request to schedule it has been sent, we do the same thing for activities and child workflows in the Java SDK open tracing interceptor

bergundy · 2024-12-06T14:00:32Z

temporal-opentracing/src/main/java/io/temporal/opentracing/internal/SpanFactory.java

+    SpanCreationContext context =
+        SpanCreationContext.newBuilder()
+            .setSpanOperationType(SpanOperationType.START_NEXUS_OPERATION)
+            .setActionName(serviceName + "." + operationName)


I wonder if it'd be better to use a slash for the delimiter. That's the standard for gRPC services and likely will be the same for Nexus.

I think this may be an opportunity for Nexus spec to define what a single-string qualified operation name looks like. I think I like slash because that represents how it looks in a URL. I also think we should consider disallowing slashes (and maybe other non-url-safe characters) in operation names, but that's not that important.

Going with / for now, can resist since we are still not GA and this is the open tracing interceptor

bergundy · 2024-12-06T14:01:02Z

temporal-opentracing/src/test/java/io/temporal/opentracing/NexusOperationTest.java

@@ -0,0 +1,198 @@
+/*
+ * Copyright (C) 2022 Temporal Technologies, Inc. All Rights Reserved.


Don't you want to update the year?

bergundy · 2024-12-06T14:07:47Z

...sdk/src/main/java/io/temporal/common/interceptors/NexusOperationInboundCallsInterceptor.java

+  StartOperationOutput startOperation(StartOperationInput input)
+      throws OperationUnsuccessfulException;


I wonder if this shouldn't follow the operation handler interface but I understand that what you have here is more consistent with Temporal interceptors.

Quinn-With-Two-Ns mentioned this pull request Oct 17, 2024

Add operation interceptors nexus-rpc/sdk-java#14

Merged

Quinn-With-Two-Ns force-pushed the issue-2220v2 branch from b7e91c4 to 4102533 Compare December 5, 2024 00:29

Quinn-With-Two-Ns marked this pull request as ready for review December 5, 2024 00:29

Quinn-With-Two-Ns requested a review from a team as a code owner December 5, 2024 00:29

cretz reviewed Dec 5, 2024

View reviewed changes

bergundy approved these changes Dec 6, 2024

View reviewed changes

cretz approved these changes Dec 6, 2024

View reviewed changes

Quinn-With-Two-Ns added 8 commits December 6, 2024 09:54

Add Nexus Worker interceptor

e1beb6a

Switch to middleware approach

45f9608

Update to new API

38d1a57

Run spotless

9ce980a

update traces

77bc027

switch from . to /

04ea9c1

update nexus sdk

7ed7c9d

Refactor test tracer for consistency

b6e8ba6

Quinn-With-Two-Ns force-pushed the issue-2220v2 branch from 7212c59 to b6e8ba6 Compare December 6, 2024 17:54

Quinn-With-Two-Ns added 3 commits December 6, 2024 10:44

Disable CancelAsyncOperationTest on real server for now

dd9c3b2

Run spotless

639cc8f

fix assume

8d7b91d

Quinn-With-Two-Ns merged commit 30f391f into temporalio:master Dec 6, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Nexus Worker interceptor #2278

Add Nexus Worker interceptor #2278

Quinn-With-Two-Ns commented Oct 17, 2024

Quinn-With-Two-Ns commented Dec 5, 2024

cretz left a comment

cretz Dec 5, 2024 •

edited

Loading

Quinn-With-Two-Ns Dec 5, 2024

Quinn-With-Two-Ns Dec 5, 2024

cretz Dec 5, 2024 •

edited

Loading

Quinn-With-Two-Ns Dec 5, 2024

cretz Dec 5, 2024 •

edited

Loading

Quinn-With-Two-Ns Dec 5, 2024

bergundy Dec 6, 2024

Quinn-With-Two-Ns Dec 6, 2024

bergundy Dec 6, 2024

cretz Dec 6, 2024

Quinn-With-Two-Ns Dec 6, 2024

bergundy Dec 6, 2024

bergundy Dec 6, 2024

		@@ -0,0 +1,198 @@
		/*
		* Copyright (C) 2022 Temporal Technologies, Inc. All Rights Reserved.

		StartOperationOutput startOperation(StartOperationInput input)
		throws OperationUnsuccessfulException;

Add Nexus Worker interceptor #2278

Add Nexus Worker interceptor #2278

Conversation

Quinn-With-Two-Ns commented Oct 17, 2024

Quinn-With-Two-Ns commented Dec 5, 2024

cretz left a comment

Choose a reason for hiding this comment

cretz Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Dec 5, 2024 • 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

Choose a reason for hiding this comment

cretz Dec 5, 2024 •

edited

Loading

cretz Dec 5, 2024 •

edited

Loading

cretz Dec 5, 2024 •

edited

Loading