-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat: yggdrasil engine #152
base: main
Are you sure you want to change the base?
Conversation
This is awesome work! But as early feedback, these are my initial thoughts: should we consider having:
I think the current implementation should co-exist until there is full feature parity and we have enough testing done with the client using rust implementation of the engine. There is also a question on how we would provide custom strategies, etc, but I think that is less relevant to bring up at the moment. Otherwise, I only have kudos to give! This is really awesome and impressive! |
So this:
Is an excellent idea and I'd like to do that. Yggdrasil is surprisingly more battle tested than it appears, since it forms the core of Edge and Edge has been feature complete for 6 months or so (minus custom strategies). The Ruby engine from Yggdrasil is a pretty thin wrapper around that but there's always scope for things to go wrong with such a large change. Medium to long term, the Ruby implementation won't be able to keep up with Yggdrasil unless it implements a full grammar parser (which I don't think is sane) so the pure Ruby implementation days are probably coming to an end in the next few months to years. I'm not convinced about pulling the existing Ruby engine in the SDK code out into a separate gem because of that (and I also don't want to deal with potential issues with refactoring that out on top of this complexity). I had kinda of thought to revert all the deletes and toggle between Yggdrasil and the existing code within the SDK. Once we're happy it works we can cull out the existing implementation. Idle thoughts still Custom strategies are coming! I have a blob of terribly ugly code on my machine that is a drop in replacement for custom strategies here but it needs work before more eyes see it. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Shhh stalebot, shh. |
211fcdf
to
b513382
Compare
…h namespace and remove usage of these throughout the bodebase
@@ -40,9 +40,9 @@ def metrics_interval_in_millis | |||
def validate! | |||
return if self.disable_client | |||
|
|||
raise ArgumentError, "URL and app_name are required parameters." if self.app_name.nil? || self.url.nil? | |||
raise ArgumentError, "app_name is a required parameter." if self.app_name.nil? |
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.
Bootstrap only should be a viable option (we can do this in the others), relaxing this constraint makes testing a ton easier but I may be able to roll this one back if we need
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.
Valid point.
@@ -23,6 +23,22 @@ def to_s | |||
",app_name=#{@app_name},environment=#{@environment}>" | |||
end | |||
|
|||
def as_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.
For Yggdrasil to use so we can send the context as a JSON blob
Pull Request Test Coverage Report for Build 8002626095Details
💛 - Coveralls |
3622c19
to
d85faf8
Compare
d85faf8
to
156356c
Compare
1622da5
to
8e64f84
Compare
f865a47
to
c2fba12
Compare
3c49290
to
9dc382e
Compare
@rarruda Okay, this is coming out of draft. We've spoke a lot internally about keeping a pure Ruby impl around but I think we'd rather go forward and if folks need a pure Ruby impl they can use a previous version until they can upgrade. Coverage is failing but looking at the details, a lot of that is because of the massive LOC reduction on this. There's little but logging that isn't covered. |
@@ -15,48 +14,35 @@ def initialize | |||
end | |||
|
|||
def generate_report | |||
now = Time.now | |||
metrics = Unleash&.engine&.get_metrics() |
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.
Why is this optional here? It doesn't seem optional in client.rb
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 question! This may be my Rust opinions coming through here. The Client is owns the engine and makes it available to everything else when it's started up. Everything else may or may not have access to it, so this is paranoia and memory and ownership hygiene
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 looks reasonable to me. Can't wait to see Yggdrasil take over the rest of our SDKs as well :)
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!
After looking extremely superficially at the PR:
all of the above can be done in a separate branch, before merging to master, but it is not strictly required. an extra thought then is that it might also be a good idea to have a pointer in the README in the master branch a pointer to the pre release branch, so to make it more obvious what the roadmap is. This is amazing work! ❤️ I hope someone bakes you a cake 🍰 and buys you a beer 🍺 ! |
What's this?
This swaps out the logic for feature toggles in the SDK by using Yggdrasil. In particular, constructs that are now removed are:
For consideration
Perhaps controversial but I've also trimmed back the logic for custom strategies, a lot of this was to support the legacy way of registering custom strategies and has been marked as deprecated for some time. Yggdrasil won't let the caller get hooks into the custom strategies and so a lot of this isn't really possible anymore
We have no JRuby implementation right now, that's possible with Yggdrassil but not trivial. JRuby seems fairly important so we need to make a plan for this
Release strategy for this. It's a big change and we should be careful here