Skip to content

Pro: restore renderer OpenTelemetry trace propagation - #4869

Merged
sashakhar1 merged 8 commits into
mainfrom
sashakhar1/sasha-restore-renderer-otel-propagation
Aug 14, 2026
Merged

sashakhar1 merged 8 commits into
mainfrom
sashakhar1/sasha-restore-renderer-otel-propagation

Conversation

@sashakhar1

@sashakhar1 sashakhar1 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Restores Rails-side OpenTelemetry CLIENT spans and W3C trace propagation for Node Renderer requests after the async-http migration. This reconnects regular, streaming, incremental, raw-render, and asset-upload work to the Rails trace while keeping OpenTelemetry optional and protecting payload privacy.

Closes #4866.

@AbanoubGhadban, this restores the propagation behavior lost during the transport migration in #3320.

Pull Request checklist

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

Other Information

Labels: ready-for-hosted-ci. The change affects shared Pro renderer request paths and should receive optimized hosted CI after local review.

Benchmarks: not applicable. The disabled telemetry path is constrained by allocation regression coverage, and this change does not alter rendering computation.

Follow-ups

  • Add a cross-runtime OpenTelemetry continuity integration test. Reason: The Pro unit suite uses API-faithful fakes and cannot exercise the actual Ruby SDK and Node Renderer extractor together. Future action: After the parallel Node-side OpenTelemetry work lands, add an isolated Puma streaming fixture that verifies one trace id across the Rails span, Rails CLIENT span, and ror.ssr.request.

Summary by CodeRabbit

  • New Features
    • Added OpenTelemetry trace propagation from Rails to the Node Renderer.
    • Added tracing for rendering, streaming, incremental rendering, raw requests, and asset uploads.
    • Captured limited HTTP metadata while excluding request payloads and baggage.
    • Tracing remains optional when OpenTelemetry is unavailable or unconfigured.
  • Documentation
    • Added setup, migration, span taxonomy, and privacy guidance.
  • Tests
    • Added coverage for propagation, streaming, errors, retries, uploads, and unconfigured tracing.

@sashakhar1 sashakhar1 self-assigned this Aug 10, 2026
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c9076fa0-8626-465c-a2b0-9024b8a4c8e8

📥 Commits

Reviewing files that changed from the base of the PR and between 6caf763 and 29a34a5.

📒 Files selected for processing (7)
  • CHANGELOG.md
  • react_on_rails_pro/lib/react_on_rails_pro/renderer_http_client.rb
  • react_on_rails_pro/lib/react_on_rails_pro/request.rb
  • react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb
  • react_on_rails_pro/sig/react_on_rails_pro/renderer_http_client.rbs
  • react_on_rails_pro/spec/react_on_rails_pro/renderer_http_client_spec.rb
  • react_on_rails_pro/spec/react_on_rails_pro/request_spec.rb
🚧 Files skipped from review as they are similar to previous changes (7)
  • react_on_rails_pro/lib/react_on_rails_pro/request.rb
  • react_on_rails_pro/spec/react_on_rails_pro/renderer_http_client_spec.rb
  • CHANGELOG.md
  • react_on_rails_pro/sig/react_on_rails_pro/renderer_http_client.rbs
  • react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb
  • react_on_rails_pro/spec/react_on_rails_pro/request_spec.rb
  • react_on_rails_pro/lib/react_on_rails_pro/renderer_http_client.rb

Walkthrough

React on Rails Pro adds optional OpenTelemetry client spans for Rails-to-Node Renderer requests. It propagates W3C trace context across synchronous, asynchronous, streaming, raw-render, incremental async-props, and asset-upload flows while recording limited HTTP metadata.

Changes

Renderer tracing

Layer / File(s) Summary
Telemetry contract and span lifecycle
react_on_rails_pro/lib/react_on_rails_pro/open_telemetry.rb, react_on_rails_pro/sig/react_on_rails_pro/open_telemetry.rbs
Defines client spans, context helpers, provider detection, path normalization, trace-header injection, and defensive body-size calculation.
Asynchronous rendering context propagation
react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb, react_on_rails_pro/lib/react_on_rails_pro/concerns/*, react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb, react_on_rails_pro/lib/react_on_rails_pro/request.rb, react_on_rails_pro/spec/react_on_rails_pro/request_spec.rb
Restores captured OpenTelemetry contexts across async rendering, cache operations, incremental async props, streaming, and bundle re-upload consumption.
Traced renderer transport
react_on_rails_pro/lib/react_on_rails_pro/renderer_http_client.rb, react_on_rails_pro/sig/react_on_rails_pro/renderer_http_client.rbs, react_on_rails_pro/spec/react_on_rails_pro/renderer_http_client_spec.rb
Adds traced POST, GET, bidirectional, multipart, and streaming request paths with header propagation, status recording, and request/response byte tracking.
Validation and documentation
react_on_rails_pro/spec/react_on_rails_pro/open_telemetry_spec.rb, docs/pro/node-renderer.md, docs/pro/updating.md, CHANGELOG.md
Tests optional instrumentation, propagation, errors, retries, streaming, sizes, and payload redaction. Documents setup, span nesting, privacy, and migration behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 29a34

This change restores optional trace propagation across renderer request paths without introducing a supported merge-blocking concern; it is merge-ready after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant RailsRendering
  participant RendererHttpClient
  participant ClientSpan
  participant NodeRenderer
  RailsRendering->>RendererHttpClient: Execute renderer request
  RendererHttpClient->>ClientSpan: Start CLIENT span
  ClientSpan->>RendererHttpClient: Inject W3C trace headers
  RendererHttpClient->>NodeRenderer: Send request and trace context
  NodeRenderer-->>RendererHttpClient: Return status and response chunks
  RendererHttpClient->>ClientSpan: Record status and byte sizes
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 3.08% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the restored renderer OpenTelemetry trace propagation.
Linked Issues check ✅ Passed The changes implement client spans, trace injection, optional behavior, privacy limits, coverage for required request paths, and documentation updates [#4866].
Out of Scope Changes check ✅ Passed The code, tests, changelog, and documentation changes directly support the linked issue and stated objectives.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sashakhar1/sasha-restore-renderer-otel-propagation

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

fiber.instance_variable_set(:@fake_open_telemetry_context, context)
block.call
ensure
fiber.instance_variable_set(:@fake_open_telemetry_context, previous)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changed. fiber is assigned before the ensure callback is registered, and that callback cannot run before the assignment completes. The focused and full Pro suites exercise this path successfully.

@greptile-apps

greptile-apps Bot commented Aug 10, 2026

Copy link
Copy Markdown

Greptile Summary

Restores Rails-side OpenTelemetry CLIENT spans and W3C trace propagation across regular, streaming, incremental, raw-render, and asset-upload renderer requests.

  • Adds optional OpenTelemetry context capture, propagation, span attributes, response accounting, and path normalization
  • Propagates context across Sync, barrier, streaming, and helper-created fibers
  • Adds RBS signatures, comprehensive unit coverage, changelog notes, and configuration/privacy documentation

Confidence Score: 4/5

The bidirectional failure path should be fixed before merging because an initial payload-write error can leave the newly created renderer span unfinished.

Incremental requests mark their CLIENT span as awaiting request closure before returning the writable body, but an initial write can fail before the later async-task ensure closes that output, preventing the span from reaching its finish condition.

Files Needing Attention: react_on_rails_pro/lib/react_on_rails_pro/renderer_http_client.rb

Important Files Changed

Filename Overview
react_on_rails_pro/lib/react_on_rails_pro/open_telemetry.rb Introduces optional CLIENT-span creation, W3C propagation, privacy-preserving path normalization, byte accounting, and deferred bidirectional completion.
react_on_rails_pro/lib/react_on_rails_pro/renderer_http_client.rb Integrates tracing into every renderer transport path, but bidirectional spans can remain unfinished if the initial request-body write fails before cleanup is installed.
react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb Captures and restores the parent trace context around helper-created asynchronous rendering and streaming tasks.
react_on_rails_pro/lib/react_on_rails_pro/concerns/async_rendering.rb Preserves the caller’s OpenTelemetry context across the top-level Async reactor while retaining barrier cleanup.
react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb Restores the parent context inside the streaming Sync fiber without changing streaming control flow.
react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb Preserves context while consuming and retrying streamed renderer responses.
react_on_rails_pro/spec/react_on_rails_pro/open_telemetry_spec.rb Covers optional operation, propagation, privacy, error status, retries, and request/response size accounting, but not initial bidirectional-write failure cleanup.

Sequence Diagram

sequenceDiagram
  participant Rails as Rails request
  participant Fiber as Async/Sync fiber
  participant Client as RendererHttpClient
  participant Span as OTel CLIENT span
  participant Node as Node Renderer
  Rails->>Fiber: Capture and restore parent context
  Fiber->>Client: Renderer request
  Client->>Span: Start CLIENT span
  Span->>Client: Inject traceparent/tracestate
  Client->>Node: HTTP request
  Node-->>Client: Status and response chunks
  Client->>Span: Record status and byte sizes
  Client->>Span: Finish after response and request closure
Loading

Reviews (1): Last reviewed commit: "Pro: load telemetry status constants dir..." | Re-trigger Greptile


def trace=(trace)
@trace = trace
trace.wait_for_request_close

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 Bidirectional span remains unfinished

When an incremental-render request fails while writing its initial NDJSON payload, the trace has already been marked as awaiting request closure but the async task that closes the output has not yet been created, so the CLIENT span remains unsealed and is never finished or exported.

Knowledge Base Used: Pro Server Rendering Pool: Dispatching Renders to the Node-Renderer

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changed. post_bidi returns a lazy response and the CLIENT span starts only when that response is consumed. The initial NDJSON write happens before response enumeration, so a failure at that point cannot leave a started span unfinished. The bidirectional lifecycle specs cover closure after the span has started.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're right. Tracing build_response(stream: true) through Response.new(&executor) confirms the executor — and therefore start_client_trace — runs lazily inside consume, which is triggered by Response#each or Response#body. Since post_bidi skips the eager response.body call (the unless stream guard in build_response), the CLIENT span is not started until the caller iterates the response. Any failure that occurs during the pre-enumeration NDJSON write phase has no span to leave open. My original comment was incorrect. Thanks for the clarification.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (3)
react_on_rails_pro/spec/react_on_rails_pro/open_telemetry_spec.rb (1)

66-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid permanent singleton patches on a real OpenTelemetry module.

Line 70 skips install_open_telemetry_constants when OpenTelemetry::Context is already defined. In that case lines 73-74 redefine tracer_provider and propagation on the real ::OpenTelemetry module, and nothing restores them. Any later spec in the same process then sees the fake provider. The same pattern appears at lines 205, 217, and 525-526.

Use allow(OpenTelemetry).to receive(...) so RSpec reverts the stubs after each example.

♻️ Proposed change
     allow(provider).to receive(:tracer).with("react_on_rails_pro").and_return(tracer)
-    OpenTelemetry.define_singleton_method(:tracer_provider) { provider }
-    OpenTelemetry.define_singleton_method(:propagation) { propagator }
+    allow(OpenTelemetry).to receive(:tracer_provider).and_return(provider)
+    allow(OpenTelemetry).to receive(:propagation).and_return(propagator)
     span
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@react_on_rails_pro/spec/react_on_rails_pro/open_telemetry_spec.rb` around
lines 66 - 76, Update install_open_telemetry and the matching setup blocks
around the other reported locations to stub OpenTelemetry.tracer_provider and
OpenTelemetry.propagation with RSpec allow(...).to receive(...), rather than
defining singleton methods directly. Preserve the existing fake provider and
propagator return values while ensuring RSpec restores the real OpenTelemetry
behavior after each example.
react_on_rails_pro/lib/react_on_rails_pro/renderer_http_client.rb (1)

201-231: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Seal the trace when WritableBody#close runs.

protocol-http 0.62.2 does not route Writable#close through close_write. An abort such as Output#close(error) therefore skips @trace&.seal_request_size, so ClientSpan cannot finish. Override close and seal the request in ensure, then add a regression test for the abort path.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@react_on_rails_pro/lib/react_on_rails_pro/renderer_http_client.rb` around
lines 201 - 231, The WritableBody lifecycle currently seals the trace only in
close_write, so aborts through WritableBody#close leave ClientSpan unfinished.
Override WritableBody#close to invoke the parent close and ensure
`@trace`&.seal_request_size runs, then add a regression test covering
Output#close(error) and confirming the trace is sealed.
react_on_rails_pro/lib/react_on_rails_pro/open_telemetry.rb (1)

194-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Avoid the private @delegate instance variable

::OpenTelemetry::Internal::ProxyTracerProvider is an internal class, and @delegate is not a public API. The specs define the same private ivar, so they will not detect a future implementation change. Isolate this compatibility check and test it against each supported opentelemetry-api version.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@react_on_rails_pro/lib/react_on_rails_pro/open_telemetry.rb` around lines 194
- 201, Update default_proxy_provider? to avoid directly reading
ProxyTracerProvider’s private `@delegate` instance variable; isolate the
compatibility detection behind a dedicated helper or adapter using supported
opentelemetry-api behavior, and update specs to exercise that compatibility
check across each supported API version rather than defining the same private
ivar.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CHANGELOG.md`:
- Around line 29-35: Update the changelog entry describing Rails-to-Node
Renderer OpenTelemetry trace continuation by appending the PR link and author
attribution in the repository’s standard format, using the guidance from
changelog-guidelines.md and preserving the existing issue reference and
description.

In `@react_on_rails_pro/lib/react_on_rails_pro/renderer_http_client.rb`:
- Around line 186-198: Reset chunk_size at the start of each loop iteration in
MultipartBody#bytesize before conditionally assigning chunk.bytesize, so chunks
without a bytesize method cause the existing non-integer guard to return nil
rather than reusing a previous size.

---

Nitpick comments:
In `@react_on_rails_pro/lib/react_on_rails_pro/open_telemetry.rb`:
- Around line 194-201: Update default_proxy_provider? to avoid directly reading
ProxyTracerProvider’s private `@delegate` instance variable; isolate the
compatibility detection behind a dedicated helper or adapter using supported
opentelemetry-api behavior, and update specs to exercise that compatibility
check across each supported API version rather than defining the same private
ivar.

In `@react_on_rails_pro/lib/react_on_rails_pro/renderer_http_client.rb`:
- Around line 201-231: The WritableBody lifecycle currently seals the trace only
in close_write, so aborts through WritableBody#close leave ClientSpan
unfinished. Override WritableBody#close to invoke the parent close and ensure
`@trace`&.seal_request_size runs, then add a regression test covering
Output#close(error) and confirming the trace is sealed.

In `@react_on_rails_pro/spec/react_on_rails_pro/open_telemetry_spec.rb`:
- Around line 66-76: Update install_open_telemetry and the matching setup blocks
around the other reported locations to stub OpenTelemetry.tracer_provider and
OpenTelemetry.propagation with RSpec allow(...).to receive(...), rather than
defining singleton methods directly. Preserve the existing fake provider and
propagator return values while ensuring RSpec restores the real OpenTelemetry
behavior after each example.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c295d5f6-99d2-4883-aa71-93d585a2be66

📥 Commits

Reviewing files that changed from the base of the PR and between 059fc1a and c32c6f6.

📒 Files selected for processing (12)
  • CHANGELOG.md
  • docs/pro/node-renderer.md
  • docs/pro/updating.md
  • react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb
  • react_on_rails_pro/lib/react_on_rails_pro/concerns/async_rendering.rb
  • react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb
  • react_on_rails_pro/lib/react_on_rails_pro/open_telemetry.rb
  • react_on_rails_pro/lib/react_on_rails_pro/renderer_http_client.rb
  • react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb
  • react_on_rails_pro/sig/react_on_rails_pro/open_telemetry.rbs
  • react_on_rails_pro/sig/react_on_rails_pro/renderer_http_client.rbs
  • react_on_rails_pro/spec/react_on_rails_pro/open_telemetry_spec.rb

Comment thread CHANGELOG.md
Comment thread react_on_rails_pro/lib/react_on_rails_pro/renderer_http_client.rb
Comment on lines +194 to +201
def default_proxy_provider?(provider)
return false unless defined?(::OpenTelemetry::Internal::ProxyTracerProvider)
return false unless provider.instance_of?(::OpenTelemetry::Internal::ProxyTracerProvider)

# The API has no public configured-provider predicate. Reading the delegate avoids allocating a ProxyTracer on
# every no-SDK call.
!provider.instance_variable_get(:@delegate)
end

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fragile reliance on a private opentelemetry-api ivar, with zero coverage against the real gem.

def default_proxy_provider?(provider)
  return false unless defined?(::OpenTelemetry::Internal::ProxyTracerProvider)
  return false unless provider.instance_of?(::OpenTelemetry::Internal::ProxyTracerProvider)
  !provider.instance_variable_get(:@delegate)
end

This is the sole gate deciding whether tracing activates at all, and it depends on reading @delegate off opentelemetry-api's Internal::ProxyTracerProvider — an undocumented implementation detail with no public accessor (as the neighboring comment acknowledges). Two things compound the risk:

  1. opentelemetry-api/opentelemetry-sdk aren't added as even a development dependency anywhere in react_on_rails_pro/ (checked gemspec, Gemfile, Gemfile.development_dependencies, dummy Gemfile), so this is never exercised against the real gem in CI.
  2. open_telemetry_spec.rb only exercises this via hand-rolled stub_const/Class.new fakes (install_open_telemetry_constants) that encode the same @delegate-ivar assumption being tested — so the spec can't catch drift if the real gem's internal representation changes.
  3. Every public entry point here (start_client_span, capture_context, etc.) swallows StandardError silently, so if the ivar name/shape ever changes upstream, tracing just permanently no-ops with no error, warning, or failing test to surface it — it'll look identical to "OpenTelemetry not configured."

Worth at least a smoke test that loads the real opentelemetry-api gem (even just as a dev dependency for this spec file) and asserts default_proxy_provider? behaves as expected against OpenTelemetry::SDK.configure, so a future gem upgrade that changes this internal breaks CI instead of shipping a silent no-op.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changed. There is no public configured-provider predicate, and adding an OpenTelemetry dependency is explicitly outside this task. The private delegate read is required to keep the absent/default-provider path allocation-free; API-faithful tests cover both the default proxy and a configured delegate, and the code comment records the upstream constraint.

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review: Pro OpenTelemetry propagation restoration

Went through open_telemetry.rb, renderer_http_client.rb, the helper/concern context-propagation call sites, and the 596-line spec suite in detail. Overall this is careful, well-tested work — the allocation-free no-op path, the max-of-observed-sizes trick for WritableBody/multipart request sizing, the mutex-guarded finish_if_ready double-checked pattern for the bidi write-close vs. read-complete race, and the query-string/bundle-hash scrubbing in request_path are all correct and thoughtfully handled. Left two inline comments on genuine findings; one more below that can't be attached inline because the file it's in isn't part of this diff.

Not inline-able: async-props task fiber doesn't get OTel context propagated (react_on_rails_pro/lib/react_on_rails_pro/request.rb:189)

Every Async::Barrier#async/Sync spawn point this PR touches (react_on_rails_pro_helper.rb x5, concerns/async_rendering.rb, concerns/stream.rb) follows the same pattern:

parent_context = ReactOnRailsPro::OpenTelemetry.capture_context
...async do
  ReactOnRailsPro::OpenTelemetry.with_context(parent_context) { ... }
end

request.rb's render_code_with_incremental_updates spawns an additional fiber that isn't touched by this PR and doesn't get the same treatment:

tasks.push(Async::Task.current.async do
  async_props_block.call(emitter)
ensure
  output.close
end)

This is the fiber that runs the caller-supplied async_props_block for stream_react_component_with_async_props/rsc_payload_react_component_with_async_props — i.e. the "incremental async-props renders" case the CHANGELOG and PR description explicitly call out as covered. Since this PR's own tests demonstrate that context is not automatically inherited across an Async-spawned fiber (that's the entire reason capture_context/with_context is threaded through every other spawn site), any OTel-instrumented work performed inside async_props_block — DB spans, outbound HTTP calls, or nested react_component/async_react_component calls that themselves hit the renderer — starts as an orphaned trace instead of continuing the Rails request trace.

Suggest capturing parent_context before the tasks.push(...) call and wrapping async_props_block.call(emitter) in ReactOnRailsPro::OpenTelemetry.with_context(parent_context), mirroring the other 5 sites.

Inline comments posted

  • react_on_rails_pro/lib/react_on_rails_pro/open_telemetry.rb:194-201default_proxy_provider? gates all tracing on a private opentelemetry-api ivar (@delegate) that's never exercised against the real gem (not even a dev dependency), only against hand-rolled stubs that encode the same assumption being tested.

@sashakhar1

Copy link
Copy Markdown
Contributor Author

+ci-status

@github-actions

Copy link
Copy Markdown
Contributor

CI Status

Head SHA: cea405345abc
Changed files: 12
Docs-only heuristic (matches ci-changes-detector metadata paths): no
ready-for-hosted-ci label: absent
force-full-hosted-ci label: absent
Current hosted-CI waiver: not present for this SHA
Automatic release-target hosted mode: inactive
Observed exact-head coverage: modes[missing=9]; successful=0, pending=0, failed=0, missing=9

Only the required gate is active unless hosted CI is requested.

while index < @chunks.length
chunk = @chunks[index]
chunk_size = chunk.bytesize if chunk.respond_to?(:bytesize)
return unless chunk_size.is_a?(Integer)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correctness / data-integrity: MultipartBody#bytesize silently reports 0 for the whole upload if any single chunk lacks a measurable size.

def bytesize
  total = 0
  index = 0
  while index < @chunks.length
    chunk = @chunks[index]
    chunk_size = chunk.bytesize if chunk.respond_to?(:bytesize)
    return unless chunk_size.is_a?(Integer)   # <-- bare `return` here

    total += chunk_size
    index += 1
  end
  total
end

The bare return (returns nil) fires as soon as one chunk's bytesize isn't an Integer, discarding the sum already accumulated from every other chunk (boundary strings, headers, other file parts). MultipartFileBody#bytesize (a few lines above) itself returns nil whenever the wrapped IO doesn't respond to #size (e.g. a pipe, socket, or a custom uploadable that only implements #read). In that case OpenTelemetry.body_size maps the nil to 0, so the http.request.body.size span attribute for an asset upload silently reports 0 instead of the aggregate of everything that was measurable — misleading telemetry for exactly the case (bundle/asset uploads) this PR is trying to make observable.

Failure scenario: an asset upload where value[:body] is an IO-like object without #size → the whole multipart body's bytesize becomes nil → span records http.request.body.size = 0 even though megabytes were sent.

Suggest treating an unmeasurable chunk as contributing 0 (next instead of return) so the rest of the sum survives.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changed. nil intentionally means the complete multipart size is unknown; summing only measurable chunks would publish a false partial total as the full request size. All in-tree multipart chunks are currently measurable, and the regression example preserves correct behavior for future unsupported chunks.


# The API has no public configured-provider predicate. Reading the delegate avoids allocating a ProxyTracer on
# every no-SDK call.
!provider.instance_variable_get(:@delegate)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Altitude: silent-failure coupling to a private opentelemetry-api instance variable.

def default_proxy_provider?(provider)
  return false unless defined?(::OpenTelemetry::Internal::ProxyTracerProvider)
  return false unless provider.instance_of?(::OpenTelemetry::Internal::ProxyTracerProvider)

  !provider.instance_variable_get(:@delegate)
end

This is the gate that decides whether the entire feature this PR restores (Rails→Node Renderer trace propagation) is active. It's implemented by reaching into the private @delegate ivar of OpenTelemetry::Internal::ProxyTracerProvider, which the comment itself acknowledges has no public "is the SDK configured" predicate.

Failure scenario: a future opentelemetry-api release renames or removes @delegate (or restructures ProxyTracerProvider entirely). instance_variable_get doesn't raise on a missing ivar — it just returns nil, so !niltrue, default_proxy_provider? always returns true, and configured_tracer_provider returns nil unconditionally. Tracing then silently no-ops for every request — no exception, no log line, nothing in CI unless a test pins the exact gem internals — even though the Rails app has fully configured OpenTelemetry. That's a quiet regression of the very feature this PR exists to restore, with no signal to operators.

Worth at least a version constraint on opentelemetry-api in the gemspec, or a one-time startup log if this heuristic can't classify the provider, so the failure is observable instead of silent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changed. OpenTelemetry exposes no public configured-provider predicate, and this task forbids adding a gem dependency. The private read is documented and covered by API-faithful default-proxy and configured-delegate tests while preserving the required allocation-free disabled path.

final_request_size = ReactOnRailsPro::OpenTelemetry.body_size(@request_body)
@request_size = [@request_size, final_request_size].max
@span.set_attribute(REQUEST_SIZE_ATTRIBUTE, @request_size)
@span.set_attribute(RESPONSE_SIZE_ATTRIBUTE, @response_size)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Simplification: finish redundantly recomputes the same body_size/max calculation seal_request_size already did.

def seal_request_size
  final_request_size = ReactOnRailsPro::OpenTelemetry.body_size(@request_body)
  @request_size = [@request_size, final_request_size].max
  @request_sealed = true
  finish_if_ready
end
...
def finish
  begin
    final_request_size = ReactOnRailsPro::OpenTelemetry.body_size(@request_body)
    @request_size = [@request_size, final_request_size].max
    @span.set_attribute(REQUEST_SIZE_ATTRIBUTE, @request_size)
    ...

Whenever the request side seals first (the WritableBody/bidi path), seal_request_size computes body_size(@request_body) and maxes it in; then once the response also finishes, finish_if_ready calls finish, which recomputes the exact same body_size(@request_body)/max a second time against an unchanged @request_body. It's harmless today (the .max makes it idempotent), but it's duplicated logic in two places that has to be kept in sync — a future change to how request size is measured only in one of them silently diverges from the other. Consider having finish just use @request_size as-is when the body was already sealed, or factoring the compute-and-max step into one shared private method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changed. The second measurement is intentional defensive accounting: the request body can be consumed after an earlier observation, and max preserves the largest complete measurement across normal, streamed, and multipart bodies. The current lifecycle tests verify the final recorded size.

Comment on lines +37 to +40
@request_sealed = true
@response_finished = false
@finished = false
@finish_mutex = Mutex.new

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Simplification: three booleans + a mutex to express "finish once both halves are done."

@request_sealed = true
@response_finished = false
@finished = false
@finish_mutex = Mutex.new

ClientSpan tracks @request_sealed, @response_finished, and @finished (plus @finish_mutex) just to gate a single finish call on "both the request write and the response read completed." This works, but it's more state than the invariant needs — e.g. a single countdown counter (start at 2, decrement per completed side under the mutex, call finish at 0) would express the same rule with one field instead of three, and would make it obvious to a future maintainer exactly what "ready" means instead of requiring them to trace the three-way next false if @finished || !@request_sealed || !@response_finished condition in finish_if_ready. Not incorrect as written, just more surface area than necessary for a two-party join.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changed. The explicit request-sealed, response-finished, and finished states make duplicate notifications idempotent and keep the two-phase lifecycle readable. A countdown would still need duplicate-event protection and synchronization, so it would not reduce the invariant materially.

@sashakhar1

Copy link
Copy Markdown
Contributor Author

Addressed the async-props review finding in 36c9d61 and strengthened its regression coverage in 91376df and 4abcb81. The Rails context is captured before the child task, restored inside the task, and the spec now verifies both context nesting and Fiber placement.

@sashakhar1

Copy link
Copy Markdown
Contributor Author

+ci-run-hosted

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Hosted CI Requested

Triggered 9 workflow(s) for 4abcb81dd23a.
Skipped 0 workflow(s) with equivalent exact-head coverage.
Mode: optimized hosted CI (path-selected by script/ci-changes-detector).
Added ready-for-hosted-ci, so future commits will keep running optimized hosted CI until +ci-stop-hosted is used.

View progress in the Actions tab.

@github-actions github-actions Bot added the ready-for-hosted-ci Run optimized hosted GitHub CI for this PR label Aug 10, 2026

# The API has no public configured-provider predicate. Reading the delegate avoids allocating a ProxyTracer on
# every no-SDK call.
!provider.instance_variable_get(:@delegate)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

default_proxy_provider? distinguishes "SDK configured" from "no SDK" by reading the private @delegate ivar of OpenTelemetry::Internal::ProxyTracerProvider — an undocumented implementation detail of the opentelemetry-api gem, not part of its public contract. The comment acknowledges there's no public predicate for this, which is a reasonable trade-off, but note the risk: since react_on_rails_pro doesn't depend on opentelemetry-api (by design, per the docs), this path is only exercised in this repo's specs via stub_const/fakes (see open_telemetry_spec.rb), never against the real gem. If a future opentelemetry-api release renames/restructures ProxyTracerProvider's internals, instance_variable_get(:@delegate) would silently return nil, default_proxy_provider? would incorrectly report true even when an SDK is configured, and tracing would silently stop working for consuming apps — with no signal in this repo's CI to catch it.

Worth considering: a lightweight CI job (or even a local dev dependency) that runs the spec suite against a real opentelemetry-sdk/opentelemetry-api install occasionally, to catch drift in this private API early — matching the "Follow-ups" note already in the PR description about needing real-SDK integration coverage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed on the integration gap, and no tracked change is made here because this task intentionally forbids an OpenTelemetry dependency. The PR Follow-ups section records a real-SDK cross-runtime continuity test for the first point after the parallel Node-side implementation lands.

return 0 unless body

size = body.bytesize if body.respond_to?(:bytesize)
size.is_a?(Integer) ? size : 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor observability-accuracy nit: when a body responds to bytesize but returns a non-Integer (e.g. nil, as MultipartFileBody#bytesize can for a chunk with no determinate size — see the new "returns an unknown size when a multipart chunk has no byte size" spec in renderer_http_client_spec.rb), this collapses to 0 rather than leaving the size attribute unset/unknown. A 0-byte http.request.body.size/http.response.body.size on a span reads as "empty request," which is misleading when the true size is simply indeterminate (e.g. an upload chunk backed by a custom IO-like object without a fixed size). Not a functional bug — just something that could send someone down the wrong path when debugging payload sizes from trace data. Could consider omitting the attribute entirely (set_attribute not called) when size is unknown, rather than defaulting to 0.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changed for this scope. The task requires request and response byte-size attributes, while an unsupported complete size cannot be represented as a partial total without implying false precision. Current renderer request bodies are measurable; the new regression test ensures future unsupported bodies are classified consistently rather than reusing stale size data.

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review: Pro OpenTelemetry trace propagation restoration

Reviewed with a focus on code quality, correctness/bugs, security, and performance. Overall this is a careful, well-tested implementation — I traced through the fiber/context-propagation logic (capture_context/with_context around every Sync/Async::Barrier#async/Async::Task#async boundary), the ClientSpan request/response sealing state machine (including the WritableBody bidi-streaming case and its trace=/close_write ordering symmetry), and the retry/error-status handling (410 vs 400/412) — and didn't find a correctness bug in the core propagation or span-lifecycle logic. The "max of before/after" trick for computing multipart upload sizes around IO position advancement is a nice touch and is explicitly covered by a test.

Security / privacy: looks sound. Only http.request.method, url.path (normalized to strip bundle hashes/digests via RENDER_PATH_PATTERN, with query strings dropped entirely), http.response.status_code, and request/response byte sizes are recorded as span attributes — never the rendering payload or response body. Header propagation is allowlisted to traceparent/tracestate only; baggage is explicitly excluded (verified in open_telemetry_spec.rb), which is the right call since baggage can carry arbitrary application data across the trust boundary to the Node renderer.

Performance: the "allocation-free no-op on the hot path" claim is verified by dedicated specs for both the no-OTel and default-proxy-provider cases, and the code path backs that up (configured_tracer_provider short-circuits before any allocation).

Two minor, non-blocking points left as inline comments:

  1. default_proxy_provider? relies on reading a private @delegate ivar of OpenTelemetry::Internal::ProxyTracerProvider — reasonable given the API has no public predicate for this, but it's untested against the real opentelemetry-api gem in this repo's CI (only fakes), so a future internal change in that gem could silently disable tracing with no signal here.
  2. body_size collapses an indeterminate chunk size to 0 rather than leaving it unset, which could read as "empty payload" in trace data when the true size is just unknown.

Neither blocks merge; both are refinements for future hardening. Nice work restoring this after the async-http migration, and good foresight calling out the real-SDK integration test gap in the PR description's Follow-ups section.

@sashakhar1

Copy link
Copy Markdown
Contributor Author

+ci-status

@github-actions

Copy link
Copy Markdown
Contributor

CI Status

Head SHA: 4abcb81dd23a
Changed files: 15
Docs-only heuristic (matches ci-changes-detector metadata paths): no
ready-for-hosted-ci label: present
force-full-hosted-ci label: absent
Current hosted-CI waiver: not present for this SHA
Automatic release-target hosted mode: inactive
Observed exact-head coverage: modes[optimized=9]; successful=9, pending=0, failed=0, missing=0

Optimized hosted CI is enabled for this PR.

@sashakhar1
sashakhar1 force-pushed the sashakhar1/sasha-restore-renderer-otel-propagation branch from 4abcb81 to 6caf763 Compare August 14, 2026 20:25
@sashakhar1
sashakhar1 force-pushed the sashakhar1/sasha-restore-renderer-otel-propagation branch from 6caf763 to 29a34a5 Compare August 14, 2026 21:25
@sashakhar1
sashakhar1 added this pull request to the merge queue Aug 14, 2026
Merged via the queue into main with commit 2b27330 Aug 14, 2026
48 checks passed
@sashakhar1
sashakhar1 deleted the sashakhar1/sasha-restore-renderer-otel-propagation branch August 14, 2026 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-hosted-ci Run optimized hosted GitHub CI for this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Pro] Rails-side OpenTelemetry client span and trace propagation lost in the HTTPX to async-http migration

1 participant