Propagate OpenTelemetry context to async thread pool workers#46
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
tycooon
left a comment
There was a problem hiding this comment.
Reviewed the OTel propagation and the Ruby 3.3 modernization sweep.
The core logic is sound: yield inside the wrapper block correctly reaches the caller's block on the worker thread (and avoids the Ruby 3.3 SyntaxError on anonymous forwarding inside block bodies), attach/detach are correctly paired via ensure on whichever thread executes — including PreemptableProxy's preemption path and its exception rethrow — every hash-shorthand site resolves to a real local/kwarg/let, the checked_transaction/migrator/upsert call sites still match, and keeping opentelemetry-api out of the gemspec is the right dependency design.
Inline comments below: versioning of the release, the shape of the defined?(OpenTelemetry) gate, two test-coverage gaps, and a few simplifications.
One non-blocking note: apps that enable the standard opentelemetry-instrumentation-concurrent_ruby (it patches ThreadPoolExecutor#post with the same with_current wrapping) will get harmless double propagation on top of this. A short README note about the new behavior would cover that and general discoverability.
* Refactor async_run to properly handle OpenTelemetry context propagation in async jobs * Refactor rspec by isolating OpenTelemetry context propagation logic * Update upsert method documentation to clarify parameters and usage example * Bump umbrellio-sequel-plugins version to 0.19.0
tycooon
left a comment
There was a problem hiding this comment.
Re-review of the current head (b55d1ee) against origin/master, focused on what changed since my 2026-07-03 review — the #47 refactor. release.yml is excluded from this review per request.
All eight findings from the previous review are addressed in the code:
- gemspec →
0.19.0withrequired_ruby_version >= 3.3(matches the 3.3/3.4 CI matrix) - gate is now
defined?(OpenTelemetry::Context)— no moreNameErrorwhen only a partial/stub OTel is loaded (I confirmeddefined?returns nil rather than raising for a non-moduleOpenTelemetryon 3.3/3.4) - ROOT short-circuit added, so an app with opentelemetry-api loaded but no active span keeps the zero-overhead plain path
- attach/detach delegated to
OpenTelemetry::Context.with_current - plain/non-OTel branch is now covered (the
stub_const"defined without Context" example genuinely hits theelse), and therequire "opentelemetry-api"is isolated to its own spec file - detach/cleanup is covered — "restores context after block completes" on a single-thread executor, plus the raise case
- the no-op
detach(attach(ROOT))hook is gone - upsert YARD now documents
target:with a corrected example
I verified locally that the anonymous-&-inside-a-block form (with_current(otel_context, &)) both parses and forwards the caller's block correctly at runtime on Ruby 3.3 and 3.4 — my earlier worry about a 3.3 SyntaxError there was unfounded. CI is green on both.
One optional simplification inline. Nothing blocking on the code I reviewed.
No description provided.