feat: filter support in experimentation client #35

mahatoankitkumar · 2024-05-07T10:31:05Z

Problem

Filter prefix from experiments.

Solution

Added Filter prefix support in experimentation client.

pratikmishra356 · 2024-05-08T09:43:46Z

crates/experimentation_client/src/lib.rs

+    pub async fn get_satisfied_experiments(
+        &self,
+        context: &Value,
+    ) -> Result<Experiments, String> {


can we use App error here

pratikmishra356 · 2024-05-08T09:44:08Z

crates/experimentation_client/src/lib.rs

+        &self,
+        context: &Value,
+        toss: i8,
+    ) -> Result<Vec<String>, String> {


can we use App error here

You can't use AppError here, this will be integrated in other codebases that may or may not understand the struct AppError

pratikmishra356 · 2024-05-08T09:47:50Z

crates/experimentation_client/src/lib.rs

@@ -24,15 +30,16 @@ pub struct Client {
 // DO NOT let panics show up in library

 impl Client {
-    pub fn new(config: Config) -> Self {
-        Client {
+    pub fn new(config: Config) -> Result<Self, String> {


can you use app error here also

pratikmishra356 · 2024-05-08T10:04:23Z

crates/experimentation_client/src/lib.rs

+mod utils;
+use std::{
+    collections::{HashMap, HashSet},
+    convert::identity,


its not getting used can you remove this

pratikmishra356 · 2024-05-08T10:11:15Z

crates/experimentation_client/src/lib.rs

-            .collect::<Experiments>()
+            .collect::<Experiments>();
+
+        if let Some(prefix) = context.get("prefix") {


can we use different param for prefix, since its not related to the context instead with the filter,
we also might need to change this in cac_client

pratikmishra356 · 2024-05-08T10:15:13Z

crates/experimentation_client/src/lib.rs

+                    format!("Prefix is not a valid string.")
+                })
+                .map_err_to_string()?
+                .split(",")


can we expect prefix as array of strings instead of comma separated string(since this is not query params of any api),

crates/experimentation_client/src/lib.rs

Datron · 2024-05-08T07:47:07Z

crates/experimentation_client/src/lib.rs

@@ -24,15 +30,16 @@ pub struct Client {
 // DO NOT let panics show up in library

 impl Client {
-    pub fn new(config: Config) -> Self {
-        Client {
+    pub fn new(config: Config) -> Result<Self, String> {


Is there a need for this? Will we ever get an error?

Wanted to maintain consistency everywhere. Maybe in future.

Can we not do it then? Lets add it when the error comes

Datron · 2024-05-09T07:43:23Z

crates/experimentation_client/src/lib.rs

+        &self,
+        context: &Value,
+        toss: i8,
+    ) -> Result<Vec<String>, String> {


You can't use AppError here, this will be integrated in other codebases that may or may not understand the struct AppError

crates/experimentation_client/src/interface.rs

crates/experimentation_client/src/lib.rs

Datron

Had some questions

sauraww · 2024-05-14T11:25:16Z

crates/experimentation_client/src/interface.rs

@@ -187,9 +187,11 @@ pub extern "C" fn get_applicable_variant(
    let variants = local.block_on(&Runtime::new().unwrap(), unsafe {


Can we remove this print statement ?
// println!("Fetching variantIds");

It's already removed, you seeing the code on the opposite(removed) side.

This comment is not removed .
This was the comment that was removed :
// println!("variantIds: {:?}", variants);

sauraww · 2024-05-14T11:30:41Z

crates/experimentation_client/src/interface.rs

-    match serde_json::to_string::<Vec<String>>(&variants) {
-        Ok(result) => rstring_to_cstring(result).into_raw(),
+    match variants {
+        Ok(result) => match serde_json::to_string::<Vec<String>>(&result) {


variants_result .map(|result| { serde_json::to_string(&result) .map(|json| rstring_to_cstring(json).into_raw()) .unwrap_or_else(|err| error_block(err.to_string())) }) .unwrap_or_else(|err| error_block(err.to_string()))

sauraww · 2024-05-14T11:31:45Z

crates/experimentation_client/src/interface.rs

    let experiments = match serde_json::to_value(experiments) {
        Ok(value) => value,


let experiments = serde_json::to_value(experiments) .unwrap_or_else(|err| return error_block(err.to_string()));

Can we make this change at all the places ?
Ideally match statements should be used only for the cases where we have more than 2 conditions to match .
For these cases , we can directly leverage map .

crates/experimentation_client/src/lib.rs

Datron · 2024-05-17T06:47:43Z

crates/experimentation_client/src/utils/mod.rs

@@ -0,0 +1 @@
+pub mod core;


You can just put the code of core.rs into this file

mahatoankitkumar requested a review from a team as a code owner May 7, 2024 10:31

pratikmishra356 reviewed May 8, 2024

View reviewed changes

Datron approved these changes May 9, 2024

View reviewed changes

Datron self-requested a review May 9, 2024 07:47

Datron requested changes May 9, 2024

View reviewed changes

crates/experimentation_client/src/lib.rs Show resolved Hide resolved

mahatoankitkumar force-pushed the feat/filter-support-in-exp-client branch 2 times, most recently from ccd0976 to 71a5b45 Compare May 13, 2024 06:39

mahatoankitkumar requested review from pratikmishra356 and Datron May 13, 2024 06:43

mahatoankitkumar force-pushed the feat/filter-support-in-exp-client branch from 71a5b45 to 77ad805 Compare May 13, 2024 08:26

Datron requested changes May 13, 2024

View reviewed changes

mahatoankitkumar force-pushed the feat/filter-support-in-exp-client branch 2 times, most recently from 9605a32 to 21348a3 Compare May 13, 2024 10:06

sauraww reviewed May 14, 2024

View reviewed changes

sauraww requested changes May 14, 2024

View reviewed changes

crates/experimentation_client/src/lib.rs Show resolved Hide resolved

mahatoankitkumar force-pushed the feat/filter-support-in-exp-client branch from 21348a3 to 8003f3a Compare May 15, 2024 14:37

mahatoankitkumar linked an issue May 16, 2024 that may be closed by this pull request

Add prefix filter to experimentation client #61

Closed

Datron reviewed May 17, 2024

View reviewed changes

pratikmishra356 approved these changes May 17, 2024

View reviewed changes

feat: filter support in experimentation client

2c307ec

mahatoankitkumar force-pushed the feat/filter-support-in-exp-client branch from 8003f3a to 2c307ec Compare May 17, 2024 06:51

Datron approved these changes May 17, 2024

View reviewed changes

sauraww approved these changes May 17, 2024

View reviewed changes

mahatoankitkumar merged commit 0c9d070 into main May 17, 2024
4 checks passed

mahatoankitkumar deleted the feat/filter-support-in-exp-client branch May 17, 2024 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: filter support in experimentation client #35

feat: filter support in experimentation client #35

mahatoankitkumar commented May 7, 2024

pratikmishra356 May 8, 2024

pratikmishra356 May 8, 2024

Datron May 9, 2024

pratikmishra356 May 8, 2024

pratikmishra356 May 8, 2024

pratikmishra356 May 8, 2024

pratikmishra356 May 8, 2024

Datron May 8, 2024

mahatoankitkumar May 9, 2024

Datron May 13, 2024

Datron May 9, 2024

Datron left a comment

sauraww May 14, 2024 •

edited

Loading

mahatoankitkumar May 14, 2024

sauraww May 14, 2024

sauraww May 14, 2024

sauraww May 14, 2024

sauraww May 14, 2024

Datron May 17, 2024

		@@ -187,9 +187,11 @@ pub extern "C" fn get_applicable_variant(
		let variants = local.block_on(&Runtime::new().unwrap(), unsafe {

		let experiments = match serde_json::to_value(experiments) {
		Ok(value) => value,

feat: filter support in experimentation client #35

feat: filter support in experimentation client #35

Conversation

mahatoankitkumar commented May 7, 2024

Problem

Solution

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

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

Choose a reason for hiding this comment

Datron left a comment

Choose a reason for hiding this comment

sauraww May 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sauraww May 14, 2024 •

edited

Loading