Skip to content

feat(otel,affinity): affinity-path span instrumentation and concurrent forward dispatch - #6164

Open
jonpspri wants to merge 6 commits into
mainfrom
jps-affinity-concurrency
Open

feat(otel,affinity): affinity-path span instrumentation and concurrent forward dispatch#6164
jonpspri wants to merge 6 commits into
mainfrom
jps-affinity-concurrency

Conversation

@jonpspri

@jonpspri jonpspri commented Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

Supersedes #6154. This PR is the rebased origin-branch counterpart and contains the session-affinity, OTEL, and dependency changes:

  • Add affinity-path spans for owner checks, HTTP/RPC forwarding, local dispatch, and forwarded execution.
  • Propagate W3C trace context across session-affinity hops, including signed RPC envelopes and trusted internal dispatch.
  • Add Redis, HTTPX, and SQLAlchemy auto-instrumentation behind explicit settings and compose opt-ins.
  • Dispatch forwarded affinity work concurrently with bounded global concurrency while preserving per-session FIFO ordering, timeout fallback, and weak lock lifecycle management.
  • Add the related concurrency, trace-envelope, instrumentation, and lock-lifecycle regression coverage.
  • Bump h2 to 4.4.1 for CVE-2026-71554 and prune expired exclude-newer overrides.

The portal, Grafana, pgAdmin, and py-spy compose infrastructure remains in the companion monitoring PR, superseding the material originally moved here from #6153.

Closes #6284 — affinity-path span instrumentation and concurrent forward dispatch (performance epic #6104).

This was referenced Aug 17, 2026
Open
Closed
Closed
Closed
Closed
@jonpspri
jonpspri force-pushed the jps-affinity-concurrency branch from 92fa6fc to 7f0d283 Compare August 20, 2026 13:26
@gandhipratik203

gandhipratik203 commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this one, the trace propagation across the three hop types is clean and well commented. A few findings:

Duplicate owner lookup

streamablehttp_transport.py:4311 and :4315 both call get_session_owner(). Two Redis round-trips per request instead of one, in a PR about cutting affinity latency. The span also reports the first read while routing uses the second.

Tautological span tests

test_streamablehttp_transport.py:17980-18052: the three test_affinity_span_attribute_logic_* tests copy the production block into the test body and assert against the copy. No production code runs, which is why the duplicate lookup slipped through. Drive the real handler with a stubbed pool instead.

Unbounded dispatch queue

session_affinity.py:991-1000: the listener spawns one task per message with no cap. The semaphore limits execution to 32 but not queue depth, so bursts pile up with no backpressure. Dropping envelopes older than mcpgateway_pool_rpc_forward_timeout would cap it.

Once these are addressed we're good to go.

@jonpspri
jonpspri force-pushed the jps-affinity-concurrency branch 2 times, most recently from 32580a4 to 3dba821 Compare August 27, 2026 15:57
gandhipratik203
gandhipratik203 previously approved these changes Aug 28, 2026
View reviewed changes

@gandhipratik203 gandhipratik203 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the changes, LGTM!

@jonpspri jonpspri added the MUST P1: Non-negotiable, critical requirements without which the product is non-functional or unsafe label Aug 31, 2026
@jonpspri
feat(otel): affinity-path span instrumentation and redis auto-instrum…
37351eb 
…entation

Deep instrumentation of the session-affinity delegation path, which
showed that cross-worker RPC delegation costs ~85ms while a much larger
pre-handler delay traces to gunicorn workers still initializing
(per-worker plugin manager builds and FastAPI route-state analysis).

- New spans: mcp.affinity.check (owner lookup + decision attributes),
  mcp.affinity.forward_http / forward_rpc (delegation calls),
  mcp.affinity.dispatch_local (same-worker dispatch),
  mcp.affinity.execute_forwarded (owner-side verify + dispatch, nested via
  envelope trace-context attach in the Redis listener), and a
  zero-duration mcp.transport.enter marker at transport-handler entry
- Auto-instrument redis/redis.asyncio clients
  (opentelemetry-instrumentation-redis); pub/sub receive loops are not
  instrumented, so listeners stay quiet
- otel_*_instrumentation_enabled settings now default to false; compose
  opts in explicitly for the observability stack
- Enabled-but-missing instrumentor packages now log at WARNING
- Fix latent test failure (protocol= vs _protocol= kwarg) exposed when
  the observability extra is installed

Signed-off-by: Jonathan Springer <jps@s390x.com>

feat(affinity): concurrent forward dispatch with per-session in-memory ordering

The affinity Redis listener awaited each forwarded execution inline, so
under load the Nth envelope for an owner worker waited ~N x execution
time (observed 1-4.6s forward waits at ~500ms executions). The listener
now spawns bounded dispatch tasks; measured forward wait on the slow
trace cohort drops to 2-49ms.

- Per-session FIFO preserved without Redis: the listener claims an
  in-memory per-session lock synchronously (arrival order) and dispatch
  tasks take it before the global concurrency slot, so same-session
  forwards serialize while different sessions run concurrently
  (mcpgateway_affinity_forward_concurrency, default 32)
- Deadlock safety: session-lock acquire is bounded by
  mcpgateway_affinity_session_lock_timeout (default 30s); on timeout the
  forward executes without the ordering guarantee and logs a warning
- Lock lifecycle handled by weakref.WeakValueDictionary: entries
  evaporate when the last dispatch task drops its reference
- locustfile_echo_delay: use fast_test_server's delay fuzz
  (delay_stddev; ECHO_DELAY_STDDEV_MS, default 25% of ECHO_DELAY_MS)
- Regression tests: same-session non-overlap + cross-session
  concurrency, lock-timeout fallback, lock GC eviction

Signed-off-by: Jonathan Springer <jps@s390x.com>
@jonpspri
fix(deps): bump h2 to 4.4.1 for CVE-2026-71554; prune expired exclude…
243d22c 
…-newer overrides

h2 4.4.1 (2026-08-03) fixes CVE-2026-71554; pinned past the 10-day
soak cutoff via tool.uv.exclude-newer-package as a vetted security fix.

Also remove 21 exclude-newer-package entries whose dates now fall
inside the global 10-day window (no-ops) along with their comment
blocks, and drop the dangling override reference on the authlib pin.
Verified none of the explicit transitive pins are redundant: every
containing package's Requires-Dist floor is looser than the pin.

Signed-off-by: Jonathan Springer <jps@s390x.com>
@jonpspri
feat(otel): propagate W3C trace context across session-affinity hops
e26713d 
Traces for virtual-server requests appeared flat: the trusted-internal
/_internal/mcp/rpc dispatch started a new trace because no W3C context
crossed the hop, and the MCP SDK's session task group (created at app
startup) never sees per-request contextvars.

- OpenTelemetryRequestMiddleware detects requests without a valid remote
  parent (explicit is_valid + is_remote check) and injects the newly
  created root span's context into the ASGI scope headers before invoking
  the app, so downstream raw-header copies (session-task handoff, affinity
  envelopes, trusted-internal dispatch) carry the trace.
- post_rpc_in_process injects the active trace context; the affinity LOCAL
  and http_forward paths now route through it instead of inline duplicate
  dispatches, and the http_forward consumer carries traceparent/tracestate
  explicitly past the passthrough allowlist.
- rpc_forward envelopes embed the trace context before signing.

Signed-off-by: Jonathan Springer <jps@s390x.com>
@jonpspri
fix(otel): remove redundant create_span imports
f86acd3 
Signed-off-by: Jonathan Springer <jps@s390x.com>
@jonpspri
test: improve diff coverage for affinity/observability/db/transport gaps
1bd01f3 
Cover the 33 missing lines identified in the diff coverage report:

mcpgateway/db.py (lines 252-260)
- Add reload-based tests for the OTel SQLAlchemy instrumentation block:
  success path (SQLAlchemyInstrumentor called with engine) and ImportError
  path (warning logged when package unavailable).

mcpgateway/observability.py (lines 1247-1255)
- Add three tests for the Redis instrumentor block inside init_telemetry:
  successful instrumentation, exception during instrument() (non-fatal
  warning), and REDIS_INSTRUMENTOR=None (package-unavailable warning).

mcpgateway/services/session_affinity.py (lines 88-97, 108-114, 717, 1090-1091, 1100-1104)
- _attach_envelope_trace_context: ImportError returns None, missing/empty
  headers returns None, valid traceparent attaches context (skipif otel
  absent), attach exception returns None (skipif otel absent).
- _detach_envelope_trace_context: None token is no-op, real token calls
  otel detach (skipif otel absent), import failure is swallowed silently.
- drain_all: assert running _forward_tasks are cancelled.
- _dispatch_forwarded: broad-except logs warning and does not re-raise.
- _on_forward_task_done: cancelled task discarded without calling exception(),
  task exception logs warning, no exception produces no warning.

mcpgateway/transports/streamablehttp_transport.py (lines 4312-4314)
- Three direct tests for the affinity span attribute logic: owner matches
  WORKER_ID (decision='local'), owner is different worker (decision='forward'),
  owner is None (owner attribute 'none', decision 'local').

Signed-off-by: Jonathan Springer <jps@s390x.com>
@jonpspri
fix(affinity): address PR review findings
f0662d3 
- Remove duplicate get_session_owner() Redis round-trip in
  handle_streamable_http: hoist the single call before the span block
  so both span attribute recording and the forward decision use the
  same result (streamablehttp_transport.py:4309-4315).

- Replace tautological affinity span attribute tests with real handler
  tests: each of the three test_affinity_span_attribute_logic_* tests
  copied the production if-block verbatim and asserted against the
  copy.  Replaced with async tests that drive handle_streamable_http
  with a stubbed pool, patched create_span (non-None sentinel) and
  set_span_attribute capture, so production code is exercised.

- Add stale-envelope backpressure in _dispatch_forwarded: envelopes
  carrying a timestamp older than mcpgateway_pool_rpc_forward_timeout
  are dropped with a WARNING before acquiring the session lock,
  preventing burst back-log from accumulating work the originating
  caller has already timed out on.  Envelopes without a timestamp
  (legacy/synthetic) pass through unchanged.  Two new tests cover the
  drop and execute paths; existing listener test updated to set
  mock_settings.mcpgateway_pool_rpc_forward_timeout.

Signed-off-by: Jonathan Springer <jps@s390x.com>
@jonpspri
jonpspri force-pushed the jps-affinity-concurrency branch from 3dba821 to f0662d3 Compare September 1, 2026 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gandhipratik203 gandhipratik203 gandhipratik203 left review comments
@crivetimihai crivetimihai Awaiting requested review from crivetimihai crivetimihai is a code owner
@brian-hussey brian-hussey Awaiting requested review from brian-hussey brian-hussey is a code owner
@Lang-Akshay Lang-Akshay Awaiting requested review from Lang-Akshay Lang-Akshay is a code owner
@ja8zyjits ja8zyjits Awaiting requested review from ja8zyjits ja8zyjits is a code owner
@msureshkumar88 msureshkumar88 Awaiting requested review from msureshkumar88 msureshkumar88 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

MUST P1: Non-negotiable, critical requirements without which the product is non-functional or unsafe

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[PERF]: Affinity-path span instrumentation and concurrent forward dispatch

2 participants

@jonpspri @gandhipratik203