Skip to content

Add plugin hooks for activity, Nexus, and worker factory options - #3082

Open
patbeqo wants to merge 3 commits into
patbeqo/otel-v2-workflow-prerequisitesfrom
patbeqo/otel-v2-workflow-prerequisites-2
Open

patbeqo wants to merge 3 commits into
patbeqo/otel-v2-workflow-prerequisitesfrom
patbeqo/otel-v2-workflow-prerequisites-2

Conversation

@patbeqo

@patbeqo patbeqo commented Sep 16, 2026

Copy link
Copy Markdown

What changed?

  • Added experimental ActivityClientPlugin and NexusClientPlugin, propagated from the service stubs to ActivityClient and NexusClient the same way ScheduleClientPlugin reaches the schedule client, with setPlugins/getPlugins on ActivityClientOptions and NexusClientOptions.
  • SimplePlugin implements both, and its builder gained addScheduleClientInterceptors, addActivityClientInterceptors, and addNexusClientInterceptors. configureScheduleClient now applies its interceptors instead of being a no-op, so plugins no longer need configure* overrides to install interceptors on any client.
  • Replaced the three hand-rolled plugin extraction loops with PluginUtils.extractPlugins, which the two new sites also use.
  • StartNexusOperationExecutionInput now copies headers into a mutable, case-insensitive map, matching ExecuteNexusOperationInput, so header-adding interceptors can populate it in place as its javadoc describes. Previously it kept the caller-provided Collections.emptyMap().
  • Added WorkerFactoryOptions support for context propagators, so plugins installed only on worker factory options can propagate workflow context into workflow child threads.
  • WorkerFactory now combines workflow-client and factory propagators, deduplicating by propagator name with client configuration taking precedence.
  • Added coverage for propagation of the new hooks from the service stubs, the builder helpers, plugin copying on both options classes, factory-only workflow-thread propagation, and duplicate propagator names.

Why?

Standalone activities and client-started Nexus operations had no plugin hook to install an interceptor through, and the schedule hook had no builder helper. This is the second prerequisite slice, stacked on #3049.

The worker-factory additions also make the documented factory-only plugin installation path functional: context propagators configured by a plugin on WorkerFactoryOptions are available to workers and workflow child threads, while avoiding duplicate propagators when the same name is configured on both the workflow client and the factory.

Breaking changes?

None. The public APIs are additive and experimental.

Server PR

None.

Test plan

  • ./gradlew :temporal-sdk:test --tests 'io.temporal.common.*' --tests 'io.temporal.client.*'
  • ./gradlew :temporal-sdk:test -PtestServer=dev-server --tests 'io.temporal.client.nexus.*' --tests 'io.temporal.client.functional.StandaloneActivityTest' --tests 'io.temporal.client.schedules.*' --tests 'io.temporal.common.PluginPropagationTest'
  • ./gradlew :temporal-sdk:spotlessCheck
  • ./gradlew :temporal-sdk:javadoc
  • Focused context propagation tests for worker-factory-only installation and duplicate propagator names.

@patbeqo
patbeqo added this pull request to stack #3083 September 16, 2026 22:06
@patbeqo
patbeqo force-pushed the patbeqo/otel-v2-workflow-prerequisites-2 branch from 58542ea to 49a89cd Compare September 17, 2026 17:26
@patbeqo

patbeqo commented Sep 17, 2026

Copy link
Copy Markdown
Author

@codex

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 17, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-18T22:53:45.967164Z 1557bdd Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 49a89cd802

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@patbeqo patbeqo mentioned this pull request Sep 17, 2026
3 tasks
@patbeqo patbeqo changed the title Add plugin hooks for activity and Nexus clients Add plugin hooks for activity, Nexus, and worker factory options Sep 18, 2026
@patbeqo
patbeqo force-pushed the patbeqo/otel-v2-workflow-prerequisites-2 branch from 02c9bb0 to df40d1c Compare September 18, 2026 19:38
@patbeqo

patbeqo commented Sep 18, 2026

Copy link
Copy Markdown
Author

@codex

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: df40d1c491

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread temporal-sdk/src/main/java/io/temporal/worker/WorkerFactoryOptions.java Outdated
Comment thread temporal-sdk/src/test/java/io/temporal/workflow/ContextPropagationTest.java Outdated
Comment thread temporal-sdk/src/main/java/io/temporal/common/SimplePlugin.java
@patbeqo
patbeqo force-pushed the patbeqo/otel-v2-workflow-prerequisites-2 branch from df40d1c to 5e7a14c Compare September 18, 2026 20:08
@patbeqo

patbeqo commented Sep 18, 2026

Copy link
Copy Markdown
Author

@codex

@patbeqo
patbeqo force-pushed the patbeqo/otel-v2-workflow-prerequisites-2 branch from 5e7a14c to 1a6bd84 Compare September 18, 2026 20:09
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 5e7a14c31f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@patbeqo
patbeqo force-pushed the patbeqo/otel-v2-workflow-prerequisites-2 branch from e173af5 to b49dc54 Compare September 18, 2026 21:14
@patbeqo
patbeqo force-pushed the patbeqo/otel-v2-workflow-prerequisites-2 branch from b49dc54 to 1557bdd Compare September 18, 2026 21:43
@patbeqo
patbeqo marked this pull request as ready for review September 18, 2026 22:51
@patbeqo
patbeqo requested a review from a team as a code owner September 18, 2026 22:51
@patbeqo
patbeqo requested a review from a team September 18, 2026 22:51

@DABH DABH left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultrareviewed the exact stacked delta against #3049. I found two independently reproduced replay-compatibility blockers inline. Both generate nondeterministic commands when histories created by the base SDK are replayed at this head, so green same-version tests do not cover them.

The three earlier Codex comments were valid and are correctly addressed: unset propagators are normalized, the workflow test now uses SDKTestWorkflowRule, and standalone activity clients receive plugin propagators. I found no further issues in the activity/Nexus/schedule hooks, option copying, header mutability, plugin extraction, or propagator deduplication. Current PR checks are green.

handlers.add(() -> proc.apply(resultPromise));
// Handlers run in a callback thread created by the runner, which inherits its propagated
// contexts from the workflow header rather than from the thread registering the handler.
Map<String, Object> contexts = ContextThreadLocal.getCurrentContextForPropagation();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Version this workflow-visible continuation change

Capturing the registration context changes what an incomplete-promise continuation observes from the completing thread's context to the registering thread's context. That value is workflow-observable, so existing histories can take a different command branch after an SDK upgrade.

I reproduced this on the exact base/head pair: the base ran an async child under context child, registered a continuation under registration, and completed without a timer; replaying that completed history here made the continuation observe registration, emit START_TIMER, and fail with TMPRL1100 because history contained WORKFLOW_EXECUTION_COMPLETED.

Please gate the new semantics behind an SdkFlag so histories without the flag retain the old completion-thread behavior, and add the generated base history as replay coverage. The current new-execution test cannot detect this compatibility break.

for (ContextPropagator propagator : workflowClient.getOptions().getContextPropagators()) {
contextPropagatorsByName.putIfAbsent(propagator.getName(), propagator);
}
for (ContextPropagator propagator : factoryOptions.getContextPropagators()) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve old behavior for existing factory-only histories

SimplePlugin.addContextPropagators(...) and factory-only plugin installation both existed before this PR, but those propagators previously did not reach workflow child threads. Activating them here silently changes workflow-visible behavior during an SDK upgrade.

I reproduced a second cross-version failure: a base workflow with an existing factory-only plugin created an Async.function child, which saw no parent context and completed without a timer. Replaying that history at this head made the child inherit the context and take a timer branch, again failing with WORKFLOW_EXECUTION_COMPLETED versus START_TIMER.

This path also needs SDK-version gating (or another migration that preserves legacy workflow execution) plus a legacy-history replay test. The merge order and name deduplication themselves look correct.

@DABH DABH left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultrareviewed the exact stacked delta against #3049 a second time with an independent trace. Both P1 replay-compatibility findings stand: an incomplete-promise continuation now observes the registering thread's context instead of the completing thread's, and factory-only plugin propagators now reach workflow child threads that previously inherited nothing. Both need SdkFlag gating plus a replay test from a base-generated history; SKIP_YIELD_ON_VERSION and DETERMINISTIC_CANCELLATION_SCOPE_ORDER show the pattern.

Two new items are inline: the data converter customizer is not applied to the activity, Nexus, or schedule clients (P2), and the CompletablePromiseImpl comment misdescribes the threading model while the PR description claims no breaking changes (P2). Smaller items, not blocking: stubs-level ActivityClientPlugins now also wrap the SDK-internal ActivityClients built in TemporalNexusClientImpl, so the comment at line 1118 saying standalone interceptors are unavailable in the Nexus handler lifecycle is no longer accurate for stubs-installed plugins; WorkerFactoryOptions.setContextPropagators is not @Experimental although the sibling setPlugins is, and its Javadoc says "appended" while same-name duplicates are dropped with client precedence; and there is no replay coverage for either behavior change. The three Codex threads remain correctly addressed. Current PR checks are green.

}

@Override
public void configureActivityClient(@Nonnull ActivityClientOptions.Builder builder) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Apply the data converter customizer to the activity, Nexus, and schedule clients

configureWorkflowClient applies dataConverterCustomizer, but configureScheduleClient, configureActivityClient, and configureNexusClient do not, although all three option builders expose setDataConverter and the schedule client uses its converter for memos and action payloads. Now that SimplePlugin is an ActivityClientPlugin and a NexusClientPlugin, a codec plugin installed on the service stubs reaches ActivityClient.newInstance(stubs) and NexusClient.newInstance(stubs), but those clients keep GlobalDataConverter while the workers decode with the customized converter, so standalone activity or Nexus inputs are encoded differently from what the worker expects. The schedule half is pre-existing, since that hook used to be a no-op, but has the same shape. Please apply the customizer in all three methods and cover it with tests. This does not block the OpenTelemetry stack, which uses no customizer.

unregisterWithRunner();
} else {
handlers.add(() -> proc.apply(resultPromise));
// Handlers run in a callback thread created by the runner, which inherits its propagated

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Correct the continuation threading comment and the compatibility claim

This comment says handlers run in a callback thread created by the runner that inherits its contexts from the workflow header. That is only true for SDK-driven completions such as activities, timers, child workflows, and signals, which arrive through executeInWorkflowThread on a new callback thread. invokeHandlers() below runs the handlers inline on whatever thread calls complete(), so a promise completed by an Async.function child runs the continuation on that child thread with its creation-time context. Before this change a continuation therefore observed the completing thread's context; after it, the registering thread's. That is the workflow-visible difference behind the P1 above, and it also makes the PR description's "Breaking changes? None" inaccurate. Please rewrite the comment to describe the inline invocation and update the description once the SdkFlag gating is in place.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants