Skip to content

feat(sync-service): tail-drop empty shape-GET response spans#4682

Open
alco wants to merge 10 commits into
mainfrom
alco--origin-drop-empty-response-spans
Open

feat(sync-service): tail-drop empty shape-GET response spans#4682
alco wants to merge 10 commits into
mainfrom
alco--origin-drop-empty-response-spans

Conversation

@alco

@alco alco commented Jul 2, 2026

Copy link
Copy Markdown
Member

What

Tail-drop the OpenTelemetry spans of empty / up-to-date shape-GET responses at the origin, to cut exported trace volume. Empty responses are ~95% of Plug_shape_get root spans in production, yet carry no useful trace detail. Part of the ongoing effort to reduce Honeycomb trace volume.

How

  • Electric.Telemetry.EmptyResponseSampler — a pure, unit-tested decision function for the root span's SampleRate weight. In order:
    1. status >= 500SampleRate = 1, never dropped (keep-on-error wins).
    2. empty, non-SSE 2xx + drop enabled → SampleRate = 0 (the drop sentinel).
    3. otherwise → unchanged (whatever the upstream rate hint produced).
  • Electric.Telemetry.OpenTelemetry.EmptyResponseDropProcessor — an :otel_span_processor whose on_end/2 returns :dropped for spans stamped SampleRate = 0. Registered ahead of the exporting processors so that a dropped span is never queued for export. Only empty-response spans carry the sentinel; everything else passes through.

Config

  • ELECTRIC_DROP_EMPTY_RESPONSE_SPANS (boolean, default false) — opt-in; set it to true to enable the drop. Error (5xx) and SSE responses are never dropped.
  • ELECTRIC_LONG_POLL_TIMEOUT (integer, ms) — exposes the existing long-poll timeout as a config knob (used by the integration test to trigger an empty long-poll response quickly).

Fixes #4664

🤖 Generated with Claude Code

alco and others added 3 commits July 2, 2026 18:15
Pure function deciding the SampleRate weight for a shape-GET root span so
the spans of empty/up-to-date responses can be tail-dropped. Keep-on-error
wins first (5xx -> SampleRate=1), empty non-SSE 2xx responses get the
SampleRate=0 drop sentinel when enabled, everything else is left unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add an OTel span processor whose on_end/2 returns :dropped for spans
carrying SampleRate=0, and register it ahead of otel_batch_processor. The
SDK folds on_end with a short-circuiting andalso, so a non-true return
before the batch processor prevents the span from ever being queued for
export. Only empty-response spans carry the sentinel; all others pass
through unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wire the empty-response drop into the request-end SampleRate stamp: empty,
non-SSE 2xx shape-GET responses are stamped with the SampleRate=0 sentinel
so the drop processor skips them at export time. Gated by the new
ELECTRIC_DROP_EMPTY_RESPONSE_SPANS env var (default true); set it to false
to export those spans normally.

Empty/up-to-date responses are ~95% of shape-GET root spans in production,
so dropping them substantially cuts exported trace volume. Error (5xx) and
SSE responses are never dropped. Ratio-based keep-sampling is deferred to
the cloud worker, which will propagate its rate to the origin via
tracestate.

Fixes #4664

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@alco alco added the claude label Jul 2, 2026
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.41%. Comparing base (1b611ca) to head (79aebf8).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4682      +/-   ##
==========================================
- Coverage   60.20%   59.41%   -0.79%     
==========================================
  Files         410      394      -16     
  Lines       44378    43283    -1095     
  Branches    12581    12331     -250     
==========================================
- Hits        26716    25718     -998     
+ Misses      17584    17523      -61     
+ Partials       78       42      -36     
Flag Coverage Δ
electric-telemetry 71.11% <ø> (-2.27%) ⬇️
elixir 71.11% <ø> (-2.27%) ⬇️
packages/agents 72.64% <ø> (ø)
packages/agents-mcp 77.70% <ø> (ø)
packages/agents-mobile 80.67% <ø> (ø)
packages/agents-runtime 83.72% <ø> (-0.03%) ⬇️
packages/agents-server 75.67% <ø> (+0.20%) ⬆️
packages/agents-server-ui 8.32% <ø> (ø)
packages/electric-ax 51.06% <ø> (ø)
packages/experimental ?
packages/react-hooks ?
packages/start 82.83% <ø> (ø)
packages/typescript-client ?
packages/y-electric 56.05% <ø> (ø)
typescript 59.24% <ø> (-0.77%) ⬇️
unit-tests 59.41% <ø> (-0.79%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@claude

claude Bot commented Jul 2, 2026

Copy link
Copy Markdown

Claude Code Review

Summary

Tail-drops the OTel spans of empty/up-to-date shape-GET responses via a SampleRate = 0 sentinel stamped on the root Plug_shape_get span and a first-in-line :otel_span_processor that returns :dropped. Iteration 5 adds one commit since my last pass: empty is now a tag on the electric.plug.serve_shape.requests.count metric, so empty vs non-empty responses stay precisely countable even when their spans are dropped. This directly addresses the count-visibility gap I flagged in iteration 4. No new correctness issues.

What's Working Well

  • The metric tag closes the count-reconstruction gap. My iteration-4 review noted that once the drop is enabled, empties vanish from origin traces with no SampleRate-weighted event to reconstruct true request counts. empty on electric.plug.serve_shape.requests.count fixes exactly that: the metric path is independent of span sampling, so operators keep an exact empty/non-empty split regardless of the drop flag. Good, targeted follow-up.
  • The metric flag is sourced identically to the span attribute and the drop decision. is_empty = response_trace_attrs(conn)[:ot_is_empty_response] || false (serve_shape_plug.ex:447) reads the same response.trace_attrs[:ot_is_empty_response] that feeds the span's shape_req.is_empty_response (line 595) and the EmptyResponseSampler.sample_rate/4 drop decision (line 532). So a dropped span is guaranteed to be the same request the metric counts with empty=true — no drift between "what was dropped" and "what was counted."
  • Tag hygiene is correct. empty is always present in the event metadata (serve_shape_plug.ex:464), so telemetry_metrics never hits a missing-tag value; and it's a boolean, so it only doubles cardinality on that one counter at most. The lux assertion (grep ... "Metric electric\.plug\.serve_shape\.requests\.count .*empty=true") follows the established metric-assertion pattern and verifies the dropped-but-counted invariant end-to-end.
  • Everything from prior iterations still holds. Drop ships default-off; sample_rate_attrs/1 returns base_attrs verbatim when disabled; ELECTRIC_LONG_POLL_TIMEOUT drives the test's empty path; @alco's inline comment thread is resolved.

Issues Found

Critical (Must Fix)

None identified.

Important (Should Fix)

None identified.

Suggestions (Nice to Have)

1. Minor: response_trace_attrs(conn) is now resolved twice per emit. emit_shape_telemetry/1 calls it once for is_empty (line 447), then add_span_attrs_from_conn/1 (line 481) re-resolves the same request_and_response/trace_attrs inside open_telemetry_attrs and sample_rate_attrs. These are cheap map lookups, so this is purely cosmetic — flag only for awareness, not worth a change against the current readability.

2. Orphaned-child robustness comment (carried over, still optional). The processor drops only the single span carrying SampleRate = 0; the empty path is flat by construction (no_change_response leaves chunked: false, so no stream_chunk child), so nothing is orphaned today. A one-line note near the sentinel recording that flat-trace invariant would protect a future child-span addition — the issue's "verify children are not orphaned" ask.

3. Positional booleans in sample_rate/4 (carried over, optional). Three consecutive boolean() args are transposition-prone at future call sites; the single current call site is correct.

Issue Conformance

Fixes #4664. Ships the all-or-nothing boolean (ELECTRIC_DROP_EMPTY_RESPONSE_SPANS, default false) rather than the issue's floated ELECTRIC_EMPTY_SPAN_SAMPLE_RATIO, deferring ratio-based keep-sampling to the cloud worker via the tracestate rate hint — a refinement the issue explicitly invited. Consequence (a) from prior iterations (count under-reporting) is now resolved by the empty metric tag. Consequence (b) remains deferred and only bites once the flag is enabled: when the worker eventually propagates a keep-rate hint, this origin logic still forces SampleRate = 0 for empties (drop overrides the hint), so keeping selected empties will need revisiting then. Fine to defer while the drop ships off.

Previous Review Status

  • Resolved: count-visibility gap — empty vs non-empty is now a first-class metric tag independent of span sampling.
  • Carried over (both optional): orphaned-child invariant comment and positional booleans in sample_rate/4.
  • Still deferred (by design): ratio-based keep-sampling of empties (consequence (b)).

Review iteration: 5 | 2026-07-03

alco and others added 4 commits July 2, 2026 18:37
Extend the otel-export lux suite: an already-up-to-date request returns an
empty no-change response, and the suite asserts a data Plug_shape_get root
span is exported (shape_req.is_empty_response=false) while the empty one is
never exported (no Plug_shape_get span with is_empty_response=true reaches
the collector). Exercises the drop end-to-end through the real OTel export
pipeline.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Extract request_and_response/1 so open_telemetry_attrs/1 and
response_trace_attrs/1 share a single resolver for the request/response
structs instead of each re-deriving them from the conn assigns, keeping the
two in sync.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The empty-response assertion was exercising the wrong path: ot_is_empty_response
is set only by no_change_response, which is reached only when a live long-poll
times out with no new data. A plain non-live request at the current offset
returns is_empty_response=false, so the drop was never triggered and the
assertion passed vacuously.

Use a live request that long-polls until timeout, and assert its body is the
up-to-date control only (no data element) so the drop check is meaningful.
Also replace the bare "0" count match, which flaked on lux prompt-gluing
(SH-PROMPT:0), with the suite's `test ... && echo TOKEN` / `??TOKEN` idiom.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Flip ELECTRIC_DROP_EMPTY_RESPONSE_SPANS to default false so the drop ships
dormant and is opt-in per environment: merging the code changes nothing
until an operator sets it true. The otel-export integration test now enables
it explicitly to exercise the drop.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment thread integration-tests/tests/otel-export.lux Outdated
Comment thread integration-tests/tests/otel-export.lux Outdated
Comment thread integration-tests/tests/otel-export.lux Outdated
Comment thread integration-tests/tests/otel-export.lux Outdated
Comment thread packages/sync-service/lib/electric/plug/serve_shape_plug.ex Outdated
Comment thread packages/sync-service/lib/electric/telemetry/empty_response_sampler.ex Outdated
Comment thread packages/sync-service/lib/electric/telemetry/empty_response_sampler.ex Outdated
Comment thread packages/sync-service/lib/electric/telemetry/empty_response_sampler.ex Outdated
Comment thread packages/sync-service/lib/electric/config.ex Outdated
Comment thread packages/sync-service/lib/electric/telemetry/open_telemetry/config.ex Outdated
alco and others added 3 commits July 3, 2026 17:34
Expose the existing long_poll_timeout config value via
ELECTRIC_LONG_POLL_TIMEOUT so it can be tuned per environment, e.g. shortened
in integration tests to trigger an empty long-poll response quickly.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review feedback: remove over-explanatory module/inline comments, drop
references to the private worker repo and to SDK-internal fold mechanics, and
stop restating default values in comments. Simplify the otel-export lux test to
trigger the empty long-poll response via a short ELECTRIC_LONG_POLL_TIMEOUT
instead of waiting out the default, and tighten its comments.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add `empty` to the [:electric, :plug, :serve_shape] event metadata and to the
electric.plug.serve_shape.requests.count metric's tags. This keeps a precise
count of empty vs non-empty responses via metrics even when their spans are
dropped (ELECTRIC_DROP_EMPTY_RESPONSE_SPANS=true), since the metric path is
independent of span sampling. The otel-export test asserts the dropped empty
response is still counted with empty=true in the exported metric.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@alco alco self-assigned this Jul 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tail-drop empty/up-to-date shape-GET spans at the origin to cut OTel event volume

1 participant