Repository navigation
perf: scalable, non-blocking event pipeline with a pooled autoscaling dispatcher - #202
ceolinrenato wants to merge 36 commits into
Conversation
Switch ldclient_event_server:add_event/3 from gen_server:call to a cast so flag evaluations no longer block application callers on the analytics pipeline. Publish a shared counters-based buffer depth per tag; callers check it before casting and shed (drop + telemetry [ldclient, events, shed]) once the configured events_shed_threshold is reached. This bounds the event server mailbox and memory under overload. Also track the buffered event count in state instead of calling length/1 on every insert, making the capacity check O(1).
Replace the single event dispatcher process with a pool of reporter
workers coordinated by the event server:
- Add ldclient_event_buffer, an ETS-backed FIFO that holds pending raw
events off the owning process heap.
- Add ldclient_event_worker_sup, a simple_one_for_one supervisor that
commissions reporter workers per tag.
- The event server now owns the buffer and, on each flush, hands batches
to idle workers while tracking busy workers, so concurrency is bounded
and worker mailboxes cannot grow unbounded.
- ldclient_event_process_server becomes a poolable worker that formats,
encodes and dispatches a single batch, then reports back to the event
server via {worker_done, Pid}.
- Add events_min_workers, events_max_workers and events_batch_size
options. min/max default to 1, preserving the previous single-worker
behaviour and wire ordering.
The event server periodically samples the buffered event depth and commissions or decommissions reporter workers between events_min_workers and events_max_workers: - Scale up when depth reaches events_scale_up_threshold. - Scale down an idle worker once depth falls to or below events_scale_down_threshold. - A cooldown (events_scale_cooldown_ms) and sampling interval (events_scale_interval_ms) prevent thrashing. - Emit [ldclient, events, pool_scale] telemetry with the new worker count and direction on every change. - Workers are only ever removed while idle, and the pool never shrinks below the configured minimum. min_workers and max_workers both default to 1, so autoscaling is opt-in and existing behaviour is unchanged.
Reporter workers now react to LaunchDarkly API responses instead of retrying transient failures at a fixed 1s interval: - Temporary failures are retried with exponential backoff (1s initial, 60s cap) plus jitter. - Permanent failures pause egress for the batch for 60s so a misconfigured SDK does not hammer the events endpoint. - Both cases emit [ldclient, events, send_error] telemetry with the failure type.
The contract-test service builds ldclient as a rebar3 checkout and lists its transitive dependencies explicitly. Add telemetry so the release continues to assemble and boot.
The existing events allow counters for shedding and send errors, but there was no signal for successfully delivered events and the pool signal was only a transition, not a gauge. This adds: - [ldclient, events, published] with measurement count (events delivered per batch) emitted on every successful dispatch, backing a published events counter. - Rename the pool transition event to [ldclient, events, pool_size] and always emit the absolute workers value on change and at startup, so it can back a gauge metric. direction metadata (initial/up/down) is kept for context. Together with [ldclient, events, shed] and [ldclient, events, send_error] this gives the four requested metrics with counters and a gauge.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Address review findings in the event pipeline: - Offline instances and send_events => false were only checked on handle_call; the new add_event cast bypassed them, so events were buffered and later sent. Add guards to handle_cast. - Caller-side shedding dropped feature request events before summary aggregation, under-counting usage when the buffer was full. Feature requests are now always processed so summary analytics stay accurate; only best-effort identify/custom events are shed. - Permanent dispatch failures were retried forever, retaining a timer and payload per failed batch. They are now dropped (as the SDK did before), avoiding unbounded memory and endpoint hammering. - Scale-down terminated idle workers that were mid-retry, losing events. Decommissioning is now graceful: the worker is removed from the pool but only exits once it has no outstanding retries. - flush/1 waited on HTTP with the default 5s call timeout and the flush timer was re-armed on completion. Use an unbounded call timeout and re-arm the timer when the flush window starts. Adds regression tests for each case (330 tests total).
|
Thanks @cursor[bot] — reviewed all five. Four were genuine regressions against the previous behaviour (and against the SDK's documented summary semantics); all are fixed in
Note on the shedding metric: because feature requests are no longer shed, |
Address the second round of review findings: - A decommissioned worker only stopped when it had no outstanding retries, but a sustained temporary outage reschedules on every attempt, so the worker never exited and leaked (the supervisor could also start a replacement since it was already out of the pool). It now makes at most one more attempt after decommission and then exits. - An explicit flush waited until the buffer was empty, but feature requests always enqueue, so under sustained evaluations new events could keep extending the window indefinitely. A flush now captures the event count at the start of the window (flush_remaining) and completes once exactly those events and their summary have been dispatched; events that arrive later belong to the next window. Adds a slow-dispatcher regression test for the flush window and extends the decommission test to assert the worker eventually exits (331 tests).
|
Second round reviewed — both new findings were valid and are fixed in
331 tests pass; dialyzer clean. |
pop_batch took up to events_batch_size events rather than the remaining count for the current flush window. When drain paused waiting for a busy worker, events inserted after the window started sat at the tail of the buffer, so the next pop could pull next-window events into the current flush (the default one-worker/batch-size-100 configuration hits this whenever more than one batch is outstanding). Cap each pop at min(events_batch_size, flush_remaining) so a batch can never cross the window boundary. Adds a regression test that fails without the cap (332 tests).
|
Reviewed — this one was valid and is fixed in Flush batch exceeds window bound — Confirmed. Fixed by capping each pop at I also verified the regression test actually catches it: |
The maintainer clarified in launchdarkly#190 that the intended contract is to retry a temporary dispatch failure exactly once and stop on permanent failures. The previous phase replaced the fixed 1s retry-once with indefinite exponential backoff, which diverges from that contract and keeps rescheduling during a sustained outage. Restore retry-once: the first temporary failure schedules a single retry after 1s; if that retry fails the batch is dropped. Permanent failures are still dropped. This also removes the only source of unbounded retry rescheduling.
|
Reverted the exponential backoff retries this PR once had because it simplifies things a little bit and matches the pre-existing behavior, but we can revert commit fc855af if the exponential backoff for retrying more than once made sense/was an improvement. |
|
Hello @ceolinrenato, Thank you for the contribution. There is a lot going on here so it may take a bit to get through, but I don't have concerns with a number of the core ideas. Thank you, |
Thanks for replying so quickly! I understand it's a lot to review and might take some time. While you're at it there's two conservative approaches I took (in order to maintain current behaviour / be less disruptive) which you might wanna evaluate: 1 - Do we keep transient errors being retried only once or we let them be retried indefinitely with exp backoff and jitter? 2 - Should we change the pool size defaults so all users benefit from parallelism with the default configs? |
For now, we want to only retry transient errors 1 time. We are working on some projects that may change this, but I want to keep it consistent with our existing behavior for now.
Thank you, |
|
Hello @ceolinrenato, It appears this approach has some of the same unbounded growth problems as referenced from the previous discussions. I did some load tests with different rates of evaluations and observed mailbox size, vm memory, and other stats. These are the two primary images that demonstrate the concern. While I agree we need to decrease the back pressure, I also don't want unbounded growth to be a characteristic. I tested couple fixes in addition to your changes, but I still need to weight their impacts, and against options which reduce, but still have, a back pressure mechanism but potentially much more performant. The evaluation performance itself was good. |
|
I think we would want to extend the flush workers, as the memory behavior is triggered by a mismatch in flush performance and evaluations being generated. But in addition there would have to be some additional emergency shedding. I can get something more like this: Which probably would still be something that would require being opt-in. |
|
So green is PR with extended/more flush workers? I think to prevent unbounded growth we would need to shed all types of events not just some of them. |
The event intake process called lru:contains_or_add/3, a synchronous gen_server call into a single per-tag process, for every feature request, custom and identify event. That was the last serialization point on the intake path and a major reason the event server mailbox could grow under high evaluation rates. Replace it with two generations of ETS sets and an atomic ets:insert_new/2 check, so the seen-context lookup needs no message round-trip. When the current generation reaches context_keys_capacity the oldest generation is dropped, bounding memory at roughly twice the capacity while still remembering contexts from the last generation. This removes the lru dependency from the SDK and the contract-test service.
The caller-side shed guard only measured buffered events and exempted feature requests, so under an evaluation flood the event server mailbox could still grow unbounded (Ryan observed it reaching ~10^7 messages and multiple GB of memory in load tests). Track outstanding work with a second counter slot: every cast reserves an in-flight slot that the server releases when it processes the message, so events_shed_threshold now bounds mailbox depth plus buffered events. Introduce events_shed_all (default false): when enabled, feature request events are shed under the same bound as an emergency memory valve, trading summary accuracy while shedding. Off by default so summary analytics keep counting every evaluation. Adds a regression test that a feature request is shed and the summary under-counts by exactly that event when the option is enabled.
Add [ldclient, events, flush], emitted once per delivered batch including any retry, with measurements count (events in the batch, including the summary), size (encoded payload bytes before compression) and duration (native time from the first attempt to resolution), and metadata tag and outcome (accepted | failed). This lets a recorder derive the standardized instruments (flush count, batch size, flush duration, sent, sent.size and failed) from telemetry alone, instead of only published/send_error which are per attempt. Documents the event and adds tests for the accepted and retried-failed cases.
… a per-instance httpc profile Two review findings on the previous fix commit. The per-event JSON encoding fallback caught only error:badarg. jsx raises error:function_clause for maps whose keys are strings, floats or tuples and for improper lists, all of which are type-valid custom-event data or variation defaults. Such a term still crashed the reporter worker, the re-dispatch crashed a second one, and the whole flush window including its summary was dropped, with the raw batch printed in two crash reports. The fallback now treats every error jsx raises as "not JSON". Because the summary is one output event covering every flag, an unencodable application-supplied default value (and, for an unknown flag, the counter value derived from it) is replaced by null instead of dropping all counters. All reporter workers used httpc's default profile. Its manager queues up to max_keep_alive_length (5) requests behind the one in flight on a keep-alive connection and opens at most max_sessions (2) connections, so several workers posting at once were served one after another, and a batch handed to a worker started on demand waited behind a hung request for the whole request timeout before it was even sent. Each instance now gets its own httpc profile in which a connection is reused only when it is idle and up to events_max_workers persistent connections may be open; proxy and socket options set on the default profile are carried over. The dispatcher behaviour gains an optional stop/1 callback, called from the event server's terminate, so the profile is released with the instance. The httpc dispatcher suite now starts the application itself so it also runs on its own.
| case Result of | ||
| ok -> | ||
| emit_published(Tag, OutputEvents), | ||
| emit_flush(Tag, OutputEvents, Size, StartTime, accepted), |
There was a problem hiding this comment.
Flush telemetry counts unencodable events
Low Severity
[ldclient, events, flush] uses the pre-encode batch for count, so events already reported as dropped (reason => unencodable) are also counted as flushed. When every event is unencodable, send returns success with size 0 and the flush is emitted as accepted even though nothing was posted, which inflates sent/flush metrics derived from this event.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 27a1676. Configure here.
|
Hmm I thought of adding 27a1676 to close the gap with your telemetry table but looks like cursor had some issues with it and we were working simultaneously haha, I'll stop commuting right and let you polish this! We might also consider renaming shed to dropped to unify both events using diff reasons Thanks again |
… analytics events Evaluating a flag with an invalid context (a missing key, or a kind or key that is not a binary) only validated the context once the flag had been found. When the flag was unknown or deleted, or when evaluating it raised, the evaluator still created a feature event carrying the invalid context, and the event server crashed when it derived the context's canonical key for index-event deduplication. Each such evaluation lost the buffered events and the open summary; a burst of them exceeded the event supervisor's restart intensity and restarted the instance supervisor, which came back without its feature store and update processor while initialized/1 still returned true. With the synchronous add_event the caller at least saw the exit; with the asynchronous pipeline in #202 the failure is silent. Every place that creates an event now validates the context first. The unknown-flag, deleted-flag and exception results go through one helper that returns the default value with {error, user_not_specified}, logs a warning and records no event when the context is invalid, as the other server-side SDKs do; the known-flag path keeps its existing check and now logs the same warning. A valid context is therefore validated exactly once per evaluation and the known-flag path costs the same as before. is_valid/2 is wrapped so a term that is not a context at all counts as invalid instead of raising.
…port drops and published counts exactly Follow-ups to the review of the bounded event pipeline. The event server is registered before init/1 runs and callers admit without counting while no counters are published, so every cast arriving while the worker pool started was queued uncounted (95 000 messages measured with a slow worker start). The counters are now published first with the gate closed, so those callers shed instead, and the forced resync opens the gate once a worker exists. A failed start erases them again. active_workers/1 counted idle and busy workers only. A decommissioned worker still attempting its pending retry is a live process holding a batch and a connection, so the pool could grow past events_max_workers while it lingered (4 live workers with a maximum of 2). Every monitored worker now counts. Capacity drops are reported once per flush window; a window that ended with the server exiting never reported them. terminate/2 now does. The worker encodes a batch once and the retry resends the same bytes, so an unencodable event is dropped and reported exactly once, and the published and flush telemetry carry the number of events actually in the request rather than the size of the batch before encoding. A batch with nothing left to send publishes nothing. Millisecond intervals and the request timeout are capped at 2^32 - 1 ms: larger values raised in erlang:send_after/3 at startup or left a request waiting forever in httpc. Out-of-range values fall back to the default with a warning like every other invalid option. events_scale_up_threshold is removed: nothing read it since workers started being added on demand, and it was never released. Tests cover each change; a flush or scale timer landing in the mailbox no longer trips the inbox-bound test.
Sounds good. I am working through a number of edge cases, and may end up simplifying things like the worker scaling and instead go to just a fixed, but configurable pool. I am also opening up some smaller adjacent changes for issues that are made worse when combined with this PR. Thanks again for the contributions. It may yet be a few days before I get this one sorted. |
Sounds good @kinyoklion I think a fixed (but configurable) pool would work fine as long as we have the telemetry on dropped events because of ETS buffer capacity. As users could watch the metric and increase the pool if needed |
… analytics events (#204) **Requirements** - [x] I have added test coverage for new or changed functionality - [x] I have followed the repository's [pull request submission guidelines](../blob/main/CONTRIBUTING.md#submitting-pull-requests) - [ ] I have validated my changes against all supported platform versions (run locally on OTP 25.3; CI covers the rest) **Related issues** Found while reviewing #202. The fix is independent of that PR and applies to its branch unchanged (the evaluator is not modified there). **Describe the solution you've provided** Evaluating a flag with an invalid context (a missing key, or a kind or key that is not a binary) only validated the context once the flag had been found. When the flag was unknown or deleted, or when evaluating it raised, the evaluator still created a feature event carrying the invalid context, and the event server crashed when it derived the context's canonical key for index-event deduplication. Each such evaluation lost the buffered events and the open summary. A burst of them exceeded the event supervisor's restart intensity and restarted the instance supervisor, which comes back without its feature store and update processor while `initialized/1` still returns true. With the synchronous `add_event` on `main` the caller at least saw the exit; with the asynchronous pipeline in #202 the failure is silent. Every place that creates an event now validates the context first. The unknown-flag, deleted-flag and exception results go through one helper (`unknown_flag_result/4`) that returns the default value with `{error, user_not_specified}`, logs a warning and records no event when the context is invalid, as the other server-side SDKs do. The known-flag path keeps its existing check in `flag_for_context_check_valid/5` and now logs the same warning. A valid context is validated exactly once per evaluation, so the known-flag path costs the same as before; the unknown-flag path pays one `is_valid/2` call (about 130 ns for a single-kind context) on top of the warning it already logs. `ldclient_context:is_valid/2` is wrapped so a term that is not a context at all counts as invalid instead of raising. Tests: `unknown_flag_with_invalid_context` (five invalid shapes against an unknown, a deleted and a known flag), `exception_with_invalid_context_sends_no_event` (the feature store raising, as it does once the store has been lost; a valid context keeps the existing exception event, an invalid one records nothing) and `invalid_context_does_not_crash_the_event_server` (public API; the event server keeps its pid and still delivers afterwards). All three fail on `main` without the change. **Describe alternatives you've considered** Checking the context once at the top of `flag_key_for_context/4` is simpler to reason about but validates known flags twice (the per-flag check is still needed by `all_flags_state`, which calls the lower-level function directly) and changes the result for an invalid context on an offline or not-yet-initialised instance from `client_not_ready` to `user_not_specified`. Guarding `ldclient:variation/4` and `variation_detail/4` the way `identify` and `track` already do would cover the public API but not internal callers. Making the event server tolerate a bad context would hide the error from the caller and would have to be done separately on `main` and in #202. **Additional context** Also reproducible with `ldclient:variation(<<"missing-flag">>, #{kind => <<"org">>, key => 42}, false)`. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **Invalid contexts no longer emit analytics events** on unknown-flag, deleted-flag, or evaluation-exception paths—preventing the event server from crashing when it canonicalizes a bad context. > > Evaluation now routes those outcomes through **`unknown_flag_result/4`**, which checks **`is_valid_context/1`** first. Valid contexts keep prior behavior (including unknown-flag events and warnings for `flag_not_found`). Invalid contexts use shared **`invalid_context_result/2`**: default value, `{error, user_not_specified}`, a warning, and **no events**—aligned with the known-flag path and other server SDKs. > > The exception handler in **`flag_key_for_context/4`** uses the same helper instead of always building an unknown-flag event. **`flag_for_invalid_context`** now logs the same invalid-context warning via **`invalid_context_result`**. > > Tests cover invalid context shapes against unknown/deleted/known flags, exception + invalid context (no event), and that the event server stays alive through the public API. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 5315a2c. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Follow the other server-side SDKs (Go, Java, .NET, Python, Ruby): a fixed pool of events_flush_workers reporter workers (default 5, at most 1024) started with the event server and kept at that size, instead of a pool that grows on demand and decommissions idle workers. A flush that finds every worker still busy with a previous request is deferred: the buffered events and the open summary stay where they are, [ldclient, events, flush_skipped] reports it, and the flush starts as soon as a worker is free (one deferred flush at a time, like the single waiting payload in the Go and Java SDKs). A flush with nothing to send is a no-op. Exited workers are replaced before the pool is judged busy, and again on the housekeeping tick (events_housekeeping_interval_ms, default 1000), which also clamps the queue counter. The periodic timer is re-armed when it fired and when an explicit flush starts a window; a deferred explicit flush leaves the schedule alone. Removed: on-demand growth in drain/1, the scale tick and its decision (scale/2, scale_down/2, maybe_scale/1), worker decommissioning (the worker's decommission/1, stop_if_idle_decommissioned/1, decommission and pending fields), the usable-versus-live worker accounting, and the options events_min_workers, events_max_workers, events_scale_down_threshold, events_scale_interval_ms and events_scale_cooldown_ms. The per-instance httpc profile allows one connection per worker. With this, at most one summary is ever pending and the pool never has more live workers than its size. Tests: the autoscaling and decommissioning cases are replaced by flush_while_every_worker_is_busy_is_deferred (busy, no-op and deferred paths), default_pool_size and pool_never_exceeds_its_size.
…mes out
An events endpoint that accepts the connection (and the TLS handshake) but
then stops reading leaves the request body queued in the client socket. The
request timeout fires on time and frees the worker, but a socket closed with
output still queued is kept by the inet driver, holding the payload, until
the peer closes its side. With one request per flush and a retry, that is
two sockets per flush for the length of the outage, and they survived
stop_instance.
send_timeout cannot fix this: a single large send is accepted into the port
queue at once, so the port never becomes busy and the timer never arms.
Instead the per-instance httpc profile sets {linger, {true, 0}} on its
sockets, merged with any socket options inherited from the default profile,
so closing a connection discards whatever is still queued and releases the
port immediately. The close is abortive (a reset instead of a FIN), also for
idle keep-alive connections, whose responses have been read by then.
Regression tests post an 8 MiB body to a non-reading endpoint over tcp and
over tls with a 500 ms request timeout and check that no tcp port with
queued output remains.
…f the hot path
Align the events telemetry with what LaunchDarkly's OpenTelemetry metrics
specification derives from its eventFlushCompleted hook, so a telemetry
handler can record the spec's instruments.
- [ldclient, events, shed] is folded into [ldclient, events, dropped], which
now carries reason => capacity | inbox | shed_threshold | unencodable; each
reason names the option that bounds it. Drops with reason inbox are the
only ones whose evaluations are missing from the summary.
- Caller-side drops are counted in two new slots of the shared counters
instead of emitting a telemetry event per shed event in the evaluating
process. The event server takes the counts at every flush (also deferred,
empty, and at shutdown) and emits one dropped event and one warning line
per reason, as the spec requires drops to be reported with the flush that
follows them. Drops counted against a crashed incarnation's counters are
carried over and reported by the next incarnation's first flush.
- A dispatcher reports the status code of an HTTP error response as a fourth
element, {error, temporary | permanent, Reason, StatusCode}; the
three-element form is still accepted. flush (on failure) and send_error
carry status_code when the last response had one, for the spec's
error.type and http.response.status_code attributes.
- The flush outcome is succeeded | failed, the spec's attribute values.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
Reviewed by Cursor Bugbot for commit 1278b35. Configure here.
…o longer cascades
Three small hardenings around the event server's fail-fast init:
- inherited_options/0 tolerates an error from httpc:get_options/1. An
application that has stopped inets' default httpc profile gets an event
pipeline that inherits no network options (one warning), instead of a
worker that cannot start, an event server that cannot start, and a
supervisor restart loop that exhausts the application's intensity in
milliseconds.
- When the event server's init fails because no worker could start, the
dispatcher's stop/1 is called, so the per-instance httpc profile is not
left running under inets.
- start_instance returns {error, start_failed, Reason} when the instance's
supervisor fails to start, and unregisters the settings, instead of a
case_clause in the caller.
- start_instance for an instance that ldclient_sup is restarting returns
{error, already_started, restarting} and leaves its settings alone, instead
of unregistering them as a failed start (after which no restart could
succeed).
- A restarted event server reuses the counters its crashed predecessor left
published (same bounds), so the drops callers counted during the restart
are reported by its first flush with no hand-over window; the
persistent_term entry is no longer replaced on a restart. Drops counted
while a start attempt runs are reported even when the attempt fails.
- A flush that finds a window still open after replacing a lost worker feeds
the window and defers itself instead of reopening it; at most one summary
is pending again, as the comment in open_window claims.
- Inherited socket options of any shape ({raw, _, _, _}, atoms) reach the
instance httpc profile; only a linger is replaced.
- A failed flush reports the status code of the last response received: the
first attempt's when the retry got no response. Status codes that are not
integers in 100..599 are not put in telemetry metadata.
- Docs: the request timeout also bounds the upload of a payload and TLS
sockets are released within about 5 s; dropped is emitted at every flush
trigger; stop/1 is also called after a failed start; error.type and
http.response.status_code also apply to events.flushes; duration
conversion. The inbox log line no longer blames overload for drops during
a start, and the default-profile warning names the instance.
- Tests: regression tests for each item (restart race, counter reuse,
reporting on a failed start, window feeding, raw socket options, status
codes), a steerable test dispatcher replacing the init-fail one, the
stalled-socket tests check only connections to their own server and allow
the TLS close timeout.
The SDK only calls telemetry:execute/3 (tests add attach/4, attach_many/4 and detach/1), all of which exist since telemetry 1.0.0. Requiring ~> 1.3 would make hosts that pin an older 1.x fail dependency resolution for no benefit. The lock stays at 1.4.2; the suites also pass against 1.0.0.
|
Hey @kinyoklion hope you're having a nice day! How are the tests for this going, promising results? |
|
Hello @ceolinrenato, Things have been looking good. Working through this PR I did identify a number of secondary issues which this PR would amplify, and have been getting those resolved ahead of merging this PR. I think though that all potentials for unbounded mailbox growth have been resolved. I also need to do one more pass to validate which options are exposed, and cleanup some of the shedding concepts to make sure we are consistent with other SDK and meet the expectations of our products like experimentation. I appreciate your patience as there are a number of other issues which have been diverting focus from getting completed. Thank you, |
Conflicts were limited to the dependency lists in rebar.config and test-service/rebar.config: main bumped certifi (~> 2.17 / 2.17.0) and shotgun (~> 1.3.0 / 1.3.0) next to the telemetry dependency this branch adds. Both are kept. Full suite (377), dialyzer and xref pass on the merged tree.
Conflicts were limited to the dependency lists in rebar.config and test-service/rebar.config: main bumped certifi (~> 2.18 / 2.18.0) next to the telemetry dependency this branch adds. Both are kept. The certifi root filter removal in ldclient_config merged cleanly. Full suite (376), dialyzer and xref pass on the merged tree.
Callers shed identify and custom events once the buffer reached
events_shed_threshold, while feature evaluations were never shed there.
At the default threshold (events_capacity) those events would have been
dropped by the event server anyway, and the gate never bounded the
mailbox: that is the job of the inbox bound, which already treats every
kind of event alike, as the input queue of the Java and .NET SDKs does.
Shedding evaluations at the threshold instead would lose summary counts
the event server keeps while the buffer is full (ARCO 1.4.1.10/1.4.1.11).
Admission is now the single inbox check. Identify and custom events that
arrive at a full buffer reach the event server and are reported as
capacity drops, as on main; a dropped identify again marks its context
as seen. The buffer-depth counter, which only the gate read, is gone, so
the event server no longer writes a shared counter for every event.
Removed: the events_shed_threshold option, the shed_threshold drop
reason and its log line, and the depth and threshold-drop counter slots
(the published entry is now {Ref, InboxCapacity}). The threshold test
is replaced by one asserting that identify and custom events at a full
buffer are dropped by the server as capacity drops; it fails on the
previous commit.
Events the callers shed because the event server's queue was full (or because it was starting or restarting) are now reported as their own telemetry event, [ldclient, events, shed], with the window's count at every flush and at shutdown. They are no longer counted in [ldclient, events, dropped], whose reasons are now capacity and unencodable only. That keeps dropped in the same terms as the dropped count of the other SDKs (events discarded at a full buffer); shed counts inputs that never reached the event server, so their evaluations are missing from the summary. The log line for sheds now says events are produced faster than the event server can process them. events_inbox_capacity is no longer a public option: the inbox bound is events_capacity, as the input queue of the Java, .NET, Go and Python SDKs is. It stays an internal setting, read from the options only so that tests can set it, which makes exposing it later a documentation change. The events_capacity docs now describe the bound. Tests: the five tests that waited for dropped with reason inbox wait for shed instead, and the inbox shed test asserts that no dropped event is emitted for the sheds (it fails on a mutant that reports sheds as both).
No other SDK splits a flush: Java, .NET, Go, Python and Ruby hand the whole buffer to one worker as one payload, and this option's default (events_capacity) already does the same. Lowering it only adds requests, leaves the summary on one of them, and makes flushes defer once batches outnumber free workers. It is no longer a public option (removed from the start_instance options type and the docs). The setting stays internal, tied to events_capacity and read from the options only so that tests can split a flush across workers; exposing it later is a documentation change.
The housekeeping tick is a correctness backstop, not a tuning knob: it clamps the queued-cast counter to the mailbox length and retries worker starts that failed (a crashed worker is replaced as soon as its exit is noticed). A tick costs a mailbox-length read, so there is nothing for an application to trade off, and no other SDK has an equivalent setting. It is no longer a public option (removed from the start_instance options type and the docs). The setting stays internal, fixed at 1000 ms and read from the options only so that tests can shorten or suppress the tick.
Java, .NET, Python and Ruby all fix their event-sending pool at five workers and none exposes the size. With one request per flush and a request timeout equal to the flush interval, a second worker is only needed when an explicit flush or a retry overlaps the next flush; the pool size matters only when the flush interval is shorter than the endpoint's response time, where the remedies are the same as in the other SDKs (a longer flush interval or a larger capacity). It is no longer a public option (removed from the start_instance options type and the option list). The setting stays internal, fixed at 5 with the 1024 cap and the fallback for invalid values, and read from the options only so that tests can change the pool size. The analytics docs now state the fixed pool of five, and the internal comments no longer say that a busy flush is skipped (it is deferred).
Follows the streaming read timeout added in launchdarkly#200, which is a fixed constant because an earlier revision's option was removed in review (the right value depends on the service, not the application). The events request timeout stays at 30 s: a timeout is a temporary failure, retried once, so a stalled endpoint cannot pin a worker. It also bounds the upload, which the zero-linger close cuts off at the deadline; a full flush at the default capacity is in the order of a megabyte (1.2 MB for 10 001 harness events), so only a very slow link comes near it. It is no longer a public option (removed from the start_instance options type and the option list); the analytics docs now describe the fixed timeout. The setting stays internal, read from the options only so that the dispatcher tests can shorten it. With this, the PR adds no public options: the start_instance options type is the same as on main.
After a temporary failure the worker scheduled its retry and told the event server it was done straight away. The server then stopped keeping the batch, so a worker that exited before the retry resolved lost it without any telemetry (the server re-sends a lost batch only while it still holds it), and the next flush could be handed to the same worker, leaving the retry to wait behind that request for up to the request timeout. The worker now reports worker_done only when the batch is resolved: delivered, dropped after its retry, or dropped on a permanent failure. The retry message carries the owner for that. A pending retry holds a pool slot for the one-second delay and the retry itself; concurrency is still capped by the pool size. New test a_worker_lost_before_its_retry_has_its_batch_resent kills the worker while its retry is pending and expects the batch to be sent again with the same payload id; it fails on the previous commit.
Requirements
Related issues
gen_server:cast/2inldclient_event_server:add_event/3#64 and fix: use gen_server:cast/2 in ldclient_event_server:add_event/3 #115 (switchadd_eventto a cast).Describe the solution you've provided
Summary
Under high flag-evaluation throughput the analytics event pipeline in this SDK was a
serialization and overload point for the application:
ldclient_event_server:add_event/3was a synchronousgen_server:callinto a singlecollector process, so callers blocked on analytics.
length/1on every insert, which is O(n).with no request timeout, so one stalled request held up every later flush.
lruprocess, called synchronously for every event.This PR reshapes the pipeline into a non-blocking, bounded, pooled pipeline. It does not
change the public API and adds no public options: the pipeline's internal limits are fixed at
the values the other server-side SDKs use.
Goals / non-goals
event server mailbox under overload; move formatting, encoding and HTTP off the event server
onto a small fixed pool; make drops, sheds and delivery observable.
server-side SDKs do not have; guaranteeing event ordering (LaunchDarkly does not require it).
Architecture
Process tree
flowchart TD LS[ldclient_sup] --> IS[ldclient_instance_sup / tag /] IS --> US[ldclient_update_sup] IS --> ESup["ldclient_event_sup / tag /<br/>one_for_one"] ESup --> WSup["ldclient_event_worker_sup / tag /<br/>simple_one_for_one"] ESup --> ES[ldclient_event_server] WSup --> W1[ldclient_event_process_server] WSup --> W2[ldclient_event_process_server] WSup --> WN["... (5 workers)"]Data flow
flowchart LR App[App: variation / track / identify] -->|"admission check, then cast (never blocks)"| ES[ldclient_event_server] ES -->|"summary, insert / pop"| BUF[("ETS buffer<br/>ordered_set")] ES -->|"one batch per flush, to an idle worker"| POOL[5 reporter workers] POOL -->|"HTTP POST /bulk, 30 s timeout"| LD[LaunchDarkly events API] POOL -.->|"{worker_done, Pid}"| ES ES -.->|"dropped / shed / flush_skipped / pool_size"| TEL[(telemetry)] POOL -.->|"flush / published / send_error"| TELIngestion is non-blocking and bounded
add_event/3is now agen_server:castbehind a caller-side admission check. Each eventserver publishes a shared
countersreference (viapersistent_term) holding the number ofcasts queued in its mailbox. Callers read the counter directly:
events_capacitycasts queued → count the cast and send it;events_capacityor more → the event is shed at the caller, whatever its kind: only acounter is incremented, and the count is reported at the next flush as
[ldclient, events, shed].The bound is
events_capacity, as the input queue of the Java, .NET, Go and Python SDKs is,so the event server's mailbox and memory stay bounded under ingress overload. A one-second
housekeeping tick clamps the counter to the real mailbox length, which corrects drift from a
caller that died between counting a cast and sending it.
A full buffer does not shed anything at the caller. The reference implementation updates
the summary event for every evaluation, even when full-fidelity events are dropped at
capacity, and the SDK contributor guide requires summary events to describe all evaluations in
the interval. Below the inbox bound every event therefore reaches the event server, which
updates the summary and only drops the full-fidelity events once
events_capacityis reached.Evaluations are lost from the summary only when the event server falls a full inbox behind,
and then every kind of event is shed alike, as with the input queue of the other SDKs.
Offline instances and instances with
send_events => falsedrop all events at the casthandler, so nothing is buffered or sent.
If the event server crashes, its counters stay published with the gate closed, so callers shed
instead of queueing into a dead mailbox until the restarted server reopens it. The new
incarnation reuses the same counters, so the sheds counted meanwhile are reported by its first
flush.
Buffer and seen-context cache
Pending full-fidelity events live in an ETS
ordered_set(ldclient_event_buffer) ratherthan a process-memory list, decoupling storage from the owning process heap:
pop_batch/2atomicallyets:takes the entries of a flush;Summary events are still aggregated in the single event server process, so the summary
counters need no locking.
The seen-context cache that decides when to emit an index event is now two generations of ETS
sets owned by the event server. The check-and-insert is a single atomic
ets:insert_new/2, soit no longer needs a call into another process, and the
lrudependency is removed.Pooled egress
ldclient_event_process_serverworkers runs underldclient_event_worker_sup, the pool size every other server-side SDK uses.ldclient_event_server:flush/1) the server reportsthe drops and sheds since the last flush, snapshots the summary and hands the buffered events
to an idle worker as one batch: one request per flush, as before this PR.
formatting, encoding or HTTP.
[ldclient, events, flush_skipped]) and runsas soon as a worker reports
{worker_done, Pid}. Its events stay in the buffer meanwhile, theone waiting payload of the Go and Java SDKs.
flush/1returns once the events buffered when it started have been handed to workers, notwhen the HTTP requests complete. Events that arrive after the flush started belong to the next
one, so sustained evaluations cannot extend a flush. The flush timer is re-armed when a flush
starts, so a slow endpoint does not delay later flushes.
more with the same payload id.
HTTP delivery
worker), each reused only when idle. On the default profile, workers posting at once queued
behind each other on shared connections. Network options an application set on the default
profile, such as a proxy, are inherited.
zero linger, so a connection whose peer stopped reading is reset and released at the timeout
instead of holding the unsent body. A request still uploading at that point is cut off too.
A full payload at the default capacity is in the order of a megabyte, so only a very slow link
comes near the limit.
(reason
unencodable) instead of crashing the worker and losing the batch.Egress failure handling
The classification of responses is unchanged:
400,408,429,5xx) → the batch is retriedexactly once after 1 s with the same payload id, as before, and
[ldclient, events, send_error]is emitted withtype => temporary.send_erroris emitted withtype => permanent.[ldclient, events, flush]with
outcome => failed.Configuration
No new public options. The
start_instance/3options type is the same as onmain. Thepipeline's internal limits are fixed:
events_capacityevents_capacity(one request per flush)The SDK reads these limits from the options internally, only so tests can change them, so
exposing one later is a documentation change.
Changes to existing options:
events_capacitynow also bounds the number of events waiting for the event server.events_capacity,events_flush_intervalandcontext_keys_capacityare validated. Aninvalid value logs a warning and falls back to the default instead of being used as is
(invalid pipeline sizes used to produce infinite loops).
ldclient:start_instance/3.Telemetry and metrics
The SDK now depends on
telemetry(~> 1.0;pure Erlang, depending only on kernel and stdlib). Every event's metadata includes the instance
tag.[ldclient, events, flush]count,size,durationoutcome(succeeded/failed),status_code[ldclient, events, dropped]countreason(capacity/unencodable)capacity); by the worker while encoding (unencodable)[ldclient, events, shed]count[ldclient, events, published]count[ldclient, events, send_error]count(1)type(temporary/permanent/worker_exit),status_code[ldclient, events, pool_size]workersdirection(initial/up/down)[ldclient, events, flush_skipped]count(1)[ldclient, evaluation, stop]durationflag_key,variationNotes:
droppedcounts events discarded inside the pipeline;shedcounts events the callersdiscarded before they reached the event server. Shed events are not included in
dropped, which keepsdroppedin the same terms as the dropped count of the other SDKs.evaluation path only increments a counter. The per-event drop warning is replaced by one log
line per reason and window.
flushanddroppedcarry what the LaunchDarkly OpenTelemetry metrics specification derivesfrom its
eventFlushCompletedhook; the mapping is documented onstart_instance/3.telemetryruns handlers synchronously in the emitting process, so handlers should be fast:the evaluation event runs in the application's own process.
Backwards compatibility
add_event/3andflush/1keep their arities and contracts, andadd_event/3no longer blocks.full-fidelity events are dropped at capacity, unless the event server falls a full inbox
behind.
send_events => falsestill suppress all event delivery.events_capacityevents are waiting for theevent server;
start_instancecan also return{error, already_started, restarting}while the instanceis being restarted, and
{error, start_failed, Reason}when no reporter worker can start, inwhich case nothing of the instance is left behind.
telemetryis added andlruremoved; the contract-test service's dependencylist is updated so the test release still assembles.
Testing
rebar3 ct: all 376 tests pass.rebar3 dialyzeris clean, andrebar3 xrefreports no newwarnings.
New
test/ldclient_event_server_SUITE.erl(37 tests) covers the non-blocking cast; sheddingat the inbox bound and the
shedevent; capacity drops, including identify and custom eventsat a full buffer, with the summary preserved; one payload per flush; fan-out across workers
and deferred flushes; flush windows; retrying once with the same payload id; worker crashes;
event server crashes, with the counters reused and the sheds reported; failed starts that leave
nothing behind; telemetry contents; and invalid options.
test/ldclient_event_dispatch_httpc_SUITE.erladds the per-instance profile, parallelrequests, inherited socket options, status codes on error responses, the request timeout, and
stalled TCP and TLS peers being released at the timeout.
New
test/ldclient_context_cache_SUITE.erlcovers the two-generation cache, andtest/ldclient_evaluation_telemetry_SUITE.erlthe evaluation event.A load test against a simulated events endpoint (8-core ARM64, OTP 25, 16 producers) was run
on an earlier head of this PR (
811c859), before the internal limits were made non-public:mainDescribe alternatives you've considered
add_eventto a cast only (as in Proposal: usegen_server:cast/2inldclient_event_server:add_event/3#64/fix: use gen_server:cast/2 in ldclient_event_server:add_event/3 #115): rejected because it simply moves thebottleneck into an unbounded event-server mailbox.
JSON encoding and HTTP would still serialise on one process.
five, the size the other server-side SDKs use. In load tests the two only differed when the
flush interval was shorter than the endpoint's response time, and autoscaling needed several
tuning options of its own.
housekeeping interval (also in earlier revisions): no other server-side SDK exposes them,
and the streaming read timeout added in feat: add a read timeout to the streaming data source #200 set the same precedent, so they are fixed
internally.
would under-count summary analytics; the caller sheds only when the event server's queue is
full, and then every kind of event alike.
gen_stage/flow: rejected as it would introduce the Elixir runtime into a pureErlang package.
(as in the previous SDK); a cross-process breaker could be layered on later if needed.
Additional context
described by the maintainer in Performance/scalability improvements proposal #190; no attempt-count or interval behaviour is changed for the
success path.
synchronous
gen_server:callto the feature store (ldclient_storage_ets_server:get), whichis a separate, pre-existing serialization point on the evaluation path.
SDK-3264
Note
Overview
Reworks the analytics event path so evaluations no longer block on a single collector/flusher:
add_event/3becomes a cast with caller-side admission (shed when the event-server mailbox hitsevents_capacity), events sit in an ETS FIFO buffer with O(1) capacity tracking, and five reporter workers format, encode, and POST batches while the event server schedules flushes and deferred windows when all workers are busy.Replaces the per-tag
lruseen-context process with a two-generation ETS cache inside the event server, addsldclient_event_buffer,ldclient_event_worker_sup, and dropslruin favor oftelemetry. HTTP delivery uses a per-instance httpc profile (parallel connections), 30s request timeouts, zero linger for stalled peers, optional dispatcherstop/1, HTTP status codes on errors, and safer JSON encoding (drop unencodable events instead of losing the batch).ldclient:variation*now emits[ldclient, evaluation, stop]; the pipeline emits flush/drop/shed/publish/send_error/pool_size events documented onstart_instance/3. Config gains internal/test-only pipeline knobs and validates numeric options.start_instancecan returnstart_failedorrestarting; instance start rolls back if no worker can start.Reviewed by Cursor Bugbot for commit 06ee9b5. Bugbot is set up for automated code reviews on this repo. Configure here.