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 - #112
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: f17260dd-45f5-48a5-bd00-b064c1aa9c63
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 SDK adds configurable event tracking, buffered submission to the events API, and client methods for tracking events and retrieving experiment flags. Flags now expose variant and experiment metadata. Specifications cover event processing, experiment exposure tracking, and SDK configuration. The SDK-spec reusable workflow and spec reference are updated to v0.6.0.
Experiment exposure events are silently skipped for local-evaluation and offline clients, and identity identifiers can be written to stdout by default. Stopped event processors also stay in memory until the process exits. Resolve or explicitly accept these 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 three @events scenarios: getExperimentFlag, trackEvent and trackExposureEvent. The Ruby SDK had no event support, so 3ec4437 implements it in the SDK, following the Python client's behaviour:
Events opt-in: two new config options, enable_events and events_api_url (default https://events.api.flagsmith.com/).
Flagsmith::EventProcessor: buffers events and posts them in batches to <events_api_url>v1/events.
It flushes every 10s, when 1000 events are buffered, or at exit.
Within a flush interval, a repeated exposure is sent only once.
A failed batch is retried once on a 5xx or connection error, then logged and dropped. Sending never raises into the app.
Flag#variant and Flag#experiment: both are read from the identities API response (variant and metadata.experiment).
Client#get_experiment_flag(feature, identifier, **traits): returns the identity's flag. It records a $flag_exposure (value: the variant, metadata: experiment_id) only when the flag is enabled, the identity is in the running experiment, and a variant was served. Otherwise it logs why it skipped the exposure.
Client#track_event and Client#track_exposure_event:track_event rejects names starting with the reserved $ prefix. track_exposure_event skips an exposure with no identifier.
sdk-spec/steps.rb: wires the new steps to these SDK calls.
Unit tests for the processor and the client methods are included. Locally, rubocop is clean and rspec passes on Ruby 3.0 and 3.3 (389 examples). The full v0.6.0 spec passes (7/7) against its compose stack.
CI (3.0) and CI (3.1) will stay red, and the cause is not this change.ubuntu-latest is now ubuntu-26.04, and ruby/setup-ruby has no Ruby 3.0 or 3.1 builds for that image (Unavailable version 3.0.7 for ruby on ubuntu-26.04). Fixing that needs a change to .github/workflows/pull_request.yaml, either pinning the runner image or dropping EOL Rubies from the matrix. Workflow changes are out of scope for this fix.
sdk-spec / Spec v0.6.0 now passes with 3ec4437. The PR is still red, and I can't fix the remaining failures within the rules I'm working under (no CI configuration changes), so I'm stopping here and adding openhands:stuck.
What's still failing:CI RSpec Tests (.github/workflows/pull_request.yaml).
CI (3.0) fails in Set up Ruby: Error: Unavailable version 3.0.7 for ruby on ubuntu-26.04. ubuntu-latest has moved to ubuntu-26.04, and ruby/setup-ruby has no Ruby 3.0 or 3.1 builds for it. The same error hit CI (3.1) on the previous run.
The matrix uses the default fail-fast, so CI (3.1), CI (3.2) and CI (3.3) are cancelled before linting or tests run. They show as failed, but no SDK code ran in them.
This failure was already present on the original Renovate commit and doesn't depend on this PR's code.
Locally, bundle exec rubocop and bundle exec rspec spec both pass on Ruby 3.0.7 and 3.3 (389 examples).
Needed from a maintainer: a workflow change in a separate PR. One option is pinning the job to an image that still has these Rubies (e.g. runs-on: ubuntu-24.04). Another is dropping the EOL Rubies 3.0 and 3.1 from the matrix. The second also means deciding on required_ruby_version in flagsmith.gemspec. Once main has that fix and this branch is updated, CI should go green.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review info
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: ba792d97-2a25-4a76-b38f-f32485490773
Commits
Reviewing files that changed from the base of the PR and between ef6c84e and 3ec4437.
Files selected for processing: 9
.github/workflows/sdk-spec.yml
lib/flagsmith.rb
lib/flagsmith/sdk/config.rb
lib/flagsmith/sdk/event_processor.rb
lib/flagsmith/sdk/models/flags.rb
sdk-spec/steps.rb
spec/sdk/event_processor_spec.rb
spec/sdk/experiments_spec.rb
spec/sdk/realtime_client_spec.rb
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.
Major · Stability & Availability · Quick win
Supported by static analysis
Script executed:
sed -n '1,145p' lib/flagsmith/sdk/event_processor.rb
sed -n '1,200p' spec/sdk/event_processor_spec.rb
Repository: Flagsmith/flagsmith-ruby-client
Length of output: 10494
Limit threshold-triggered flush scheduling to one pending worker.
EventProcessor#buffer can create one thread for every non-duplicate event after the buffer reaches its limit. EventProcessor#flush holds @flush_mutex while sending and retrying the batch. A slow or failing request can therefore leave many threads blocked and allow the buffer to grow well beyond its configured limit. This is a runtime availability risk, not a trivial nitpick.
Use @flush_pending under @mutex. Clear it only when the threshold-triggered worker acquires @flush_mutex and swaps the buffer. Clearing it from any flush can allow another waiter while the scheduled worker is still blocked.
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Fixed in 2526c08 as suggested. buffer now sets @flush_pending under @mutex and starts a flush thread only when no flush is pending. Only that scheduled flush (flush(scheduled: true)) clears the flag, after it holds @flush_mutex and swaps the buffer. So a slow send can leave at most one threshold-triggered worker waiting. I added a spec, "schedules one flush at a time while a batch is being sent", which checks that Thread.new is called once when 4 events go into a 2-item buffer. As you noted, this limits the number of threads. It does not put a hard cap on @events while a send is in progress.
I merged main into this branch (f803880) to pick up #115, which pins the test runner to ubuntu-24.04. That was the cause of the red CI (3.x) jobs: ubuntu-latest (26.04) has no Ruby 3.0 build. The merge also brings in #116, which adds Ruby 3.4 and 4.0 to the matrix. This is a plain merge with no SDK or workflow edits of my own. The sdk-spec / Spec v0.6.0 check already passed on the previous head.
Locally, bundle exec rake (RSpec and RuboCop) passes on Ruby 3.0, 3.4 and 4.0: 390 examples, 0 failures, no offenses.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🔇 Additional comments: 1
lib/flagsmith/sdk/event_processor.rb-123-123: 1
123-123: Minor · Data Integrity & Integration · Quick win
⚠️Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.
Include the experiment ID in the exposure deduplication key.
If the same identity receives the same feature and variant under two experiment IDs before a flush, this key drops the second exposure. track_exposure_event accepts an experiment ID in metadata, so both calls can occur through the public client API. Include event.dig(:metadata, 'experiment_id') in the key. (raw.githubusercontent.com)
Review info
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: 64fc468f-2a79-4922-acc9-ea7bb866f3a4
Commits
Reviewing files that changed from the base of the PR and between 3ec4437 and f803880.
Files selected for processing: 7
lib/flagsmith.rb
lib/flagsmith/sdk/config.rb
lib/flagsmith/sdk/event_processor.rb
lib/flagsmith/sdk/models/flags.rb
sdk-spec/steps.rb
spec/sdk/event_processor_spec.rb
spec/sdk/realtime_client_spec.rb
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
set -u
sed -n '195,240p' lib/flagsmith/sdk/models/flags.rb | nl -ba -v 195
echoecho"== Experiment / variant references in flags.rb =="
rg -n -e 'Experiment' -e 'variant' -- lib/flagsmith/sdk/models/flags.rb ||test"$?" -eq 1
Repository: Flagsmith/flagsmith-ruby-client
Length of output: 1983
Experiment exposure is never recorded when local evaluation or offline mode supplies the flags.
When environment is loaded, get_identity_flags evaluates flags from the local document (lib/flagsmith.rb:188). Offline mode also sets environment (lib/flagsmith.rb:126). That path builds each Flag with Collection.map_evaluated_flag_to_flag_result, which passes only feature_name, enabled, value, feature_id and reason. It does not set variant or experiment (lib/flagsmith/sdk/models/flags.rb:209-216).
As a result, exposure_skip_reason returns 'identity is not in a running experiment' for every such flag, because flag.experiment is nil (lib/flagsmith.rb:284). get_experiment_flag then never calls track_exposure_event. The debug message gives the wrong reason for these modes.
Choose one of two fixes:
Propagate the experiment and variant data from the evaluation result into the Flag objects, so local evaluation and offline mode can record exposure.
Reject these modes in get_experiment_flag, with an explicit error, and document that the method requires the API path.
The shape of the evaluation result's experiment metadata was not inspected, so this comment does not propose a code change.
The reason will be displayed to describe this comment to others. Learn more.
Option 1 is not possible today, because the data does not exist on that path. The environment document behind local evaluation and offline mode has no experiment or variant-key data, so the evaluation result has nothing to propagate. The engine and the engine test data never mention experiments (grep -ri experiment lib/flagsmith/engine spec/engine-test-data finds nothing). The Flagsmith docs say the same in docs/docs/experimentation/run-an-experiment.mdx: "Experiments currently work with remote evaluation only. Support for local evaluation is in progress."
Option 2, raising in these modes, is a product decision, so I have left that choice to you. For comparison, the Python reference SDK (get_experiment_flag in flagsmith-python-client) does not raise. It returns the flag and skips the exposure. The Ruby client does the same today.
You are right that the debug reason was wrong, so I fixed that in f829d07, which does not depend on either option. When flags come from the environment document, the skip reason is now "experiments need remote evaluation, not local evaluation or offline mode". The method doc states the limit. A new spec in spec/sdk/experiments_spec.rb ("when the flags come from the environment document") covers offline mode. If you prefer an explicit Flagsmith::ClientError, it is a one-line change in get_experiment_flag.
The reason will be displayed to describe this comment to others. Learn more.
Major · Stability & Availability · Heavy lift
Do not retain stopped processors through exit callbacks.
Each call to start registers an at_exit block that retains the processor. stop cannot remove that block. If an application creates and stops clients repeatedly, every processor remains in memory until process exit. Use a shutdown registration that does not retain stopped processors. (raw.githubusercontent.com)
The reason will be displayed to describe this comment to others. Learn more.
Agreed, fixed in f829d07. start no longer registers an at_exit block for each processor. A single at_exit hook in a new Flagsmith::RunningEventProcessors registry stops the processors that are running at exit. start adds the processor to the registry and stop removes it, so a stopped processor is no longer kept in memory. This also fixes a second issue: a start/stop/start cycle used to register a second exit hook. New specs in spec/sdk/event_processor_spec.rb check that a started processor is in the registry and a stopped one is not. Separately, Flagsmith::Client has no public way to stop its processor, so clients that use events still keep theirs until exit. That is outside this change.
The reason will be displayed to describe this comment to others. Learn more.
Major · Security & Privacy · Quick win
Remove identity identifiers from duplicate-exposure logs.
If an identity identifier is an email address, a second exposure writes that address to the default debug logger on stdout. Log the duplicate without the identifier, or replace it with a non-reversible diagnostic value. (raw.githubusercontent.com)
The reason will be displayed to describe this comment to others. Learn more.
Agreed, fixed in f829d07. The duplicate-exposure debug message now names only the feature: Skipping duplicate $flag_exposure event for feature banner. I updated the spec to match. The other skip and warn messages in the client log only the feature name, never the identifier.
Implements SDK spec v0.6.0's getExperimentFlag, trackEvent and
trackExposureEvent. With `enable_events: true`, the client buffers events
and sends them in batches to the Flagsmith events API (`events_api_url`).
`get_experiment_flag` records an identity's exposure to the variant it was
served when it is in the feature's running experiment, which flags now
expose as `variant` and `experiment`.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 3ec4437)
A slow or failing events API request held the flush lock while every
further event past the buffer limit started another flush thread.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 2526c08)
- Stop running event processors at exit from one registry that a
stopped processor leaves, instead of an at_exit block per start that
kept every processor in memory.
- Drop the identifier from the duplicate exposure debug log.
- Give the accurate reason when get_experiment_flag skips an exposure
because flags come from the environment document, which carries no
experiment data.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit f829d07)
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.
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.