-
Notifications
You must be signed in to change notification settings - Fork 6
feat: get evaluation get result #88
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
base: feat/evaluation-context-mappers
Are you sure you want to change the base?
Changes from all commits
2aeb65a
ba057cb
bb96eb1
a2fd5a8
0f08fb1
da08e8e
af51bf7
0fb7587
741ceeb
6a6a129
3bad95b
f0a53b7
ef1274a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,20 +1,173 @@ | ||||||||||
| # frozen_string_literal: true | ||||||||||
|
|
||||||||||
| require_relative '../utils/hash_func' | ||||||||||
| require_relative '../features/constants' | ||||||||||
| require_relative '../segments/evaluator' | ||||||||||
|
|
||||||||||
| module Flagsmith | ||||||||||
| module Engine | ||||||||||
| module Evaluation | ||||||||||
| # Core evaluation logic module | ||||||||||
| module Core | ||||||||||
| extend self | ||||||||||
| include Flagsmith::Engine::Utils::HashFunc | ||||||||||
| include Flagsmith::Engine::Features::TargetingReasons | ||||||||||
| include Flagsmith::Engine::Segments::Evaluator | ||||||||||
| # Get evaluation result from evaluation context | ||||||||||
| # | ||||||||||
| # @param evaluation_context [Hash] The evaluation context | ||||||||||
| # @return [Hash] Evaluation result with flags and segments | ||||||||||
| def self.get_evaluation_result(_evaluation_context) | ||||||||||
| # TODO: Implement core evaluation logic | ||||||||||
| # returns EvaluationResultWithMetadata | ||||||||||
| def get_evaluation_result(evaluation_context) | ||||||||||
| segments, segment_overrides = evaluate_segments(evaluation_context) | ||||||||||
| flags = evaluate_features(evaluation_context, segment_overrides) | ||||||||||
| { | ||||||||||
| flags: {}, | ||||||||||
| segments: [] | ||||||||||
| flags: flags, | ||||||||||
| segments: segments | ||||||||||
| } | ||||||||||
| end | ||||||||||
|
|
||||||||||
| # Returns { segments: EvaluationResultSegments; segmentOverrides: Record<string, SegmentOverride>; } | ||||||||||
| def evaluate_segments(evaluation_context) | ||||||||||
| return [], {} if evaluation_context[:identity].nil? || evaluation_context[:segments].nil? | ||||||||||
|
|
||||||||||
| identity_segments = get_identity_segments_from_context(evaluation_context) | ||||||||||
|
|
||||||||||
| segments = identity_segments.map do |segment| | ||||||||||
| result = { | ||||||||||
| name: segment[:name] | ||||||||||
| } | ||||||||||
|
|
||||||||||
| result[:metadata] = segment[:metadata] if segment[:metadata] | ||||||||||
|
|
||||||||||
| result | ||||||||||
| end | ||||||||||
|
|
||||||||||
| segment_overrides = process_segment_overrides(identity_segments) | ||||||||||
|
|
||||||||||
| [segments, segment_overrides] | ||||||||||
| end | ||||||||||
|
|
||||||||||
| # Returns Record<string: override.name, SegmentOverride> | ||||||||||
| def process_segment_overrides(identity_segments) | ||||||||||
| segment_overrides = {} | ||||||||||
|
|
||||||||||
| identity_segments.each do |segment| | ||||||||||
| next unless segment[:overrides] | ||||||||||
|
|
||||||||||
| overrides_list = segment[:overrides].is_a?(Array) ? segment[:overrides] : [] | ||||||||||
|
|
||||||||||
| overrides_list.each do |override| | ||||||||||
| next unless should_apply_override(override, segment_overrides) | ||||||||||
|
|
||||||||||
| segment_overrides[override[:name]] = { | ||||||||||
| feature: override, | ||||||||||
| segment_name: segment[:name] | ||||||||||
| } | ||||||||||
| end | ||||||||||
| end | ||||||||||
|
|
||||||||||
| segment_overrides | ||||||||||
| end | ||||||||||
|
|
||||||||||
| # returns EvaluationResultFlags<Metadata> | ||||||||||
| def evaluate_features(evaluation_context, segment_overrides) | ||||||||||
| flags = {} | ||||||||||
|
|
||||||||||
| (evaluation_context[:features] || {}).each_value do |feature| | ||||||||||
| segment_override = segment_overrides[feature[:name]] | ||||||||||
| final_feature = segment_override ? segment_override[:feature] : feature | ||||||||||
| has_override = !segment_override.nil? | ||||||||||
|
|
||||||||||
| # Evaluate feature value | ||||||||||
| evaluated = if has_override | ||||||||||
| { value: final_feature[:value], reason: nil } | ||||||||||
| else | ||||||||||
| evaluate_feature_value(final_feature, get_identity_key(evaluation_context)) | ||||||||||
| end | ||||||||||
|
Comment on lines
+83
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probably reproduces the bug existing in the reference implementation, covered by test_multivariate__segment_override__expected_allocation. Can we add tests to CI so we don't have to guess? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not part of this sprint? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you're confusing with Flagsmith/flagsmith-python-client#172 (and other similar issues in Flagsmith/flagsmith#5977) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it is — I don't think we should ship with that bug. The test covering it is part of engine-test-data anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I remember, I raised this during the standup to fix it in Rust, but we decided not to include it in this sprint. Is this bug present in all the SDKs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We’re not releasing without the fix, but it can be included in a separate pull request. As far as I remember, that was the decision — please correct me if I’m mistaken. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed offline, there's no reason not to inlclude the fix in this PR. |
||||||||||
|
|
||||||||||
| # Build flag result | ||||||||||
| flag_result = { | ||||||||||
| name: final_feature[:name], | ||||||||||
| enabled: final_feature[:enabled], | ||||||||||
| value: evaluated[:value] | ||||||||||
| } | ||||||||||
|
|
||||||||||
| # Add metadata if present | ||||||||||
| flag_result[:metadata] = final_feature[:metadata] if final_feature[:metadata] | ||||||||||
|
|
||||||||||
| # Set reason | ||||||||||
| flag_result[:reason] = evaluated[:reason] || | ||||||||||
| get_targeting_match_reason({ type: 'SEGMENT', override: segment_override }) | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why we need this additional level of misdirection with |
||||||||||
|
|
||||||||||
| flags[final_feature[:name].to_sym] = flag_result | ||||||||||
| end | ||||||||||
|
|
||||||||||
| flags | ||||||||||
| end | ||||||||||
|
|
||||||||||
| # Returns {value: any; reason?: string} | ||||||||||
| def evaluate_feature_value(feature, identity_key = nil) | ||||||||||
| return get_multivariate_feature_value(feature, identity_key) if feature[:variants]&.any? && identity_key | ||||||||||
|
|
||||||||||
| { value: feature[:value], reason: nil } | ||||||||||
| end | ||||||||||
|
|
||||||||||
| # Returns {value: any; reason?: string} | ||||||||||
| def get_multivariate_feature_value(feature, identity_key) | ||||||||||
| percentage_value = hashed_percentage_for_object_ids([feature[:key], identity_key]) | ||||||||||
| sorted_variants = (feature[:variants] || []).sort_by { |v| v[:priority] || Float::INFINITY } | ||||||||||
|
|
||||||||||
| start_percentage = 0 | ||||||||||
| sorted_variants.each do |variant| | ||||||||||
| limit = start_percentage + variant[:weight] | ||||||||||
| if start_percentage <= percentage_value && percentage_value < limit | ||||||||||
| return { | ||||||||||
| value: variant[:value], | ||||||||||
| reason: get_targeting_match_reason({ type: 'SPLIT', weight: variant[:weight] }) | ||||||||||
| } | ||||||||||
| end | ||||||||||
| start_percentage = limit | ||||||||||
| end | ||||||||||
|
|
||||||||||
| { value: feature[:value], reason: nil } | ||||||||||
| end | ||||||||||
|
|
||||||||||
| # returns boolean | ||||||||||
| def should_apply_override(override, existing_overrides) | ||||||||||
| current_override = existing_overrides[override[:name]] | ||||||||||
| !current_override || higher_priority?(override[:priority], current_override[:feature][:priority]) | ||||||||||
| end | ||||||||||
|
|
||||||||||
| private | ||||||||||
|
|
||||||||||
| # Extract identity key from evaluation context | ||||||||||
| # | ||||||||||
| # @param evaluation_context [Hash] The evaluation context | ||||||||||
| # @return [String, nil] The identity key or nil if no identity | ||||||||||
| def get_identity_key(evaluation_context) | ||||||||||
| return nil unless evaluation_context[:identity] | ||||||||||
|
|
||||||||||
| evaluation_context[:identity][:key] || | ||||||||||
| "#{evaluation_context[:environment][:key]}_#{evaluation_context[:identity][:identifier]}" | ||||||||||
| end | ||||||||||
|
|
||||||||||
| # returns boolean | ||||||||||
| def higher_priority?(priority_a, priority_b) | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Or maybe even
Suggested change
|
||||||||||
| (priority_a || Float::INFINITY) < (priority_b || Float::INFINITY) | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Maybe have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can see it addressed in #89; feel free to resolve or integrate the changes here. |
||||||||||
| end | ||||||||||
|
|
||||||||||
| def get_targeting_match_reason(match_object) | ||||||||||
| type = match_object[:type] | ||||||||||
|
|
||||||||||
| if type == 'SEGMENT' | ||||||||||
| return match_object[:override] ? "#{TARGETING_REASON_TARGETING_MATCH}; segment=#{match_object[:override][:segment_name]}" : TARGETING_REASON_DEFAULT | ||||||||||
| end | ||||||||||
|
|
||||||||||
| return "#{TARGETING_REASON_SPLIT}; weight=#{match_object[:weight]}" if type == 'SPLIT' | ||||||||||
|
|
||||||||||
| TARGETING_REASON_DEFAULT | ||||||||||
| end | ||||||||||
| end | ||||||||||
| end | ||||||||||
| end | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Flagsmith | ||
| module Engine | ||
| module Features | ||
| # Targeting reason constants for evaluation results | ||
| module TargetingReasons | ||
| TARGETING_REASON_DEFAULT = 'DEFAULT' | ||
| TARGETING_REASON_TARGETING_MATCH = 'TARGETING_MATCH' | ||
| TARGETING_REASON_SPLIT = 'SPLIT' | ||
| end | ||
| end | ||
| end | ||
| end |
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'm insisting that
if evaluation_context[:identity].nil?part should happen in the mapper layer. We're not supposed to prevent identity-less evaluation in the engine, we're maintaining compatibility with/v1/flagsfor theget_environment_flagsSDK interface.