-
Notifications
You must be signed in to change notification settings - Fork 2
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
[poc] Elixir SDK #208
base: main
Are you sure you want to change the base?
[poc] Elixir SDK #208
Conversation
|
elixir-sdk/test/sdk_core_test.exs
Outdated
test "init with valid config succeeds" do | ||
config = %SdkCore.Config{ | ||
api_key: "test-key", | ||
base_url: "https://api.eppo.cloud", |
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.
- How do you mock a configuration so it is possible to write more tests locally?
We have a mock-server implementation that exposes all configurations from sdk-test-data on different URLs.
You can start it with npm run start-mock-server
and pass different base urls to the client:
http://127.0.0.1:8378/ufc/api
http://127.0.0.1:8378/obfuscated/api
http://127.0.0.1:8378/bandit/api
elixir-sdk/test/sdk_core_test.exs
Outdated
test "init with valid config succeeds" do | ||
config = %SdkCore.Config{ | ||
api_key: "test-key", | ||
base_url: "https://api.eppo.cloud", |
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 got get_string_assignment to work locally in a script (although the targeting rules don’t seem to be working; I got stuck trying to figure out how to debug).
For debugging, I think getting logs working is most important as they'll tell you what's going on under the hood and maybe even what's wrong.
The second tip for debugging is trying evaluation details methods — they give quite a bit of insight on why targeting didn't work (e.g., whether it's missing configuration, rule condition failed, etc. In multiplatform, it even shows individual attribute values that it considered, so if you mess up elixir-rust conversion, it'll probably show it)
For this particular failure, I think base url might be to blame — it's missing trailing /api
here and the default value is just an empty string, so not correct either. So what I think is happening is that it can't fetch configuration and is returning default values.
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.
Good catch and good tip! I did fix the base_url and verified that the config is downloaded successfully (get_string_assignment returns a non-default variation). I think there is a bug with the conversion of attributes but I will implement get_details and try to use that to debug
elixir-sdk/README.md
Outdated
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.
My main question is, given this steel thread, how much more work is there to do? I think there’s quite a lot (I’m guessing somewhere between 5x and 20x) and wanted to understand whether it is feasible for me to keep working on it at random hours and make meaningful progress.
I actually think that you already implemented the hardest part — figuring out how to interact between the languages. After you get the targeting bug resolved (which I hope is easy), what's left is:
- get assignment logger work (shall be quite trivial as well once you pass event to elixir side)
- define the rest of API and write all the boilerplate code (not familiar with elixir but it'll probably take between a day and three if you want to get fancy). Bandits with their "context attributes" may get tricky but maybe not given that Elixir seems loosely typed. (e.g., for Dart, it felt awkward to have generic attributes and context attributes as different types, so I found a way to unify them into a single attribute type)
- packaging and releasing (very much depends on the tooling if that's easy or not)
// Set global instance | ||
let mut instance = CLIENT_INSTANCE | ||
.write() | ||
.map_err(|e| format!("Failed to acquire write lock: {}", e))?; | ||
|
||
if let Some(existing) = instance.take() { | ||
// Shutdown existing client | ||
drop(existing); | ||
} | ||
|
||
*instance = Some(client.clone()); |
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: if you can, I would like to push singleton up as much as possible. Ideally, having a non-singleton EppoClient "class" in Elixir with a thin wrapper to initialize/get instance.
This shall make it easier to support multiple clients in the future. Not sure about Elixir's threading model but that may also help us to get rid of locks (most of core is designed to allow concurrent access from multiple threads). Non-singletons are also kind of easier to reason about
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.
Hmm Elixir is functional and hence the singleton pattern does not make as much sense; hence I figured it would be easiest to completely hide the client object in Elixir. There's something called a GenServer we could try using, but I feel like it'd needlessly complicate things (and you can only have 1 with a given name, so wouldn't support multiple clients easily either).
There's probably a good option available, I just don't know it
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.
Fixed this using a GenServer, actually is pretty nice, hope you like it!
@@ -96,7 +96,7 @@ pub(crate) struct FlagWire { | |||
} | |||
|
|||
/// Type of the variation. | |||
#[derive(Debug, Serialize, Deserialize, Clone, Copy, PartialEq, Eq)] | |||
#[derive(Debug, Serialize, Deserialize, Clone, Copy, PartialEq, Eq, rustler::NifUnitEnum)] |
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 it feels like a bit of a smell to add this but it seemed like the easiest way to make sure we can convert Elixir atoms (such as :string
to a VariationType). Let me know if you want me to avoid adding rustler and this derive in eppo_core
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 think that's ok'ish. We have some derives for python/ruby in core as well. Unfortunately, because of rust impl rules, we can't add these implementation outside of core so I think that's fine for now.
The only thing is that this should depend on a feature flag, so we don't pull any of elixir libraries into other languages SDKs.
#[derive(Debug, Serialize, Deserialize, Clone, Copy, PartialEq, Eq, rustler::NifUnitEnum)] | |
#[derive(Debug, Serialize, Deserialize, Clone, Copy, PartialEq, Eq)] | |
#[cfg_attr(feature = "rustler", derive(rustler::NifUnitEnum))] |
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 couldn't get this to work yet, might need some extra eyes later.
if let Some(event) = event { | ||
let json_value = serde_json::to_value(&event) | ||
.map_err(|e| rustler::Error::Term(Box::new(format!("Failed to serialize event: {:?}", e))))?; | ||
if let serde_json::Value::Object(map) = json_value { | ||
let converted: HashMap<String, String> = map.into_iter() | ||
.map(|(k, v)| (k, v.to_string())) | ||
.collect(); | ||
Ok(converted.encode(env)) | ||
} else { | ||
Err(rustler::Error::Term(Box::new("Event did not serialize to an object".to_string()))) | ||
} |
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 is pretty gross -- I wonder what's best here, perhaps just send the json as a string and then convert to an object in Elixir (losing the performance of Rust)? Or maybe just keep it as a string (because we'll be logging a string anyway), that seems gross too
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.
Have you looked into serde_rustler? (I didn't really but it looks like something that should help converting between rust/elixir given we already have Serialize/Deserialize implementations)
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.
Good call, the repo is archived and hasn't been updated; also doesn't seem to be working with more recent Elixir versions. Sounds like there were some plans to merge this into Rustler but the issue is still open: rusterlium/rustler#200
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.
oh, that sucks. it looks like there's some experimental serde support in rustler already: https://docs.rs/rustler/latest/rustler/serde/index.html
We may write our own encoder from events to elixir terms. Shouldn't be hard — just boring
But I think to json -> from json approach is also viable for the first version.
I think another option is keeping event in a resource arc, so the user can decide whether they want to serialize it to string or query data from it
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.
For now, opted to send raw json to Elixir, and convert using Elixir's JSON library. Helps with nesting too. We can optimize later.
Current status: got get_X_assignment and get_X_assignment_details to work locally. Up next:
|
0989da2
to
df33bef
Compare
elixir-sdk/lib/assignment_logger.ex
Outdated
@@ -0,0 +1,12 @@ | |||
defmodule Eppo.AssignmentLogger do |
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.
Unsure whether to use Eppo.X
or EppoSDK.X
defmodule Eppo.AssignmentLogger do | |
defmodule EppoSDK.AssignmentLogger do |
Curious what other think
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.
In particular:
Avoid namespace conflicts with existing packages. Plug owns the Plug namespace, if you have an authentication package for Plug use the namespace PlugAuth instead of Plug.Auth.
This guidelines holds for all of your modules, so if your package is plug_auth then all of your modules (except for special ones like mix tasks) should start with PlugAuth. (e.g. PlugAuth.Utils, PlugAuth.User). This is important because there can only be one module with a given name running in a BEAM system, and if multiple packages define the same module, then you cannot use both packages because only one version of the module will be used.
So we can not create another Eppo package if we ever wanted to.
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.
Defaulting to call it EppoSdk
for now, seems most consistent but open to other suggestions
elixir-sdk/lib/client.ex
Outdated
@@ -0,0 +1,461 @@ | |||
defmodule Eppo.Client do |
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 module is the main interface for the client
elixir-sdk/lib/server.ex
Outdated
end | ||
|
||
@impl true | ||
def init(%Eppo.Client.Config{} = config) do |
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.
Alternatively, we could require passing a client rather than a config?
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.
Awesome work. I'm not really familiar with Elixir, but on a high level looks good to me.
}); | ||
|
||
Ok(client) | ||
} |
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.
it seems like we have this same init
routine repeated 3 times for each SDK now, might be a good candidate for moving to eppo-core
I didn't approve yet since it seems like you're still iterating on it and it's probably a good idea getting another set of eyes, but no blockers from me |
This implements the basics of an Elixir SDK that can evaluate feature flags.