Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Yggdrasil engine #256

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Yggdrasil engine #256

wants to merge 24 commits into from
+440 −837

Conversation

sighphyre
Copy link
Member

@sighphyre sighphyre commented Nov 28, 2024 •
edited
Loading

Swaps out the evaluation of feature toggles for Yggdrasil

Notable changes

  • Previously the SDK would emit events for parent toggles when the child was evaluated. Looks like this SDK was doing something unusual. There's no longer a reasonable path to doing that since the SDK is no longer aware of the parent-child relationship, so it no longer does that
  • Metrics is now done purely through Yggdrasil. The abstraction on the metrics service + Unleash interface for counting toggles and variants has been removed. Yggdrasil now acts both as sink and source of metrics
  • Deprecated logic legacy variant distribution is removed. The plan was to remove this in the next major. That's now. User has confirmed on the open issue that they no longer need that behavior
  • Custom strategies are now handled by Yggdrasil via an adapter

To do

  • The Yggdrasil engine doesn't yet support feature_enabled, so the evaluation for variants needs to resolve the toggle so that it can calculate metrics
  • Platform classifier for working out which Yggdrasil jar to pull. Currently hardcoded to Linux x86_64 in the POM
  • MoreOperations needs to get trimmed. Direct access to feature toggle objects needs to get dropped
  • A lot of removal of domain objects

@coveralls
Copy link
Collaborator

coveralls commented Nov 28, 2024 •
edited
Loading

Coverage Status

coverage: 82.778% (-3.7%) from 86.458%
when pulling 76b99fa on yggdrasil-engine
into 19c1a3c on main.

sighphyre
sighphyre commented Nov 28, 2024
View reviewed changes
pom.xml
<groupId>io.getunleash</groupId>
<artifactId>yggdrasil-engine</artifactId>
<version>0.1.0-alpha.9</version>
<classifier>x86_64-linux</classifier>
Copy link
Member Author

@sighphyre sighphyre Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Classifier needs to be fixed before releasing this but I think that's a chunk of work distinct enough to want its own PR

sighphyre
sighphyre commented Nov 28, 2024
View reviewed changes
src/main/java/io/getunleash/FakeUnleash.java
@@ -107,17 +107,6 @@ public void disableAllExcept(String... excludedFeatures) {
}
}

@Override
public Variant deprecatedGetVariant(String toggleName, UnleashContext context) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a public interface change. This was in place to support legacy evaluation of variants. We're removing that in the next major, as per #234

sighphyre
sighphyre commented Nov 28, 2024
View reviewed changes
src/main/java/io/getunleash/FakeUnleash.java
@@ -176,15 +165,5 @@ public List<EvaluatedToggle> evaluateAllToggles(@Nullable UnleashContext context
getVariant(toggleName)))
.collect(Collectors.toList());
}

@Override
public void count(String toggleName, boolean enabled) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Public interface change. I don't see a reason why anyone would be using this but it is public. The Yggdrasil engine is now the sink for this data

sighphyre
sighphyre commented Nov 28, 2024
View reviewed changes
src/main/java/io/getunleash/metric/DefaultHttpMetricsSender.java
@@ -33,6 +35,7 @@ public DefaultHttpMetricsSender(UnleashConfig unleashConfig) {
this.gson =
new GsonBuilder()
.registerTypeAdapter(LocalDateTime.class, new DateTimeSerializer())
.registerTypeAdapter(Instant.class, new InstantSerializer())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yggdrasil deals in Instants not Datetimes. This allows us to serialize those to ISO 8601 when we send metrics

@sighphyre sighphyre marked this pull request as ready for review November 28, 2024 12:27
gastonfournier
gastonfournier approved these changes Nov 28, 2024
View reviewed changes
Copy link
Contributor

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great!

src/main/java/io/getunleash/metric/ClientMetrics.java
@@ -1,5 +1,6 @@
package io.getunleash.metric;

import io.getunleash.engine.MetricsBucket;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not needed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a bit sneaky but this was a drop in replacement for the existing metrics bucket. All I needed to do was include the new import and the code is unchanged

src/main/java/io/getunleash/metric/UnleashMetricServiceImpl.java Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastonfournier gastonfournier gastonfournier approved these changes

Assignees

@sighphyre sighphyre

Labels
None yet
Projects
Issues and PRs
Status: Approved PRs
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sighphyre @coveralls @gastonfournier