Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eef033e5ee
ℹ️ 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".
| Random stream = getStream(); | ||
| if (stream == null) { | ||
| return IdGenerator.random().generateTraceId(); |
There was a problem hiding this comment.
For root spans created inside workflows, getStream() returns a java.util.Random, whose internal state is only 48 bits even though two nextLong() calls are formatted as a 128-bit trace ID. Consequently different workflow-run/name seeds begin producing identical trace and span sequences around the 2^24-run birthday bound, which is reachable in long-lived Temporal installations and causes unrelated traces to be merged; use a deterministic generator whose state retains the full trace-ID entropy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The added Javadoc warning confirms the collision bound but does not mitigate it for a production tracing provider: colliding 48-bit stream states generate identical trace-ID sequences. I left the foundational fix on #3049 at #3049 (comment), since widening the named stream there fixes this consumer without a second generator implementation. This should remain open until that dependency changes.
| @@ -0,0 +1,26 @@ | |||
| description = '''Temporal Java SDK OpenTelemetry v2 Module''' | |||
There was a problem hiding this comment.
This introduces a separately published, user-facing artifact without a README or usage guide, unlike the existing OpenTelemetry module. In particular, users are not shown the dependency declaration, how to construct and register ReplaySafeOpenTelemetry, how to attach the plugin, or how provider ownership and shutdown work, leaving the new module difficult to adopt safely.
AGENTS.md reference: AGENTS.md:L53-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
A follow up PR on this stack will include the README.
There was a problem hiding this comment.
Recommend closing this thread here in favor of the README that #3091 adds, with these gaps filled there: the Setup sample wires only a span exporter, so the Metrics and Logs sections export nothing as written; there is no mention of ReplaySafeOpenTelemetry.close() or of any flush path; the ordering constraint that the global must be set before building the plugin, setHeaderKey, setPropagators, the ContextStorageProvider caveat, the 48-bit entropy caveat, the observable-instrument caveat from the ReplaySafeMeter Javadoc, and the @Experimental status are all absent; and there are no Maven coordinates or a stated pinned OpenTelemetry version. I have left the README finding on #3091 directly.
| try (Scope ignored = Context.current().with(TRACER_NAME, tracerName).makeCurrent()) { | ||
| return new ReplaySafeSpan(delegate.startSpan()); | ||
| } |
There was a problem hiding this comment.
During replay this still starts a real SDK span and only suppresses its later end(). Every sampled historical span therefore invokes the configured sampler and each SpanProcessor.onStart, but never receives the matching onEnd; processors that track active spans, allocate per-span state, or perform start-time instrumentation leak state or emit duplicate replay effects. Since the builder accepts arbitrary SDK processors, replay-only spans need a non-recording/deferred path rather than starting a delegate recording span and abandoning it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
A span can be started during replay but ended live. A span can never end during replay.
There was a problem hiding this comment.
The response covers spans that cross the replay/live boundary, but not spans whose complete lifetime is in the replayed portion. I reproduced the original concern on exact head d4a61fb: a workflow created and ended an application span, then WorkflowReplayer replayed its completed history with a counting SpanProcessor. Replay increased onStart, while onEnd did not increase because ReplaySafeSpan.end() swallowed it. Arbitrary processors therefore do receive unmatched starts and can retain state or emit start-time effects. This thread remains actionable; the replay-only path needs to avoid registering an abandon-only delegate span while still supporting genuinely cross-boundary spans.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7d52f9ab1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f239f732c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14423f6442
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73678eb01d
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 632635bb43
ℹ️ 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".
c5b05aa to
de76218
Compare
de76218 to
d3aa415
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3aa415401
ℹ️ 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".
b906ec6 to
9d77af7
Compare
9d77af7 to
ac9ffae
Compare
ac9ffae to
d4a61fb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4a61fb775
ℹ️ 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".
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Ultrareviewed the exact stacked delta and the completed #3090/#3091 follow-ups. I found one new interoperability issue inline and revalidated two previously resolved concerns: replay-only spans do leave arbitrary span processors with unmatched onStart calls, and the 48-bit ID-stream state remains a real collision limit (the foundational fix is noted on #3049). I am reopening those existing threads with concrete evidence rather than duplicating them.
The other prior comments are correctly fixed or handled later in the stack: child-thread propagation and factory-only installation are implemented in #3082, metrics/log suppression arrive in #3090/#3091, the README arrives in #3091, and the converter, schedule, attributes, Guava, explicit timestamp, and header-reuse fixes are present.
CI has one failed Edge job on the current SHA, but an identical duplicate job passed. The sole failure is the known MetricsTest.testStickyCacheSize race, reproduced on main and already tracked by #3069; it is unrelated to this module. Focused OpenTelemetry tests and isolated replay probes passed locally, and the worktree is clean.
| public final class TemporalContextStorageProvider implements ContextStorageProvider { | ||
| @Override | ||
| public ContextStorage get() { | ||
| return new TemporalContextStorage(ContextStorage.defaultStorage()); |
There was a problem hiding this comment.
[P2] Do not disable context propagation when another storage provider exists
OpenTelemetry 1.66 service-loads ContextStorageProviders and, when it finds more than one without an explicit system-property selection, logs a warning and falls back to ContextStorage.defaultStorage(). Publishing this provider therefore makes any application that already has a provider lose both that provider and TemporalContextStorage; workflow child threads then use physical-thread-local context and the plugin's advertised propagation stops working.
I reproduced this in a fresh test process by adding a second test provider: ServiceLoader found both providers and ContextStorage.get() was no longer a TemporalContextStorage. Selecting either provider with the system property still discards the other, so that is not composition. Please install this as a composable wrapper/explicit integration (or otherwise preserve an existing provider) rather than contributing a competing process-global provider.
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Ultrareviewed the exact stacked delta a second time, with an independent trace and scratch reproductions, and revalidated the open threads. One new P1 is inline: sticky-cache eviction ends and exports blocked inbound spans as complete, and the replayed run then re-exports the same deterministic trace and span IDs with different timings. Two new P2s are inline: OtelTestBase does not actually disable the sticky cache, so the module's tests never exercise a cache-miss replay, and span events and recorded exceptions during replay carry replay-time wall-clock timestamps.
The three open threads remain actionable as previously described: replay-only spans still reach SpanProcessor.onStart on every replay without a matching onEnd, the 48-bit stream state still bounds trace-ID uniqueness (fix on #3049), and the process-global ContextStorageProvider still loses to any second provider. On the Codex documentation thread I have recommended closing it in favor of the README in #3091, with a list of the gaps to fill there. Every previously resolved Codex item is genuinely fixed in code; the metrics suppression resolved via #3090 covers synchronous instruments only, and the observable-instrument gap is tracked on the open #3090 thread.
Smaller items, not blocking: ReplaySafeSpan does not override Span.addLink(...), so links added after start are silently dropped by the interface default; OpenTelemetryPlugin.Builder.build() checks the global through GlobalOpenTelemetry.getTracerProvider(), which initializes the global to no-op when unset, so the GlobalOpenTelemetry.set remedy the error message suggests then throws "already been called"; OpenTelemetryScheduleClientCallsInterceptor clears and writes the caller's live Header on ScheduleActionStartWorkflow rather than a copy; an undecodable _tracer-data header fails every workflow task permanently because SpanCodec.decode throws before any handler runs (parity with the OpenTracing module, but a fallback to the current context would be friendlier); the public interceptor classes are non-final, not @Experimental, and take the internal.InterceptorTracer type in their constructors; and temporalRunID is emitted as an empty string when a stub is unbound. CI has one failed Edge job on this SHA, again the known MetricsTest.testStickyCacheSize race tracked by #3069; the module's own tests pass in every job.
| try (Scope ignored = span.makeCurrent()) { | ||
| return run(span, call); | ||
| } finally { | ||
| span.end(); |
There was a problem hiding this comment.
[P1] Do not export evicted inbound spans as complete
traceInbound always ends the span in this finally, run rethrows DestroyWorkflowThreadError without marking anything, and ReplaySafeSpan.end() only suppresses while isReplaying(), which is false once the live task has run. So when a cached run is evicted while blocked, every RunWorkflow, HandleSignal, and HandleUpdate span for it is ended and exported as complete with status UNSET although the workflow is still running. Because span IDs are drawn deterministically, the replayed run then regenerates the same trace and span IDs and exports the same span a second time at real completion, with a different start time.
Reproduced on this head with a workflow blocked on a signal, setAddTemporalSpans(true), and SDKTestWorkflowRule.invalidateWorkflowCache() mid-run: the eviction export and the final export of RunWorkflow:EvictWf carried the identical span ID 63cb341eaa579549, one with the live wall-clock start and one with the replayed workflow-time start, about 114 ms apart. A second reproduction with cache size 1 and two blocked workflows behaved the same. Every deploy or WorkerFactory.shutdown (which invalidates the whole cache), LRU eviction, and workflow task failure therefore emits fake-complete spans for all cached runs. The OpenTracing module handles the same unwind by tagging evicted=true, and its non-deterministic IDs avoid the duplicate-ID export. Application spans that wrap an await in the standard try/finally { span.end() } pattern suffer the same.
Please skip end() (or at minimum set an evicted attribute) when the unwind is a DestroyWorkflowThreadError, so the replayed instance is the one that ends the span, and add a test that invalidates the cache mid-run.
| .setPlugins( | ||
| OpenTelemetryPlugin.newBuilder().setAddTemporalSpans(addTemporalSpans).build()) | ||
| .build()) | ||
| .setWorkerFactoryOptions(WorkerFactoryOptions.newBuilder().setWorkflowCacheSize(0).build()); |
There was a problem hiding this comment.
[P2] The test base does not disable the sticky cache
WorkerFactoryOptions.Builder.build() coerces workflowCacheSize <= 0 back to the default of 600 (WorkerFactoryOptions.java:257-258), so this rule keeps a full sticky cache and the Javadoc claim that "replay runs on every task" is false. As a result the interceptor, propagation, and tracer tests in this module never run a cache-miss replay, the test that promises an explicit start timestamp survives replay never replays, and the task-retry test re-executes a failed task rather than replaying recorded history. Genuine history replay of spans is exercised only by the three dev-server-gated reset tests and by no WorkflowReplayer test in this PR, which is how the eviction P1 went unnoticed. Please force replay explicitly, with WorkflowReplayer or invalidateWorkflowCache(), and correct the comment.
|
|
||
| @Override | ||
| public Span addEvent(String name, Attributes attributes) { | ||
| delegate.addEvent(name, attributes); |
There was a problem hiding this comment.
[P2] Use workflow time for events and exceptions recorded during replay
NamedStreamSpanBuilder.startSpan() substitutes Workflow.currentTimeMillis() for the start timestamp while replaying, but addEvent(name, attributes) and recordException(...) here delegate without a timestamp, so SdkSpan stamps them with the replay-time clock. A span started before an await, annotated with an event, and ended after a cache miss will therefore carry an event timestamp equal to the replay time, potentially hours after the true moment and inconsistent with the span's own recorded start. Please pass Workflow.currentTimeMillis() explicitly under the same isWorkflowThread && isSubjectToReplay && isReplaying condition used for the start timestamp. Traced in code; not reproduced.
This branch has not been deployed
2 participants
What changed?
temporal-opentelemetry-v2contrib module and registered it insettings.gradleand the BOM.OpenTelemetryPlugininstalls tracing interceptors on the workflow, schedule, activity, and Nexus clients plus the worker through theSimplePluginbuilder helpers. Its builder exposessetHeaderKey(defaults to_tracer-data) andsetAddTemporalSpans(defaults to false, so only context propagation happens unless enabled).ReplaySafeOpenTelemetryis theOpenTelemetryto register withGlobalOpenTelemetry.set. Its tracer provider wraps spans so they are no-ops during replay and in read-only workflow contexts, and its id generator draws span and trace ids from named workflow random streams so ids are stable across replay. Propagators default to W3C trace context plus baggage and are configurable viasetPropagators.OpenTelemetryPlugin.Builder.build()rejects a global that is not aReplaySafeOpenTelemetry.InterceptorTracerserializes the current context into Temporal headers with the global propagator and, when enabled, creates spans for Temporal operations (StartWorkflow,RunWorkflow,RunActivity,HandleSignal,HandleUpdate,RunStartNexusOperationHandler, and so on). It rethrows the Continue-As-New unwind without recording an error.Why?
Part of #3046. This is the third slice, stacked on #3049 and #3082, which supplied the named random streams, read-only detection, and client plugin hooks the module depends on.
Breaking changes?
None. The module is new and marked experimental.
Server PR
None.
Test plan
Added tests covering all client, worker, and workflow paths the plugin instruments, with and without Temporal spans enabled.
mise exec -- ./gradlew :temporal-opentelemetry-v2:test -PtestServer=dev-server -x spotlessCheck -x spotlessJava(13 tests; the Reset and comprehensive tests require the dev server and are gated withassumeTrue)mise exec -- ./gradlew :temporal-opentelemetry-v2:spotlessCheckmise exec -- ./gradlew :temporal-opentelemetry-v2:javadoc