Repository navigation
deps: update dependency flagsmith/sdk-spec to v0.7.0 - #64
renovate[bot] wants to merge 2 commits into
Conversation
khvn26
changed the title
renovate
Bot
changed the title
f9e4c74 to
1853c7f
Compare
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Spec v0.6.0 adds the
Tests: added One existing test, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
267d49f8-a62e-4ca3-b57f-cbd2fe5887ea
-
.github/workflows/sdk-spec.ymlHEXDOCS.mdlib/configuration/flagsmith_configuration.exlib/flagsmith_client.exlib/flagsmith_client_events_processor.exlib/flagsmith_supervisor.exlib/schemas/experiment.exlib/schemas/features/feature_state.exlib/schemas/features/feature_state_metadata.exlib/schemas/flag.exlib/supervisors/events_processor_supervisor.exsdk-spec/steps.exstest/flagsmith_configuration_test.exstest/flagsmith_events_test.exs
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
-
| def flush(environment_key) do | ||
| case whereis(environment_key) do | ||
| pid when is_pid(pid) -> GenServer.call(pid, :flush) | ||
| :undefined -> :noop | ||
| end |
There was a problem hiding this comment.
flush/1 can exit the caller when the HTTP request is slow.
GenServer.call(pid, :flush) uses the default 5000 ms timeout. The processor runs the HTTP request inside this call. request_timeout_milliseconds is at least 5000 and retries can multiply the wait. A slow or failing events API can therefore make the caller exit with :timeout. The processor also keeps the late reply.
Use a longer call timeout, or :infinity, because the HTTP client already bounds the request.
Proposed fix
-
- pid when is_pid(pid) -> GenServer.call(pid, :flush) + pid when is_pid(pid) -> GenServer.call(pid, :flush, :infinity)
Committable suggestion
-
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Suggested changedef flush(environment_key) do case whereis(environment_key) do pid when is_pid(pid) -> GenServer.call(pid, :flush) :undefined -> :noop end def flush(environment_key) do case whereis(environment_key) do pid when is_pid(pid) -> GenServer.call(pid, :flush, :infinity) :undefined -> :noop end
| case length(data.buffer) >= @max_buffered_events do | ||
| true -> elem(do_flush(data), 1) | ||
| false -> data | ||
| end |
There was a problem hiding this comment.
A full buffer triggers a blocking HTTP request for every new event while the events API is failing.
buffer/2 calls do_flush/1 whenever length(data.buffer) >= @max_buffered_events. On failure, do_flush/1 keeps Enum.take(buffer, 1000), so the buffer stays at 1000 events. The next {:track, event} cast makes it 1001, which is still >= 1000, so the processor sends another batch. The result is as follows:
- Each tracked event causes one synchronous POST to the failing endpoint inside the GenServer. The POST can block up to
request_timeout_milliseconds(and longer withretries). - The mailbox grows without bound while the calls block.
- Calls to
flush/1time out.
The retry design says failed batches are retried on the next flush. It does not say that every cast must retry.
Flush from buffer/2 only when the size reaches the limit. When the buffer is already full after a failure, drop the oldest event and wait for the scheduled flush. length/1 is also O(n) on every event, so keep a size counter in the state.
Proposed fix
-
defstruct [ :configuration, buffer: [], + buffer_size: 0, exposure_keys: MapSet.new() ]defp buffer(%__MODULE__{buffer: buffer, buffer_size: size} = data, event) do - data = %{data | buffer: [event | buffer]} - - case length(data.buffer) >= @max_buffered_events do - true -> elem(do_flush(data), 1) - false -> data - end + case size + 1 do + n when n < @max_buffered_events -> + %{data | buffer: [event | buffer], buffer_size: n} + + @max_buffered_events -> + elem(do_flush(%{data | buffer: [event | buffer], buffer_size: @max_buffered_events}), 1) + + _ -> + # A previous flush failed and the buffer is full: drop the oldest event + # and wait for the scheduled flush instead of retrying on every event. + %{data | buffer: Enum.take([event | buffer], @max_buffered_events)} + end endAlso set
buffer_size: 0in the success branch ofdo_flush/1. Keepbuffer_sizeequal to@max_buffered_eventsafter a failed flush.
| test "events Flagsmith fails to receive are sent again on the next flush", %{ | ||
| config: config, | ||
| receive_events: receive_events | ||
| } do | ||
| # Given | ||
| expect(Tesla.Adapter.Mock, :call, fn _tesla_env, _options -> | ||
| {:ok, %Tesla.Env{status: 503, body: "unavailable"}} | ||
| end) | ||
|
|
||
| expect(Tesla.Adapter.Mock, :call, receive_events) | ||
| :ok = Client.track_event(config, "checkout_completed", identifier: "user_123") | ||
|
|
||
| # When | ||
| log = | ||
| capture_log(fn -> | ||
| assert {:error, "unavailable"} = Processor.flush(config.environment_key) | ||
| end) | ||
|
|
||
| :ok = Client.track_event(config, "signed_out", identifier: "user_123") | ||
| :ok = Processor.flush(config.environment_key) | ||
|
|
||
| # Then | ||
| assert log =~ "Failed to send events to Flagsmith, keeping them to retry" | ||
|
|
||
| assert_receive {:events_request, _tesla_env, %{"events" => events}} | ||
| assert ["checkout_completed", "signed_out"] = Enum.map(events, & &1["event"]) | ||
| end |
There was a problem hiding this comment.
Add a test for retrying at full buffer capacity.
The failure test covers a small buffer only. It does not cover the case where the buffer is full and the events API keeps failing. Add a test that tracks more than 1000 events while the adapter returns 503. The test must assert that the adapter does not receive a request for every extra event. It would have caught the retry-per-event behaviour in Flagsmith.Client.Events.Processor.buffer/2.
8541003 to
ed91bc8
Compare
renovate
Bot
changed the title
Implements SDK spec v0.6.0's `@events` capability: - `Flagsmith.Client.get_experiment_flag/4` gets an identity's flag and, when the identity is enrolled in a running experiment on an enabled feature, sends a `$flag_exposure` event valued with the variant served. - `Flagsmith.Client.track_event/3` and `track_exposure_event/3` send events to Flagsmith's events API. - Events are buffered per environment key by a supervised processor and sent in batches on an interval, when the buffer fills, and on shutdown; failed batches are kept and retried. - Flags from the identities API now carry their `variant` and `experiment` metadata. - New options: `enable_events`, `events_api_url` and `events_flush_interval_milliseconds`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 8541003)
Edited/Blocked NotificationRenovate will not automatically rebase this PR, because it does not recognize the last commit author and assumes somebody else may have edited the PR. You can manually request rebase by checking the rebase/retry box above. |
This PR contains the following updates:
v0.3.0→v0.7.0v0.3.0→v0.7.0Release Notes
Flagsmith/sdk-spec
v0.7.0Compare Source
What's Changed
New Contributors
Full Changelog: Flagsmith/sdk-spec@v0.6.0...v0.7.0
v0.6.0Compare Source
What's Changed
New Contributors
Full Changelog: Flagsmith/sdk-spec@v0.5.1...v0.6.0
v0.5.1Compare Source
What's Changed
Full Changelog: Flagsmith/sdk-spec@v0.5.0...v0.5.1
v0.5.0Compare Source
What's Changed
Full Changelog: Flagsmith/sdk-spec@v0.4.0...v0.5.0
v0.4.0Compare Source
What's Changed
New Contributors
Full Changelog: Flagsmith/sdk-spec@v0.3.0...v0.4.0
Configuration
📅 Schedule: (UTC)
🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.
♻ Rebasing: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.
🔕 Ignore: Close this PR and you won't be reminded about these updates again.
This PR was generated by Mend Renovate. View the repository job log.