Conversation
68e96a1 to
3255ee6
Compare
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: 3255ee6a06
ℹ️ 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".
e309f2a to
c746de3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c746de314b
ℹ️ 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 #3091 delta and the combined final stack. I found no additional PR-local defects in the replay-safe logger provider/builder/record wrapper or the new README.
The prior LoggerBuilder.setInstrumentationAttributes comment is correctly refuted: the pinned OpenTelemetry Java 1.66 LoggerBuilder exposes only schema URL, instrumentation version, and build, so the proposed forwarding method does not exist. The replay-aware isEnabled fix and documentation/comment cleanup are correct.
Focused LoggerTest and MetricsTest runs passed locally, the pinned interfaces were checked against the compiled 1.66 API, the worktree is clean, and current checks are green. I am leaving this as a comment rather than an approval because the final stack still inherits the actionable findings on #3049, #3082, #3089, and #3090.
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Ultrareviewed the exact #3091 delta and the combined final stack a second time, with the pinned 1.66.0 interfaces checked against the compiled API. The logger wrappers are complete (every Logger, LoggerBuilder, LoggerProvider, and LogRecordBuilder method is overridden), the suppression predicate matches the tracer and meter, and LoggerTest proves suppression under both WorkflowReplayer and a real cache-miss replay by asserting on exporter output. The setInstrumentationAttributes Codex comment was correctly refuted.
One new P2 is inline: the README setup sample wires only a span exporter, so the Metrics and Logs sections export nothing as written. Smaller items, not blocking: the PR description still says both isEnabled overloads delegate unchanged, and the ReplaySafeLogger class Javadoc mentions only emission, although isEnabled now returns false under suppression; that isEnabled fix has no test, and neither does the loggerBuilder() path or the propagation of setSchemaUrl and setInstrumentationVersion into the exported scope; isEnabled semantics now diverge from the meter instruments and ReplaySafeSpan.isRecording(), which still delegate under suppression (tracked on #3090); the README never mentions ReplaySafeOpenTelemetry.close(), the @Experimental status, the pinned OpenTelemetry version, Maven coordinates, the observable-instrument caveat, or how this relates to Workflow.getLogger(); and the comment at MetricsTest.java:95 does not end with a period. Current PR checks are green. I am again leaving this as a comment rather than an approval because the final stack still inherits the actionable findings on #3049, #3082, #3089, and #3090.
|
|
||
| ReplaySafeOpenTelemetry openTelemetry = | ||
| ReplaySafeOpenTelemetry.newBuilder() | ||
| .setTracerProviderBuilder( |
There was a problem hiding this comment.
[P2] Wire metric and log exporters in the setup sample
This sample configures only setTracerProviderBuilder, and ReplaySafeOpenTelemetry.Builder defaults the meter and logger providers to bare SdkMeterProvider.builder() and SdkLoggerProvider.builder() with no reader or processor. A user who follows this Setup section and then the Metrics and Logs sections verbatim records into providers that export nothing, with no hint why. Please add .setMeterProviderBuilder(SdkMeterProvider.builder().registerMetricReader(PeriodicMetricReader.builder(OtlpGrpcMetricExporter.builder().build()).build())) and .setLoggerProviderBuilder(SdkLoggerProvider.builder().addLogRecordProcessor(BatchLogRecordProcessor.builder(OtlpGrpcLogRecordExporter.builder().build()).build())) to the sample, or one sentence in each section saying an exporter must be configured on the corresponding provider.
This branch has not been deployed
2 participants
What changed?
ReplaySafeOpenTelemetrynow wraps the configuredSdkLoggerProviderin a replay-safe logger provider, so loggers obtained from the global logs bridge (including throughloggerBuilder) are replay safe inside workflows, matching the tracer and meter providers. This replaces the TODO left on the field.ReplaySafeLogger, which wrapslogRecordBuilder()so the builder'semit()is dropped when it is called by replaying workflow code, and the record is not emitted again on every replay. Every setter and bothisEnabledoverloads delegate unchanged, so severity filtering and record construction behave exactly as they do on the underlying SDK logger.contrib/temporal-opentelemetry-v2/README.mdwith setup and usage examples for tracing, metrics, and logs.Why?
Part of #3046. This is the final slice, stacked on #3090.
Breaking changes?
None. The module is new and marked experimental.
Server PR
None.
Test plan
mise exec -- ./gradlew :temporal-opentelemetry-v2:test -PtestServer=dev-server -x spotlessCheck -x spotlessJava(39 tests, all passing; the 2InterceptorTestfailures noted on Make the OpenTelemetry v2 meter provider replay safe #3090 no longer reproduce)mise exec -- ./gradlew :temporal-opentelemetry-v2:spotlessCheckmise exec -- ./gradlew :temporal-opentelemetry-v2:javadoc