-
Notifications
You must be signed in to change notification settings - Fork 5
feat: fastly compute service to perform assignments (FF-3552) #69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…l assignments for a given subject against the cached flags (FF-3571)
…mpute all assignments for a given subject against the cached flags (FF-3571)" This reverts commit a159065.
|
||
# Build only the `fastly-edge-assignments` package for WASM | ||
.PHONY: fastly-edge-assignments-build | ||
fastly-edge-assignments-build: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these changes helpful? I got tired of putting the various parameters in the right place.
[local_server.kv_stores] | ||
[[local_server.kv_stores.edge-assignment-kv-store]] | ||
key = "ufc-by-sdk-key-token-hash-V--77TScV5Etm78nIMTSOdiroOh1__NsupwUwsetEVM" | ||
file = "../sdk-test-data/ufc/flags-v1.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this instructs the fastly serve
process to populate the local in-memory KV store with one key, a UFC configuration at a precomputed hash. See the unit test in this repo for the associate SDK key to use.
allocation_key: String, | ||
variation_key: String, | ||
variation_type: String, | ||
variation_value: serde_json::Value, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: inside core we have ValueWire
that represents this better
} | ||
|
||
const KV_STORE_NAME: &str = "edge-assignment-kv-store"; | ||
const SDK_KEY_QUERY_PARAM: &str = "apiKey"; // For legacy reasons this is named `apiKey` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: given this is a completely new service, I think we can go with sdkKey
as there are no legacy clients?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to do this as well - @sameerank used our existing javascript client to access this service to setup a connection for our eppo.cloud QA environment so we can start collect telemetry on real-world use. Eppo-exp/js-sdk-common#137
I have a couple of goals to evolve the network architecture of this service:
- Serve it behind our existing CDN (with no caching) to be able to share the same domain name and SSL certificate
- Since Sameeran developed the new client separately from the old we have a lot of control over the query parameters and should make this change; or even better, pass the auth information in a
Authorization
header.
base64_url::encode(&hasher.finalize()) | ||
} | ||
|
||
pub fn handle_assignments(mut req: Request) -> Result<Response, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: this function has potential to be split into multiple helper functions, which is currently hindered by the fact that error responses are constructed inline. If we have an explicit error type, that would make splitting much easier and the main body more pleasant and straightforward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I see what you mean - ideally we return explicit errors and this handle just is responsible to rendering those into various http codes and messages?
|
||
impl FlagAssignment { | ||
pub fn try_from_assignment(assignment: Assignment) -> Option<Self> { | ||
// WARNING! There is a problem here. The event is only populated for splits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rasendubi @sameerank I believe this is a problem as described here: https://github.com/Eppo-exp/eppo-multiplatform/blob/main/eppo_core/src/ufc/compiled_flag_config.rs#L231
During compilation, an optimization step to reduce the configuration, event
is only populated for splits with do_log == True
.
A possible solution is to produce a configuration that is "not optimized" for use in this function so that we can access do_log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is to make fields from event optional. There's no reason to include keys and extra logging if doLog
is false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think its a problem because we want the precomputed server to return evaluations for all flags and do_log
is true or false depending on if the user wants them log, but they still need to be present. The flag will be present in the response which changes the behavior of the SDK / application instead of now the flag will not be present and the default value will be used. Am I understanding that correctly?
pub(crate) struct UniversalFlagConfigWire { | ||
/// When configuration was last updated. | ||
pub created_at: Timestamp, | ||
pub format: AssignmentFormat, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heads up @sameerank I added a new key to UFC parsing. I see it is being returned now always so I feel it is safe to make it required.
base64_url::encode(&hasher.finalize()) | ||
} | ||
|
||
pub fn handle_assignments(mut req: Request) -> Result<Response, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I see what you mean - ideally we return explicit errors and this handle just is responsible to rendering those into various http codes and messages?
@sameerank I'm going to merge this PR even though we still have some feedback from Oleksii and improvements I pointed out in the comments, to have a milestone to match your JS SDK release; the |
extra_logging: event | ||
.base | ||
.extra_logging | ||
.iter() | ||
.map(|(k, v)| (k.clone(), v.clone())) | ||
.collect(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as:
extra_logging: event.base.extra_logging.clone(),
} | ||
|
||
impl FlagAssignment { | ||
pub fn try_from_assignment(assignment: Assignment) -> Option<Self> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: prefer implementing From
and TryFrom
traits
let subject_assignments = flag_keys | ||
.iter() | ||
.filter_map(|key| { | ||
match evaluator.get_assignment(key, &subject_key, &subject_attributes, None) { | ||
Ok(Some(assignment)) => FlagAssignment::try_from_assignment(assignment) | ||
.map(|flag_assignment| (key.clone(), flag_assignment)), | ||
Ok(None) => None, | ||
Err(e) => { | ||
eprintln!("Failed to evaluate assignment for key {}: {:?}", key, e); | ||
None | ||
} | ||
} | ||
}) | ||
.collect::<HashMap<_, _>>(); | ||
|
||
// Create the response | ||
let assignments_response = PrecomputedAssignmentsServiceResponse::from_configuration( | ||
configuration, | ||
subject_assignments, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
major: computing subject assignments should be part of core. super-weird to have request/response formats in the core but the way to calculate assignment not in the core
// Request | ||
#[derive(Debug, Deserialize)] | ||
pub struct PrecomputedAssignmentsServiceRequestBody { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
major: eppo_core
shall be unconcerned with network request format
pub(crate) struct UniversalFlagConfigWire { | ||
/// When configuration was last updated. | ||
pub created_at: Timestamp, | ||
pub format: AssignmentFormat, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: ideally, this field should be optional because there could exist configurations without it (remember that users may be storing and passing configurations out of band now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Motivation
Eppo is launching a server-based assignment service hosted on the Fastly Compute platform. This is an edge-product that makes
Design details available in the internal doc.
PR goals
POST /assignments
with asubjectKey
andsubjectAttributes
and return assignments for active allocations.PR non-goals
This work does not include integration testing or CICD integration beyond compilation. This functionality will be added before a
1.0.0
release in the coming weeks.Description
GET /health
andPOST /assignments?apiKey
- theapiKey
format is re-used for legacy reasons although the marketing copy isSDK Key
.apiKey
is converted to a hash and used to look up items in the attached KV store. An external process populates the KV store on Flag changes.