refactor(core): move PrecomputedConfiguration into core #110

rasendubi · 2024-12-05T10:17:55Z

@leoromanovsky this is opened to merge into #81

rasendubi · 2024-12-05T10:19:54Z

eppo_core/src/eval/eval_precomputed_assignments.rs

+#[serde(rename_all = "camelCase")]
+pub struct PrecomputedAssignment {
+    variation_type: VariationType,
+    variation_value: ValueWire,


One diff here is that JSON assignment are now encoded as strings — following UFC flags config approach

rasendubi · 2024-12-05T10:20:43Z

eppo_core/src/eval/eval_precomputed_assignments.rs

+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    allocation_key: Option<Str>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    variation_key: Option<Str>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    extra_logging: Option<HashMap<String, String>>,


Decided to skip serializing these if do_log is false for now. There is no reason for the client to need these except the logging purposes

rasendubi · 2024-12-05T10:21:36Z

eppo_core/src/eval/eval_precomputed_assignments.rs

+    /// `format` is always `AssignmentFormat::Precomputed`.
+    format: AssignmentFormat,
+    // Environment might be missing if configuration was absent during evaluation.
+    environment: Option<Environment>,


There's no good default environment value, so decided to null it instead of sending random string.

rasendubi · 2024-12-05T10:22:59Z

eppo_core/src/ufc/assignment.rs

@@ -29,10 +31,38 @@ pub enum AssignmentValue {
    /// A boolean value.
    Boolean(bool),
    /// Arbitrary JSON value.
-    Json(Arc<serde_json::Value>),
+    Json {
+        raw: Str,


Saving raw json value here. This will also allow us to return raw string in some SDKs if users want to use their own parser.

rasendubi · 2024-12-05T10:35:13Z

eppo_core/src/eval/eval_precomputed_assignments.rs

-    pub format: AssignmentFormat,
-    pub environment: Environment,
-    pub flags: HashMap<String, Result<Assignment, EvaluationError>>,
+    created_at: DateTime<Utc>,


Another change here is that all fields of PrecomputedConfiguration are private to eppo_core: edge server shouldn't need to tinker with them and clients would have their own parsers.

Basically, the only way to interact with PrecomputedConfiguration is via serialize/deserialize.

This should allow us tweaking the type here without being afraid of breaking the edge server.

Good idea! That's a nice way to signal where it should used, noted.

leoromanovsky

Just a single question on do_log case

leoromanovsky · 2024-12-06T01:55:24Z

eppo_core/src/eval/eval_precomputed_assignments.rs

-    pub format: AssignmentFormat,
-    pub environment: Environment,
-    pub flags: HashMap<String, Result<Assignment, EvaluationError>>,
+    created_at: DateTime<Utc>,


Good idea! That's a nice way to signal where it should used, noted.

leoromanovsky · 2024-12-06T01:57:20Z

eppo_core/src/eval/eval_precomputed_assignments.rs

+            None => PrecomputedAssignment {
+                variation_type: assignment.value.variation_type(),
+                variation_value: assignment.value.variation_value(),
+                do_log: true,
+                allocation_key: None,
+                variation_key: None,
+                extra_logging: None,


good idea! thanks for figuring out how to handle these. If I'm understanding you correctly this will enable the getAssignment to return a variation but it won't be logged.

Does it make sense here for do_log: false ?

Suggested change

None => PrecomputedAssignment {

variation_type: assignment.value.variation_type(),

variation_value: assignment.value.variation_value(),

do_log: true,

allocation_key: None,

variation_key: None,

extra_logging: None,

None => PrecomputedAssignment {

variation_type: assignment.value.variation_type(),

variation_value: assignment.value.variation_value(),

do_log: false,

allocation_key: None,

variation_key: None,

extra_logging: None,

cc @sameerank looking at the implementation of (https://github.com/Eppo-exp/js-sdk-common/blob/main/src/client/eppo-precomputed-client.ts#L160-L185)

I see that this will return the variation value but I think logging is not desirable.

The background here is that during UFC compilation (an optimization step), we remove any allocations that are configured with do_log = false and these evaluation to the default value. However for this API we need to return the variation value so that precompute client users can continue to use getAssignment but without getting a log. Am I thinking about that correctly or is the code correct as-is?

Yup, doLog in the js client sdk is used to control if the assignment is logged, and if there's no allocation match, we shouldn't invoke the logger callback (if that's how the do_log check is also getting used in eppo_core)

Good catch — that was a typo. do_log should be false here. UPD: fixed

This is one of the reason why I love lifting data dependencies to type level. Instead of having a do_log and a bunch of other fields that depend on it, the compiled configuration has a nullable event field — no way to make the typo or for dependent fields to be missing

leoromanovsky · 2024-12-06T02:00:24Z

eppo_core/src/eval/evaluator.rs

    ) -> PrecomputedConfiguration {
        let configuration = self.get_configuration();
        get_precomputed_assignments(
            configuration.as_ref().map(AsRef::as_ref),
            &subject_key,
            &subject_attributes,
-            early_exit,


👍 ok your philosophy is not to implement something without a use, I can understand that

Wouldn't say it's my "philosophy" but in this case I'm pretty skeptical that we ever see a use case for early exit (given we spec'ed precomputed format and that it shouldn't early exit).

leoromanovsky · 2024-12-06T02:02:19Z

eppo_core/src/ufc/assignment.rs

+    },
+}
+
+impl PartialEq for AssignmentValue {


haha, PartialEq was auto-generated before but with adding raw json strings, we can't compare on them (because serialization may differ), so I had to roll my own implementation — that's rather un-cool 😅

#81) * refactor: split precomputed assignments between eppo-core and fastly-edge-assignments server. * reset * rename method * part of evaluator * EvaluationError * simplify no error * eppo_core 5.1.0 * compile and simplify * fix spec * refactor(core): move PrecomputedConfiguration into core (#110) * Revert "eppo_core 5.1.0" This reverts commit 9621c51. --------- Co-authored-by: Oleksii Shmalko <[email protected]>

rasendubi requested a review from leoromanovsky December 5, 2024 10:17

rasendubi commented Dec 5, 2024

View reviewed changes

rasendubi mentioned this pull request Dec 5, 2024

chore(ci): fix rust publish workflow #109

Merged

rasendubi force-pushed the oleksii/refactor-precomputed-configuration branch from 6bac46c to 1041efc Compare December 5, 2024 10:31

rasendubi commented Dec 5, 2024

View reviewed changes

leoromanovsky reviewed Dec 6, 2024

View reviewed changes

refactor(core): move PrecomputedConfiguration into core

f5f95a2

rasendubi force-pushed the oleksii/refactor-precomputed-configuration branch from 1041efc to f5f95a2 Compare December 6, 2024 11:13

leoromanovsky approved these changes Dec 6, 2024

View reviewed changes

leoromanovsky merged commit f4ecb00 into lr/refactor-precomputed-assignments Dec 6, 2024
26 checks passed

leoromanovsky deleted the oleksii/refactor-precomputed-configuration branch December 6, 2024 15:22

rasendubi mentioned this pull request Dec 6, 2024

FF-3674 feat: obfuscate precomputed configuration #113

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): move PrecomputedConfiguration into core #110

refactor(core): move PrecomputedConfiguration into core #110

rasendubi commented Dec 5, 2024 •

edited

Loading

rasendubi Dec 5, 2024

rasendubi Dec 5, 2024

rasendubi Dec 5, 2024

rasendubi Dec 5, 2024

rasendubi Dec 5, 2024 •

edited

Loading

leoromanovsky Dec 6, 2024

leoromanovsky left a comment

leoromanovsky Dec 6, 2024

leoromanovsky Dec 6, 2024

leoromanovsky Dec 6, 2024 •

edited

Loading

sameerank Dec 6, 2024

rasendubi Dec 6, 2024 •

edited

Loading

leoromanovsky Dec 6, 2024

rasendubi Dec 6, 2024

leoromanovsky Dec 6, 2024

rasendubi Dec 6, 2024

refactor(core): move PrecomputedConfiguration into core #110

refactor(core): move PrecomputedConfiguration into core #110

Conversation

rasendubi commented Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rasendubi Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoromanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoromanovsky Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rasendubi Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rasendubi commented Dec 5, 2024 •

edited

Loading

rasendubi Dec 5, 2024 •

edited

Loading

leoromanovsky Dec 6, 2024 •

edited

Loading

rasendubi Dec 6, 2024 •

edited

Loading