You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
deps: update dependency flagsmith/sdk-spec to v0.7.0 - #203
To trigger a single review, invoke the @coderabbitai review command.
Run configuration
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: d316c03f-4ab6-47e1-8cc8-0908f4fc56b7
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
🔍 Trigger review
Walkthrough
The client adds configurable event buffering and delivery, event tracking APIs, and experiment flag retrieval with exposure tracking. Event payloads include SDK metadata and filtered traits. The processor flushes on its configured interval or buffer limit and retries specified server and connection failures once. Tests cover event and exposure payloads, validation, buffering, retries, and experiment conditions. SDK spec steps add support for event tracking and experiment retrieval, and the reusable workflow reference changes from v0.3.0 to v0.6.0.
Events may be lost even when an application calls FlushEvents before exiting. Fix delivery completion and the other supported event-flushing failures before merging.
- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Spec v0.6.0 adds the @events capability: experiment flags, tracking events and exposure events. The spec failed on all five @events scenarios because the SDK had no event tracking, so the steps file could not implement Create the SDK client with events enabled. 78121ba adds event tracking to the SDK. It is opt-in, so nothing changes for existing users.
Changes
EnableEvents turns on event tracking. EventsApiUri (default https://events.api.flagsmith.com/), EventsFlushInterval (10 s) and EventsMaxBufferItems (1000) tune it. Events cannot be enabled in offline mode.
GetExperimentFlag(feature, identifier, traits) returns the identity's flag. It records a $flag_exposure for the served variant only when the flag is enabled and the identity is enrolled in a running experiment. A default flag, a disabled flag or no enrolment records nothing.
TrackEvent records application events such as conversions. Names starting with $ are reserved and rejected.
TrackExposureEvent records exposures. It needs an identifier.
FlushEvents sends buffered events at once, for example before the application exits.
Events are sent in batches to <events API>/v1/events, on the flush interval or when the buffer fills. The payload and headers match the Python, Node and Java SDKs. Equal exposures are sent once per batch. A batch is retried once after a 5xx or connection error and then dropped. A 4xx is never retried.
Flag now exposes Variant and Experiment, which come from remote identity evaluation. Local evaluation carries neither, so it records no exposures.
The new methods are also on IFlagsmithClient, so apps that inject the interface can use them. Note for reviewers: this is source-breaking for any third-party class that implements IFlagsmithClient itself (mocks are not affected).
sdk-spec/steps.cs maps the three new When steps and the events client straight to these SDK calls.
Tested with 20 new unit tests in EventsTests (all 68 client tests pass on net10.0, and dotnet format is clean) and with spec v0.6.0 run locally against its compose stack: 7/7 scenarios pass.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🔇 Additional comments: 1
.github/workflows/sdk-spec.yml-14-14: 1
14-14: Trivial · Security & Privacy · Quick win
⚠️Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.
Pin the reusable workflow to a commit SHA.
zizmor reports unpinned-uses for Line 14. The tag v0.6.0 is mutable. If the tag moves, this workflow runs different code without any change in this repository.
Pin the uses: reference to the full commit SHA of v0.6.0. Keep the version in a comment. spec-ref can stay as the tag. Update the "keep in sync" comment to cover the SHA.
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Minor · Functional Correctness · Quick win
Validate flushInterval in the constructor.
The constructor validates maxBufferItems but not flushInterval. At Line 51 the value is passed to new Timer(..., flushInterval, flushInterval).
If EventsFlushInterval is TimeSpan.Zero, the timer fires once and then stops periodic callbacks. Timed flushes then stop without any error or log. Events only leave the buffer when it fills or when the caller calls FlushEvents. A negative value other than -1 ms makes the Timer constructor throw an unclear ArgumentOutOfRangeException.
Reject non-positive intervals early, in the same way as maxBufferItems.
Proposed fix
--- "a/Flagsmith.FlagsmithClient/EventProcessor.cs"+++ "b/Flagsmith.FlagsmithClient/EventProcessor.cs"@@ -37,10 +37,14 @@
public EventProcessor(HttpClient httpClient, string environmentKey, Uri eventsApiUri, TimeSpan flushInterval, int maxBufferItems, TimeSpan requestTimeout, ILogger? logger)
{
if (maxBufferItems < 1)
{
throw new ArgumentOutOfRangeException(nameof(maxBufferItems), "The events buffer must hold at least one event.");
}
+ if (flushInterval <= TimeSpan.Zero)+ {+ throw new ArgumentOutOfRangeException(nameof(flushInterval), "The events flush interval must be positive.");+ }
_httpClient = httpClient;
_environmentKey = environmentKey;
Based on learnings: validate numeric configuration values before use so that invalid values fail fast with a clear error.
Committable suggestion
IMPORTANT
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 change
if(maxBufferItems<1)
{
thrownewArgumentOutOfRangeException(nameof(maxBufferItems),"The events buffer must hold at least one event.");
}
if(maxBufferItems<1)
{
thrownewArgumentOutOfRangeException(nameof(maxBufferItems),"The events buffer must hold at least one event.");
}
if(flushInterval<=TimeSpan.Zero)
{
thrownewArgumentOutOfRangeException(nameof(flushInterval),"The events flush interval must be positive.");
The reason will be displayed to describe this comment to others. Learn more.
Fixed in eb67c78. The constructor now throws ArgumentOutOfRangeException for a flushInterval of zero or less, next to the maxBufferItems check. The new test TestEventsFlushIntervalMustBePositive covers it.
The reason will be displayed to describe this comment to others. Learn more.
Minor · Data Integrity & Integration · Quick win
Supported by static analysis
Script executed:
sed -n '50,150p' Flagsmith.FlagsmithClient/EventProcessor.cs
sed -n '185,235p' Flagsmith.FlagsmithClient/FlagsmithClient.cs
sed -n '160,205p' Flagsmith.Client.Test/EventsTests.cs
Repository: Flagsmith/flagsmith-dotnet-client
Length of output: 9149
Script executed:
set -o pipefail
printf'%s\n''--- EventProcessor declaration and constructor ---'
sed -n '1,125p' Flagsmith.FlagsmithClient/EventProcessor.cs
printf'%s\n''--- public FlushEvents and event processor construction ---'
rg -n -F --glob '*.cs' -e '_eventProcessor' -e 'FlushEvents' -e 'EventsMaxBufferItems' Flagsmith.FlagsmithClient Flagsmith.Client.Test ||test"$?" -eq 1
printf'%s\n''--- all FlushEvents references and surrounding tests ---'
rg -n -F --glob '*.cs' -e 'FlushEvents' -e 'TestEvents' Flagsmith.Client.Test Flagsmith.FlagsmithClient ||test"$?" -eq 1
printf'%s\n''--- event test helper and relevant test body ---'
rg -n -F --glob '*.cs' -e 'MockFlagsmith' -e 'PostedJson' -e 'EventsConfiguration' Flagsmith.Client.Test ||test"$?" -eq 1
sed -n '1,120p' Flagsmith.Client.Test/EventsTests.cs
sed -n '120,280p' Flagsmith.Client.Test/EventsTests.cs
printf'%s\n''--- reviewed diff for event implementation ---'
git diff e2c5320bf13abb1ffef48a0d7f64d74c1b49656d 78121ba3d9baa0d168406ffbab882a8c85f86b58 -- Flagsmith.FlagsmithClient/EventProcessor.cs Flagsmith.FlagsmithClient/FlagsmithClient.cs Flagsmith.Client.Test/EventsTests.cs
Repository: Flagsmith/flagsmith-dotnet-client
Length of output: 42213
Make FlushEvents await automatic sends.
When the buffer reaches _maxBufferItems, EventProcessor.Buffer starts _ = Flush() without retaining the task. If that send is still waiting for HttpClient.SendAsync, a later await client.FlushEvents() sees an empty buffer and returns before the POST completes. This can lose events during application shutdown, despite the documented instruction to call FlushEvents before exit so that no events are lost.
Track pending send tasks and await them in Flush, as well as sending the current buffer.
The reason will be displayed to describe this comment to others. Learn more.
Fixed in eb67c78. Flush now registers each batch in _pendingSends while it holds the lock, before the lock is released. It then returns Task.WhenAll over every pending send, including the one it started. The buffer-full path and the timer both go through Flush, so FlushEvents now waits for their POSTs too. The new test TestFlushWaitsForEventsAlreadyBeingSent holds the events response open and asserts that FlushEvents does not complete until the response arrives.
The reason will be displayed to describe this comment to others. Learn more.
Minor · Stability & Availability · Quick win
Handle all exceptions in Send. Fire-and-forget callers drop them silently.
Send catches only HttpRequestException and TaskCanceledException. JsonConvert.SerializeObject at Line 104 sits outside the try. It serialises caller-supplied metadata and trait values. A non-serialisable value, such as a self-referencing object, throws JsonSerializationException at send time, not at TrackEvent time.
Two callers discard the returned task: the timer callback at Line 51 and the buffer-full path at Line 98. In both cases the exception is unobserved. The whole batch (up to EventsMaxBufferItems events) is lost with no log. One bad event can therefore drop unrelated events. An ObjectDisposedException from a disposed shared HttpClient gives the same silent loss.
Catch Exception in Flush or Send. Log it with LogError. Treat it as non-retryable. Alternatively, serialise each event in the Event constructor so the error reaches the TrackEvent caller.
The reason will be displayed to describe this comment to others. Learn more.
Fixed in eb67c78. Send now serialises inside a try. If serialisation fails, it logs the batch with LogError and drops it. Any other exception from SendAsync, such as ObjectDisposedException, is logged and treated as non-retryable. Send therefore never faults, and the fire-and-forget callers cannot lose an exception unobserved. I kept serialisation at send time, not in the Event constructor. Moving it would serialise each event twice or change the batching model. The new test TestEventsThatCannotBeSerialisedAreDropped tracks an event with self-referencing metadata and asserts that FlushEvents completes without throwing.
Implements the `@events` capability of SDK spec v0.6.0, opt-in with
`EnableEvents`:
- `GetExperimentFlag` gets an identity's flag and records a
`$flag_exposure` for the variant it was served, when the identity is
enrolled in a running experiment on the feature.
- `TrackEvent` and `TrackExposureEvent` record application events and
exposures; `FlushEvents` sends them now.
- Events are buffered and sent to the Flagsmith events API in batches,
on a timer and when the buffer fills. Equal exposures are sent once
per batch; a batch is retried once on a server or connection error,
then dropped.
- `Flag` exposes the `Variant` and `Experiment` that remote identity
evaluation returns.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 78121ba)
…e ones
- Reject a non-positive EventsFlushInterval up front.
- FlushEvents now also waits for batches already being sent, e.g. after
the buffer filled, so no events are lost on shutdown.
- Send logs and drops batches it cannot serialise or send for any other
reason, instead of leaving an unobserved exception.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit eb67c78)
Renovate 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.
⚠️Warning: custom changes will be lost.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openhandsFor the OpenHands agent to pick upsdk-specBumps of the Flagsmith SDK behavioural spec
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
khvn26
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.