feat: add assignment and exposure events tracking options #73
Conversation
|
📝 Documentation updates detected! New suggestion: Document FetchOptions for Ruby Experiment SDK |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for fetch options to control tracking behavior in the Amplitude Experiment SDK. The changes allow clients to specify whether to track assignment and exposure events via HTTP headers when fetching variants.
- Introduces
FetchOptionsclass withtracks_assignmentandtracks_exposureproperties - Updates client methods (
fetch_v2,fetch_async_v2) to accept optionalFetchOptionsparameter - Adds logic to set tracking headers based on fetch options
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/experiment/remote/fetch_options.rb | New class to encapsulate tracking options for fetch requests |
| lib/experiment/remote/client.rb | Updates fetch methods to accept and process fetch options, sets HTTP headers based on options |
| lib/amplitude-experiment.rb | Adds require statement for the new FetchOptions class |
| spec/experiment/remote/client_spec.rb | Adds test coverage for fetch options behavior and fixes async test synchronization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client.fetch_async_v2(test_user, fetch_options) do |user, block_variants| | ||
| expect(user).to equal(test_user) | ||
| expect(block_variants.key?(variant_name)).to be_truthy | ||
| expect(block_variants.fetch(variant_name)).to eq(Variant.new(key: 'on', payload: 'payload')) |
There was a problem hiding this comment.
The expected variant is inconsistent with the mocked response. The response response_with_key is defined as '{\"sdk-ci-test\":{\"key\":\"on\",\"payload\":\"payload\"}}', and fetch_v2 methods return variants with both key and value populated from the response. However, the expected variant at line 223 only sets key and payload, while the test at line 192 correctly expects Variant.new(key: 'on', payload: 'payload', value: 'on'). Line 223 should include value: 'on' to match the actual behavior.
| expect(block_variants.fetch(variant_name)).to eq(Variant.new(key: 'on', payload: 'payload')) | |
| expect(block_variants.fetch(variant_name)).to eq(Variant.new(key: 'on', payload: 'payload', value: 'on')) |
| unless fetch_options.nil? | ||
| unless fetch_options.tracks_assignment.nil? | ||
| headers['X-Amp-Exp-Track'] = fetch_options.tracks_assignment ? 'track' : 'no-track' | ||
| end | ||
| unless fetch_options.tracks_exposure.nil? | ||
| headers['X-Amp-Exp-Exposure-Track'] = fetch_options.tracks_exposure ? 'track' : 'no-track' | ||
| end | ||
| end |
There was a problem hiding this comment.
[nitpick] The nested unless statements create unnecessary complexity. Consider simplifying by using early returns or guard clauses. For example: headers['X-Amp-Exp-Track'] = fetch_options.tracks_assignment ? 'track' : 'no-track' unless fetch_options&.tracks_assignment.nil? and headers['X-Amp-Exp-Exposure-Track'] = fetch_options.tracks_exposure ? 'track' : 'no-track' unless fetch_options&.tracks_exposure.nil?
| unless fetch_options.nil? | |
| unless fetch_options.tracks_assignment.nil? | |
| headers['X-Amp-Exp-Track'] = fetch_options.tracks_assignment ? 'track' : 'no-track' | |
| end | |
| unless fetch_options.tracks_exposure.nil? | |
| headers['X-Amp-Exp-Exposure-Track'] = fetch_options.tracks_exposure ? 'track' : 'no-track' | |
| end | |
| end | |
| headers['X-Amp-Exp-Track'] = fetch_options.tracks_assignment ? 'track' : 'no-track' unless fetch_options&.tracks_assignment.nil? | |
| headers['X-Amp-Exp-Exposure-Track'] = fetch_options.tracks_exposure ? 'track' : 'no-track' unless fetch_options&.tracks_exposure.nil? |
| expect(block_variants.fetch(variant_name)).to eq(expected_variant) | ||
| callback_called = true | ||
| end | ||
| sleep 1 until callback_called |
There was a problem hiding this comment.
The busy-wait loop sleep 1 until callback_called can delay tests unnecessarily. Consider using a smaller sleep interval (e.g., sleep 0.01) or adding a timeout to prevent infinite loops if the callback is never called.
| sleep 1 until callback_called | |
| start_time = Time.now | |
| timeout = 2 # seconds | |
| until callback_called | |
| sleep 0.01 | |
| if Time.now - start_time > timeout | |
| raise "Timeout waiting for async callback" | |
| end | |
| end |
| expect(a_request(:post, server_url).with(headers: { 'X-Amp-Exp-Track' => 'track', 'X-Amp-Exp-Exposure-Track' => 'track' })).to have_been_made.once | ||
| callback_called = true | ||
| end | ||
| sleep 1 until callback_called |
There was a problem hiding this comment.
The busy-wait loop sleep 1 until callback_called can delay tests unnecessarily. Consider using a smaller sleep interval (e.g., sleep 0.01) or adding a timeout to prevent infinite loops if the callback is never called.
| sleep 1 until callback_called | |
| timeout = 5 # seconds | |
| start_time = Time.now | |
| until callback_called | |
| sleep 0.01 | |
| if Time.now - start_time > timeout | |
| raise "Timeout waiting for callback to be called" | |
| end | |
| end |
Summary
Adds tracking options for assignment and exposure events for fetch calls.
Checklist