Repository navigation
feat: experimentation support (experiment flags and events) - #229
Conversation
…kExposureEvent, FlushEvents) Parse variant, reason and experiment metadata from remote flag responses, add an EventProcessor that batches $flag_exposure and custom events to the events API, and expose WithEvents and related options on the client. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Retry only 503 and transport errors, three attempts in total with exponential backoff and jitter. Drop other non-2xx after one attempt. Stop the processor on 401/403 and log once. Re-queue a failed retryable batch at the head of a bounded, drop-oldest buffer and expose a dropped counter on the client. Log rejected entries from a 202 without resending, and clear the exposure dedupe set only after a 2xx. Refs Flagsmith/flagsmith-private#318 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ed rules Retry 408, 429, 502, 503, 504, network errors and timeouts. Backoff base 1 s, doubling, full jitter, 10 s cap. Log the 401/403 stop once at warn. Update doc comments and README to describe the behaviour. Refs Flagsmith/flagsmith-private#318 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeThe experimentation API is well covered by the completed build and pre-commit checks, but two event-delivery defects need fixing: a slow events service bypasses the configured buffer bound, and rejection logging can expose identity PII. Local test execution was unavailable because the Go toolchain is not installed.
🟠 Majors
📝 Walkthrough
🧪 How to verify
Automate: add the stalled-events capacity regression test alongside the existing event processor tests. Product take: Experiment exposure and conversion tracking is a meaningful SDK capability. It needs reliable backpressure and privacy-safe observability before applications can enable it at production traffic. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. A promising experiment, but the queue needs a lab coat before it meets production traffic · reviewed at b1839c5 |
Allow at most one timer or buffer-full send in flight. While one is pending a full buffer drops its oldest event and counts it, so a stalled events API cannot accumulate batches or goroutines. Remove identifiers and trait values from every log line. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeThis adds a substantial experimentation pipeline with good coverage of normal batching and retries, but two failure paths remain unsafe: shutdown can leave an automatic batch running and uncounted after its deadline, and a rejected API response can bypass the documented log-sanitisation guarantee. CI is still running; pre-commit completed successfully.
🟠 Majors
📝 Walkthrough
🧪 How to verify
Product take: This is a major capability for experiment measurement, but inaccurate shutdown accounting and potential identity leakage undermine operational trust in it. 🧭 Assumptions & unverified claims
The event queue has stamina; its shutdown needs a firmer curfew. · reviewed at 1092cb4 |
Apply the request timeout as a shutdown deadline to in-flight automatic batches, dropping and counting anything that does not finish. Log status, body size and a short error message instead of the raw response body. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeTL;DR: This adds the SDK's experimentation surface and a well-tested buffered events pipeline, but concurrent shutdown can leave a caller-started flush running past the advertised cutoff, and a locally unencodable event is retried forever instead of being treated as a bad payload. The existing open privacy concern around logging API-supplied error detail also remains. The recorded checks show pre-commit completed successfully; both build jobs were still running.
🟠 Majors
📝 Walkthrough
🧪 How to verify
Product take: Experiment exposure and conversion tracking is a meaningful capability for server-side users. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. A promising events pipeline; it just needs to stop retrying its own impossible payloads · reviewed at 2eefc0e |
…timeout, drop unencodable events Hold a re-queued batch until the timer or an explicit flush instead of resending it on the next buffer-full trigger. Bound shutdown to a single request timeout. Log only status and body size on non-retryable drops. Format floats in fixed notation. Release dedupe keys for dropped events, validate rejected[] entries, and count tracking after a 401/403 stop. Encode events before posting so an unencodable event is dropped and counted instead of retried and re-queued. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeThe experimentation surface is well covered, but three lifecycle and privacy defects remain: a shutdown-time manual flush can outlive the advertised bound, a concurrent flush can retry after authentication disables events, and a 202 rejection can leak server-provided text to application logs. CI is still running; the completed pre-commit check passed.
🟠 Majors
📝 Walkthrough
🧪 How to verify
Product take: Solid experimentation support, but its event-delivery lifecycle needs to honour the stated stop conditions before it is safe for production analytics data. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. Even background events need to respect a polite goodbye · reviewed at 56894ad |
…licit flushes at shutdown Log rejected events by index only. Check the disabled flag before every attempt so a concurrent flush stops after a 401/403. Run explicit flush POSTs on a context the shutdown deadline cancels. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeThe new event pipeline has broad focused coverage and the completed build and pre-commit checks passed. One concurrency gap remains: events do not fully snapshot caller-owned nested data, so tracking can race with ordinary caller updates.
🟠 Majors
📝 Walkthrough
🧪 How to verify
Product take: Solid experimentation support, but event tracking runs alongside application request paths. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. The event queue is almost ready to mind its own business · reviewed at 73c559d |
Marshal each event when it is tracked so nested trait and metadata values are never shared with the caller after the call returns. An unencodable event is dropped and counted at track time. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeThis adds a useful opt-in experimentation pipeline, but concurrent manual flushes can send the same exposure twice. The completed build and pre-commit checks passed; Go was not available here for a local test run.
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: Solid experimentation support, but duplicate exposure events can distort experiment analysis for callers that flush concurrently. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. One batch should not give every other exposure a fresh passport · reviewed at 9ebe205 |
Cut doc comments to the essentials and keep the behaviour spec in the README only. Merge helper pairs, remove unused fields, and consolidate the events tests into table-driven groups with shared helpers. No behaviour change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeObserved: a successful batch clears the global exposure-deduplication set. Predicted: concurrent flushes could resend the same exposure. CI is still running; the completed pre-commit check passed.
🟠 Majors
📝 Walkthrough
🧪 How to verify
Product take: This is a major capability for experiment measurement, but duplicate exposures would skew experiment analysis under concurrent flushes. 🧭 Assumptions & unverified claimsLocal Go tooling was unavailable, so the test suite was not run in this review. The events queue is nearly ready to leave the lab; it just needs to remember which exposure is already on its way. · reviewed at 9797f90 |
On a 2xx, release only the keys of the events in that batch instead of clearing the whole set, so an exposure still in flight or buffered is not sent twice. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@themis-blindfold review |
⚖️ Themis review: ✅ Ship itTL;DR: The opt-in experimentation path now keeps event payloads bounded, sanitised, and cancellable through shutdown, while preserving the existing flag-evaluation behaviour. The completed pre-commit check passed; the two build jobs were still running in the captured snapshot.
📝 Walkthrough
🧪 How to verify
Product take: Solid SDK parity improvement for experimentation users, with a deliberately opt-in integration and clear delivery semantics. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. Events have learned to leave tidily when the lights go out · reviewed at 8338dcf |
…ree dedupe key Build the events request headers from scratch so custom Flags API headers never reach the events host and cannot override the environment key or user agent. Leave transient traits out of exposures. Key exposure dedupe on a struct instead of a joined string, and treat a whitespace-only identifier as absent. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@themis-blindfold review |
⚖️ Themis review: ✅ Ship itThe experimentation event pipeline, experiment metadata parsing, delivery controls, and shutdown paths are coherently covered. The completed pre-commit check passed; I found no remaining unaddressed defect in the reviewed change.
📝 Walkthrough
🧪 How to verify
Product take: A solid SDK capability for experiment measurement, with clear delivery semantics for server-side users. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. The event queue now behaves like a queue, including when the network develops opinions. · reviewed at 073aec5 |
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
…e SDK header once Return an error from TrackExposureEvent for a blank identifier. Have GetExperimentFlag reject a blank identity or an evaluation context for another environment before making any request. Set Flagsmith-SDK-User-Agent only on the events request. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Adds experimentation support to the Go SDK, at parity with the Python, JS, Flutter and Node ports.
variant,reasonandmetadata.experimentfrom remote flag responses ontoFlag.EventProcessor(WithEvents(ctx)) batching$flag_exposureand custom events to the events API.GetExperimentFlag,TrackEvent,TrackExposureEvent,FlushEvents,DroppedEvents.rejected[]logged and not resent, dedupe cleared only on 2xx.WithEventscontext performs a final bounded flush;FlushEvents(ctx)for short-lived processes.Local evaluation and offline mode carry no experiment metadata and record no exposures.
flagengine/untouched.Not yet done: manual acceptance against a live environment.
🤖 Generated with Claude Code