feat(Audience Evaluation): Audience Logging #152

rashidsp · 2019-01-08T17:33:46Z

Summary

This adds logging to audience evaluation.

Test plan

Unit tests written in

spec/audience_spec.rb
spec/custom_attribute_condition_evaluator_spec.rb

Issues

OASIS-3852

…nce match types

…nfig for new audience match types

…nce match types. (#145)

…fig for new audience match types (#146)

coveralls · 2019-01-08T17:41:00Z

Coverage remained the same at 100.0% when pulling fabf023 on rashid/logging-for-audience into 37872e6 on rashid/audience-match-types.

coveralls · 2019-01-08T17:41:00Z

Coverage remained the same at 100.0% when pulling fabf023 on rashid/logging-for-audience into 37872e6 on rashid/audience-match-types.

coveralls · 2019-01-08T17:41:00Z

Coverage decreased (-0.05%) to 99.945% when pulling 30d9330 on rashid/logging-for-audience into 4c8b534 on master.

oakbani

Looks good overall.
Missing attribute value Log Level was decided to be changed from WARNING to DEBUG.

oakbani · 2019-01-09T05:32:42Z

lib/optimizely/custom_attribute_condition_evaluator.rb

+
+ if user_provided_value.nil? && condition_match != EXISTS_MATCH_TYPE
+ @logger.log(
+ Logger::WARN,


Logger::DEBUG?

oakbani · 2019-01-09T05:40:41Z

lib/optimizely/custom_attribute_condition_evaluator.rb

+ unless value_valid_for_exact_conditions?(user_provided_value)
+ @logger.log(
+ Logger::WARN,
+ "Audience condition '#{condition}' evaluated as UNKNOWN because the value for user " \


This log is being used at multiple places. Can we make this a const? Or move all of the logs into a separate file?

This could be done for logger improvement at SDK level.
However i have moved log messages to constants file for now.

oakbani · 2019-01-09T05:51:22Z

spec/audience_spec.rb

- @config_body_json = OptimizelySpec::VALID_CONFIG_BODY_JSON
- @config_typed_audience_json = JSON.dump(OptimizelySpec::CONFIG_DICT_WITH_TYPED_AUDIENCES)
- end
+ let(:config_body) { OptimizelySpec::VALID_CONFIG_BODY }


nit. unused

oakbani · 2019-01-09T06:09:31Z

spec/audience_spec.rb

+ "Audience '3468206646' evaluated as 'true'."
+ )
+
+ expect(spy_logger).to have_received(:log).once.with(


can we assert that these logs have been called in the given sequence? OR assert the total number of calls to ensure nothing else is being logged?

oakbani · 2019-01-09T06:17:32Z

spec/custom_attribute_condition_evaluator_spec.rb

+ condition = {'match' => 'exact', 'name' => 'weird_condition', 'type' => 'weird', 'value' => 'hi'}
+ condition_evaluator = Optimizely::CustomAttributeConditionEvaluator.new({'weird_condition' => 'bye'}, spy_logger)
+ expect(condition_evaluator.evaluate(condition)).to eq(nil)
+ expect(spy_logger).to have_received(:log).once.with(


nit. Can we use a construct that ensures that :log is called once and only with this message?

oakbani · 2019-01-09T06:20:40Z

spec/custom_attribute_condition_evaluator_spec.rb

+ )
+ end
+
+ it 'should log and return nil if there user-provided value is of a unexpected type' do


nit. 'there'

…dience

…uby-sdk into rashid/logging-for-audience

nchilada

Looking good! Thanks for updating this in line with the other SDKs; I think I only found nits. 👏

nchilada · 2019-03-06T02:34:35Z

lib/optimizely/custom_attribute_condition_evaluator.rb

+ Helpers::Validator.finite_number?(condition_value) &&
+ Helpers::Validator.finite_number?(user_provided_value)
+
+ return true if user_provided_value == condition_value


Do we need to do some kind of special comparison instead of user_provided_value == condition_value in order to account for cases in which user_provided_value and condition_value are of different numeric types?

If not, I think it'd be cleaner to rely on the equality check that already occurs at the end of this function. At the very least, this would be consistent with most of our other SDKs. In exchange, we'd have to modify Helpers::Validator.same_types? to return true in the case of different numeric types.

+1. It will be much cleaner that way.

nchilada · 2019-03-06T02:47:04Z

lib/optimizely/custom_attribute_condition_evaluator.rb

+ format(Helpers::Constants::AUDIENCE_EVALUATION_LOGS['UNKNOWN_CONDITION_VALUE'], condition)
+ )
+ return nil
+ end


Nit: would be slightly better (and certainly more consistent with other SDKs) to reorder the above two blocks. If the condition_value isn't a string, that'd be indicative of a broader problem that might be resolved in a future SDK, where it might actually be legitimate for the user_provided_value to be a non-string.

(I know we check for non-nil user_provided_value before we get to either of these blocks, but at least that helps us increase code re-use...)

nchilada · 2019-03-06T02:51:19Z

spec/audience_spec.rb

+ user_attributes)).to be false
+ expect(spy_logger).to have_received(:log).once.with(
+ Logger::DEBUG,
+ "Evaluating audiences for experiment 'test_experiment_with_audience': #{experiment['audienceIds']}."


Can we avoid string interpolation in these tests? It'd be helpful to show what the final strings look like, especially since we're relying on Ruby's string representation of the underlying hash.

aliabbasrizvi

LGTM

nchilada

Looks great. Thanks again!

nchilada · 2019-03-06T17:51:39Z

lib/optimizely/helpers/validator.rb

 # false otherwise.
+ # Numeric values are considered as same type.
+
+ return true if value_1.is_a?(Numeric) && value_2.is_a?(Numeric)


Looks like it's okay to do the numeric check before the boolean check, in Ruby. I've confirmed that true and false are not Numerics.

nchilada · 2019-03-08T02:05:39Z

lib/optimizely/helpers/validator.rb

+ return true if value_1.is_a?(Numeric) && value_2.is_a?(Numeric)

 return true if boolean?(value_1) && boolean?(value_2)
- return true if value_1.is_a?(Integer) && value_2.is_a?(Integer)


Thanks for changing this! I just did some final QA for the 3.0 release, and I'm sure this would have been a problem if we hadn't changed it. We do expect numbers and floats to be interchangeable for the purposes of audience targeting. 👌

rashidsp added 14 commits December 19, 2018 16:33

feat (audience match types): Update condition evaluator for new audie…

001593f

…nce match types

adds absolute limit

0412be0

feat (audience match types): Update audience evaluator and project co…

80569a1

…nfig for new audience match types

feat (audience match types): Update condition evaluator for new audie…

ad7e32b

…nce match types. (#145)

feat(audience-match-types): Update audience evaluator and project con…

8363bbf

…fig for new audience match types (#146)

feat(attribute_value): Don't target NAN, INF, -INF and > 2^53 (#148)

3990bf1

feat(audiences): Audience combinations (#147)

0c6c795

resolves conflicts

8d2b3ee

Updates header and resolves conflicts

9291b97

nit: header update

f5b37b2

Merge branch 'master' into rashid/audience-match-types

58330b3

fix: travis bundler issue

8453091

Merge branch 'master' into rashid/audience-match-types

37872e6

feat(Audience Evaluation): Audience Logging

fabf023

rashidsp requested a review from oakbani January 8, 2019 17:33

rashidsp added the WIP Work in progress label Jan 8, 2019

oakbani reviewed Jan 9, 2019

View reviewed changes

rashidsp added 3 commits January 9, 2019 13:15

Merge branch 'master' into rashid/audience-match-types

4d9ad8f

Merge branch 'rashid/audience-match-types' into rashid/logging-for-au…

e438ad2

…dience

addresses comments

0238950

rashidsp changed the base branch from rashid/audience-match-types to master January 9, 2019 11:56

rashidsp self-assigned this Jan 9, 2019

rashidsp removed the WIP Work in progress label Jan 9, 2019

rashidsp requested a review from a team January 9, 2019 12:09

rashidsp added 3 commits January 10, 2019 17:13

Fix: user attribute check

479bc84

Merge branch 'master' into rashid/logging-for-audience

b591d49

revises logging improvements

527d699

rashidsp added 9 commits January 21, 2019 13:48

Merge branch 'master' into rashid/logging-for-audience

0b7958e

Updated according to doc

ad7745d

Merge branch 'rashid/logging-for-audience' of github.com:optimizely/r…

7234863

…uby-sdk into rashid/logging-for-audience

Fix: return str bug.

5468d37

refact: new recommendations.

eb9770b

refact: separated finite_number condition

455a697

Addressing feedback

89184d7

few minor updates

006dd0f

Addressing feedback

8986472

nchilada reviewed Mar 6, 2019

View reviewed changes

review changes

30d9330

aliabbasrizvi approved these changes Mar 6, 2019

View reviewed changes

nchilada approved these changes Mar 6, 2019

View reviewed changes

nchilada merged commit c2a0931 into master Mar 6, 2019

nchilada deleted the rashid/logging-for-audience branch March 6, 2019 17:52

nchilada reviewed Mar 8, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(Audience Evaluation): Audience Logging #152

feat(Audience Evaluation): Audience Logging #152

Uh oh!

rashidsp commented Jan 8, 2019 •

edited by nchilada

Loading

coveralls commented Jan 8, 2019

coveralls commented Jan 8, 2019

coveralls commented Jan 8, 2019 •

edited

Loading

oakbani left a comment

oakbani Jan 9, 2019

oakbani Jan 9, 2019

rashidsp Jan 9, 2019 •

edited

Loading

oakbani Jan 9, 2019

oakbani Jan 9, 2019

oakbani Jan 9, 2019

oakbani Jan 9, 2019

nchilada left a comment

nchilada Mar 6, 2019

aliabbasrizvi Mar 6, 2019

nchilada Mar 6, 2019

nchilada Mar 6, 2019

aliabbasrizvi left a comment

nchilada left a comment

nchilada Mar 6, 2019

nchilada Mar 8, 2019

Labels

5 participants

feat(Audience Evaluation): Audience Logging #152

feat(Audience Evaluation): Audience Logging #152

Uh oh!

Conversation

rashidsp commented Jan 8, 2019 • edited by nchilada Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Issues

coveralls commented Jan 8, 2019

coveralls commented Jan 8, 2019

coveralls commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

oakbani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rashidsp Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

nchilada left a 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

aliabbasrizvi left a comment

Choose a reason for hiding this comment

nchilada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

5 participants

rashidsp commented Jan 8, 2019 •

edited by nchilada

Loading

coveralls commented Jan 8, 2019 •

edited

Loading

rashidsp Jan 9, 2019 •

edited

Loading