Skip to content

Commit ff5db22

Browse files
leoromanovskyleoromanovsky
committed
Address code review: fix exposure cache, config lifecycle, and cleanup
- Fix exposure dedup cache to match Java canonical impl: create ExposureCache class with length-prefixed composite keys (no collision), add() returns bool like Java's LRUExposureCache.add(), always promotes LRU position even for duplicates - Add 12 ExposureCache tests matching Java's LRUExposureCacheTest - Add LRUCache.put() (returns old value) and size() methods with tests - Fix Provider to handle config removal via ffe_config_changed(), clear exposure cache when RC removes config - Auto-flush exposures on request shutdown via register_shutdown_function - Reduce ExposureWriter curl timeout to 500ms/100ms (was 5s/2s) - Combine FFE_CONFIG + FFE_CONFIG_CHANGED into single FfeState struct behind one Mutex for atomic updates, add RwLock justification comment - Remove unused datadog-ffe-ffi dependency from Cargo.toml - Change LRUCache eviction from while to if (only one entry added) - Clarify continue-in-switch comment in ddtrace.c ffe_evaluate handler
1 parent 77213df commit ff5db22

11 files changed

Lines changed: 461 additions & 37 deletions

File tree

‎components-rs/Cargo.toml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ libdd-crashtracker-ffi = { path = "../libdatadog/libdd-crashtracker-ffi", defaul
2424
libdd-library-config-ffi = { path = "../libdatadog/libdd-library-config-ffi", default-features = false }
2525
spawn_worker = { path = "../libdatadog/spawn_worker" }
2626
datadog-ffe = { path = "../libdatadog/datadog-ffe" }
27-
datadog-ffe-ffi = { path = "../libdatadog/datadog-ffe-ffi", default-features = false }
2827
anyhow = { version = "1.0" }
2928
const-str = "0.5.6"
3029
itertools = "0.11.0"

‎components-rs/ffe.rs‎

Lines changed: 29 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -6,28 +6,38 @@ use std::collections::HashMap;
66
use std::ffi::{c_char, CStr, CString};
77
use std::sync::{Arc, Mutex};
88

9+
/// Holds both the FFE configuration and a "changed" flag atomically behind a
10+
/// single Mutex. This avoids the race where another thread could observe
11+
/// `config` updated but `changed` still false (or vice-versa).
12+
///
13+
/// A `RwLock` would be more appropriate here (many readers via `ddog_ffe_evaluate`,
14+
/// rare writer via `store_config`), but PHP is single-threaded per process so
15+
/// contention is not a practical concern. Keeping a Mutex for simplicity.
16+
struct FfeState {
17+
config: Option<Configuration>,
18+
changed: bool,
19+
}
20+
921
lazy_static::lazy_static! {
10-
static ref FFE_CONFIG: Mutex<Option<Configuration>> = Mutex::new(None);
11-
static ref FFE_CONFIG_CHANGED: Mutex<bool> = Mutex::new(false);
22+
static ref FFE_STATE: Mutex<FfeState> = Mutex::new(FfeState {
23+
config: None,
24+
changed: false,
25+
});
1226
}
1327

1428
/// Called by remote_config when a new FFE configuration arrives via RC.
1529
pub fn store_config(config: Configuration) {
16-
if let Ok(mut guard) = FFE_CONFIG.lock() {
17-
*guard = Some(config);
18-
}
19-
if let Ok(mut changed) = FFE_CONFIG_CHANGED.lock() {
20-
*changed = true;
30+
if let Ok(mut state) = FFE_STATE.lock() {
31+
state.config = Some(config);
32+
state.changed = true;
2133
}
2234
}
2335

2436
/// Called by remote_config when an FFE configuration is removed.
2537
pub fn clear_config() {
26-
if let Ok(mut guard) = FFE_CONFIG.lock() {
27-
*guard = None;
28-
}
29-
if let Ok(mut changed) = FFE_CONFIG_CHANGED.lock() {
30-
*changed = true;
38+
if let Ok(mut state) = FFE_STATE.lock() {
39+
state.config = None;
40+
state.changed = true;
3141
}
3242
}
3343

@@ -54,16 +64,16 @@ pub extern "C" fn ddog_ffe_load_config(json: *const c_char) -> bool {
5464
/// Check if FFE configuration is loaded.
5565
#[no_mangle]
5666
pub extern "C" fn ddog_ffe_has_config() -> bool {
57-
FFE_CONFIG.lock().map(|g| g.is_some()).unwrap_or(false)
67+
FFE_STATE.lock().map(|s| s.config.is_some()).unwrap_or(false)
5868
}
5969

6070
/// Check if FFE config has changed since last check.
6171
/// Resets the changed flag after reading.
6272
#[no_mangle]
6373
pub extern "C" fn ddog_ffe_config_changed() -> bool {
64-
if let Ok(mut changed) = FFE_CONFIG_CHANGED.lock() {
65-
let was_changed = *changed;
66-
*changed = false;
74+
if let Ok(mut state) = FFE_STATE.lock() {
75+
let was_changed = state.changed;
76+
state.changed = false;
6777
was_changed
6878
} else {
6979
false
@@ -168,13 +178,13 @@ pub extern "C" fn ddog_ffe_evaluate(
168178

169179
let context = EvaluationContext::new(tk, Arc::new(attrs));
170180

171-
let guard = match FFE_CONFIG.lock() {
172-
Ok(g) => g,
181+
let state = match FFE_STATE.lock() {
182+
Ok(s) => s,
173183
Err(_) => return std::ptr::null_mut(),
174184
};
175185

176186
let assignment = ffe::get_assignment(
177-
guard.as_ref(),
187+
state.config.as_ref(),
178188
flag_key,
179189
&context,
180190
expected_type,

‎ext/ddtrace.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3064,7 +3064,7 @@ PHP_FUNCTION(dd_trace_internal_fn) {
30643064
c_attrs[idx].bool_value = false;
30653065
break;
30663066
default:
3067-
continue;
3067+
continue; /* skip unsupported types; targets ZEND_HASH_FOREACH loop */
30683068
}
30693069
idx++;
30703070
} ZEND_HASH_FOREACH_END();

‎src/DDTrace/FeatureFlags/ExposureCache.php‎

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
<?php
2+
3+
namespace DDTrace\FeatureFlags;
4+
5+
/**
6+
* LRU-based exposure event deduplication cache.
7+
*
8+
* Mirrors the canonical Java implementation (LRUExposureCache).
9+
* Key = (flagKey, subjectId), Value = (variantKey, allocationKey).
10+
* Returns true from add() when the event is new or its value changed,
11+
* false when it's an exact duplicate. Even duplicates update the LRU
12+
* position to keep hot entries from being evicted.
13+
*/
14+
class ExposureCache
15+
{
16+
/** @var LRUCache */
17+
private $cache;
18+
19+
/**
20+
* @param int $capacity Maximum number of entries
21+
*/
22+
public function __construct($capacity = 65536)
23+
{
24+
$this->cache = new LRUCache($capacity);
25+
}
26+
27+
/**
28+
* Add an exposure event to the cache.
29+
*
30+
* @param string $flagKey
31+
* @param string $subjectId
32+
* @param string $variantKey
33+
* @param string $allocationKey
34+
* @return bool true if the event is new or value changed, false if exact duplicate
35+
*/
36+
public function add($flagKey, $subjectId, $variantKey, $allocationKey)
37+
{
38+
$key = self::makeKey($flagKey, $subjectId);
39+
$newValue = self::makeValue($variantKey, $allocationKey);
40+
41+
// Always put (updates LRU position even for duplicates)
42+
$oldValue = $this->cache->put($key, $newValue);
43+
44+
return $oldValue === null || $oldValue !== $newValue;
45+
}
46+
47+
/**
48+
* Get the cached value for a (flag, subject) pair.
49+
*
50+
* @param string $flagKey
51+
* @param string $subjectId
52+
* @return array|null [variantKey, allocationKey] or null if not found
53+
*/
54+
public function get($flagKey, $subjectId)
55+
{
56+
$key = self::makeKey($flagKey, $subjectId);
57+
$value = $this->cache->get($key);
58+
if ($value === null) {
59+
return null;
60+
}
61+
return self::parseValue($value);
62+
}
63+
64+
/**
65+
* Return the number of entries in the cache.
66+
*
67+
* @return int
68+
*/
69+
public function size()
70+
{
71+
return $this->cache->size();
72+
}
73+
74+
/**
75+
* Clear all entries.
76+
*/
77+
public function clear()
78+
{
79+
$this->cache->clear();
80+
}
81+
82+
/**
83+
* Build a composite key that avoids collision.
84+
* Uses length-prefixing: "<len>:<flag>:<subject>"
85+
*/
86+
private static function makeKey($flagKey, $subjectId)
87+
{
88+
$f = $flagKey !== null ? $flagKey : '';
89+
$s = $subjectId !== null ? $subjectId : '';
90+
return strlen($f) . ':' . $f . ':' . $s;
91+
}
92+
93+
/**
94+
* Build a composite value string.
95+
*/
96+
private static function makeValue($variantKey, $allocationKey)
97+
{
98+
$v = $variantKey !== null ? $variantKey : '';
99+
$a = $allocationKey !== null ? $allocationKey : '';
100+
return strlen($v) . ':' . $v . ':' . $a;
101+
}
102+
103+
/**
104+
* Parse a composite value string back into [variantKey, allocationKey].
105+
*/
106+
private static function parseValue($value)
107+
{
108+
$colonPos = strpos($value, ':');
109+
if ($colonPos === false) {
110+
return [$value, ''];
111+
}
112+
$len = (int) substr($value, 0, $colonPos);
113+
$variant = substr($value, $colonPos + 1, $len);
114+
$allocation = substr($value, $colonPos + 1 + $len + 1);
115+
return [$variant, $allocation];
116+
}
117+
}

‎src/DDTrace/FeatureFlags/ExposureWriter.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,8 +65,8 @@ public function flush()
6565
'X-Datadog-EVP-Subdomain: event-platform-intake',
6666
],
6767
CURLOPT_RETURNTRANSFER => true,
68-
CURLOPT_TIMEOUT => 5,
69-
CURLOPT_CONNECTTIMEOUT => 2,
68+
CURLOPT_TIMEOUT_MS => 500,
69+
CURLOPT_CONNECTTIMEOUT_MS => 100,
7070
]);
7171

7272
curl_exec($ch);

‎src/DDTrace/FeatureFlags/LRUCache.php‎

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,14 +66,54 @@ public function set($key, $value)
6666

6767
$this->cache[$key] = $value;
6868

69-
// Evict least recently used entries if over capacity
70-
while (count($this->cache) > $this->maxSize) {
69+
// Evict least recently used entry if over capacity
70+
if (count($this->cache) > $this->maxSize) {
7171
reset($this->cache);
7272
$evictKey = key($this->cache);
7373
unset($this->cache[$evictKey]);
7474
}
7575
}
7676

77+
/**
78+
* Put a value in the cache and return the previous value.
79+
*
80+
* Like set(), but returns the old value (or null if the key was not present).
81+
* Always updates the LRU position, even when the value is unchanged.
82+
*
83+
* @param string $key
84+
* @param mixed $value
85+
* @return mixed|null The previous value, or null if the key was new
86+
*/
87+
public function put($key, $value)
88+
{
89+
$oldValue = null;
90+
if (array_key_exists($key, $this->cache)) {
91+
$oldValue = $this->cache[$key];
92+
unset($this->cache[$key]);
93+
}
94+
95+
$this->cache[$key] = $value;
96+
97+
// Evict least recently used entry if over capacity
98+
if (count($this->cache) > $this->maxSize) {
99+
reset($this->cache);
100+
$evictKey = key($this->cache);
101+
unset($this->cache[$evictKey]);
102+
}
103+
104+
return $oldValue;
105+
}
106+
107+
/**
108+
* Return the number of entries in the cache.
109+
*
110+
* @return int
111+
*/
112+
public function size()
113+
{
114+
return count($this->cache);
115+
}
116+
77117
/**
78118
* Clear all entries from the cache.
79119
*/

0 commit comments

Comments
 (0)