Skip to content

fix(otel): scope deterministic ID generation - #627

Open
zhongkechen wants to merge 10 commits into
mainfrom
fix/otel-scoped-id-generation
Open

fix(otel): scope deterministic ID generation#627
zhongkechen wants to merge 10 commits into
mainfrom
fix/otel-scoped-id-generation

Conversation

@zhongkechen

@zhongkechen zhongkechen commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • scope deterministic trace and span ID overrides to durable-plugin span creation while preserving the configured provider ID generator for unrelated instrumentation
  • separate the deterministic Workflow trace from ambient Invocation traces and remove fabricated replay/continuation links
  • late-bind the ADOT/global tracer provider at invocation start; disable telemetry for the entire invocation when it is unavailable and retry on the next invocation without installing OpenTelemetry's no-op global
  • keep explicit builder-based providers eagerly bound and caller-owned
  • remove the unreleased plugin-owned AUTO_OTLP pipeline and redundant ProviderSource; config/no-arg constructors use the global provider and builder constructors use caller-owned providers
  • remove exporter and semantic-conventions dependencies that were only needed by AUTO_OTLP
  • remove OpenTelemetry handlers, tests, dependencies, and ADOT deployment plumbing from the examples module
  • run the shared OpenTelemetry conformance orchestrator for SDK and OTel-plugin pull requests, pushes to main, and manual long-running phases

Testing

  • mvn spotless:apply
  • mvn -pl otel-plugin -am -Djacoco.skip=true -DargLine=... test (1,293 SDK, 13 testing-utility, and 160 OTel tests passed)
  • mvn -pl examples -am -Djacoco.skip=true -DargLine=... test (103 example tests: 72 passed and 31 cloud-only tests skipped)
  • resolved examples dependency tree contains no OTel plugin, OpenTelemetry, or X-Ray artifacts
  • generated the SAM template with 37 non-OTel functions and parsed workflow YAML locally
  • GitHub accepted the pinned reusable conformance workflow

General cloud tests were not run locally because they require deployed AWS resources. OpenTelemetry end-to-end behavior is covered by the conformance workflow.

Closes #625

@zhongkechen
zhongkechen requested a review from a team August 15, 2026 18:30
@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 15, 2026 18:30 — with GitHub Actions Inactive
@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 15, 2026 18:30 — with GitHub Actions Inactive
Comment thread otel-plugin/src/main/java/software/amazon/lambda/durable/otel/ProviderSource.java Outdated
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 15, 2026 20:43 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 15, 2026 21:36 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 18, 2026 17:57 — with GitHub Actions Inactive
@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 18, 2026 19:11 — with GitHub Actions Inactive
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime August 18, 2026 20:45 — with GitHub Actions Active
@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 18, 2026 20:45 — with GitHub Actions Inactive
@github-actions

Copy link
Copy Markdown

Codex AI review

Three actionable issues were found: a public API compatibility break and two deterministic-ID regressions.

Reviewed commit ae9d83664d10a88df71552ab6e15f1bab1c7ddf9. Workflow run

@github-actions

Copy link
Copy Markdown

Claude AI review

No confirmed, actionable findings. This is a coherent, well-tested refactor of the OTel plugin's ID scoping, and I could not substantiate any correctness, regression, API-compatibility, replay, concurrency, or lifecycle defect against the PR bar.

What I verified

  • Workflow trace-ID determinism holds. The new generateTraceIdForExecution(arn, executionStartTime) (epoch-seconds prefix + sha256(arn)[0:24]) is stable across invocations only if executionStartTime is stable. InvocationInfo.executionStartTime() is documented as "taken from the initial EXECUTION operation… stable across all invocations," and DurableExecutor sources it from executionManager.getExecutionOperation().startTimestamp(). Confirmed stable, and X‑Ray‑compatible (8 hex epoch + 24 hex → 32 chars).
  • Scoped-override mechanism is sound. useIds/startSpan/IdScope restore prior state on close (try-with-resources), consumeScopedSpanId prevents span-ID reuse, and the cross-classloader bridge via thread-keyed system properties works for both the plugin instance and the agent instance. Concurrency is covered by concurrentScopedIds_doNotOverwriteEachOther.
  • resolveParentContext change is correct. Removing the prior-invocation placeholder branch in InvocationOtelPlugin is right (the old branch relied on generateTraceId(), which now returns a bogus fallback trace); operations correctly fall back to the invocation span. ExecutionOtelPlugin correctly retains the branch but re-anchors it to workflowTraceId.
  • Late-bind + telemetry-disable path is safe. When bindTracer() fails, tracingEnabled stays false and every hook early-returns; no no-op global is installed and resolution retries next invocation. Stale durableExecutionArn/workflowSpan are never read while disabled. Tests assert !GlobalOpenTelemetry.isSet() and that a later invocation binds and exports.
  • Removed API is unreleased. ProviderSource, AUTO_OTLP, and the OTLP config getters are removed, but the module is 2.1.1-SNAPSHOT; no released-API break is confirmable, consistent with the PR description.
  • Tests are adequate for the changed behavior (independent Workflow vs. Invocation trace IDs, no fabricated replay/continuation links, scoped delegation to the configured generator, order() == Integer.MAX_VALUE, late-bind retry).

Residual risk (not defects, but worth noting)

  • OTel-SDK internal coupling: DeterministicIdGenerator.installOn reflects the private IdGenerator field out of SdkTracerProviderBuilder and throws IllegalStateException if it can't read it. It works on the pinned opentelemetry.version (1.65.0) and in the supported classpath-based Lambda runtime, but an OTel upgrade or a strong-module-encapsulation environment (where trySetAccessible() returns false) would fail plugin construction. Consider guarding a future version bump with a test that builds a real provider through installOn.
  • End-to-end coverage moved out of repo: the cloud OTel/X‑Ray integration tests were deleted and trace-shape validation now lives only in the external conformance workflow, so real ADOT→X‑Ray behavior is not exercised by this repo's CI.
  • MDC correlation nuance: handler-thread logs now carry the ambient Invocation trace ID, while ExecutionOtelPlugin operation/attempt logs sit in the independent Workflow trace. This is an intentional consequence of separating the traces, but it means handler logs and in-step logs of the same execution correlate to different trace IDs — worth confirming that matches the intended observability story.

Reviewed commit ae9d83664d10a88df71552ab6e15f1bab1c7ddf9. Workflow run

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.

[otel] Scope deterministic trace IDs without replacing provider-wide generation

1 participant