refact(audience-logs): Added and refactored audience evaluation logs. #280

yasirfolio3 · 2020-07-14T09:44:21Z

Summary

Added and updated missing audience evaluation logs.

Testplan

All unit tests and FSC must pass.

Performance

There was no significant hit observed.

Master with default log level: 8.5s
Master with debug log level: 47.5s

PR changes with default loglevel: 8.5s
PR changes with debug log level: 48s

Used this code to measure performance.

	logging.SetLogLevel(logging.LogLevelDebug)
	optimizelyFactory := &client.OptimizelyFactory{SDKKey: ""}
	datafile, err := ioutil.ReadFile(filepath.Clean(path.Join(os.Getenv("DATAFILES_DIR"), "100_entities.json")))
	if err != nil {
		log.Fatal(err)
	}
	optimizelyFactory.Datafile = datafile
	client, err := optimizelyFactory.Client()
	start := time.Now()
	count := 5000
	for i := 0; i < count; i++ {
		client.GetEnabledFeatures(entities.UserContext{ID: "", Attributes: map[string]interface{}{}})
	}
	elapsed := time.Since(start)
	log.Printf("took %s", elapsed)

Issues

https://optimizely.atlassian.net/browse/OASIS-6816

msohailhussain · 2020-07-14T22:36:32Z

pkg/decision/evaluator/audience.go

@@ -1,45 +0,0 @@
-/****************************************************************************


Why deleted this file?

this file wasn't being used anywhere throughout the sdk and was throwing syntax errors due to the changes made in this PR.

msohailhussain · 2020-07-14T22:40:31Z

pkg/decision/evaluator/condition.go

 	if condition.Type != customAttributeType {
-		return false, fmt.Errorf(`unable to evaluator condition of type "%s"`, condition.Type)
+
+		c.logger.Warning(fmt.Sprintf(string(logging.UnknownConditionType), condition.StringRepresentation))


Why string is needed? string(logging.UnknownConditionType)

Since UnknownConditionType is of custom type, we must convert it to string before formatting.

msohailhussain · 2020-07-14T22:41:30Z

pkg/decision/evaluator/condition.go

 	case exactMatchType:
 		matcher = matchers.ExactMatcher{
 			Condition: condition,
+			Logger:    c.logger,


Why logger not passed in existsMatchType

No logs were required inside exists.

msohailhussain

first pass

msohailhussain · 2020-07-14T22:41:49Z

pkg/decision/evaluator/condition.go

+			Logger:    c.logger,
 		}
 	default:
+		c.logger.Warning(fmt.Sprintf(string(logging.UnknownMatchType), condition.StringRepresentation))


Please use format specifier.

msohailhussain · 2020-07-14T22:44:43Z

pkg/decision/evaluator/matchers/lt.go

-	}
-
-	return false, fmt.Errorf("audience condition %s evaluated to NULL because the condition value type is not supported", m.Condition.Name)
+	return matchGtOrLt(user, m.Condition, m.Logger, false)


Why we didn't only add log here.

since both gt and lt matchers were very similar, linter was throwing lines are duplicate error.

msohailhussain · 2020-07-14T22:47:13Z

pkg/entities/user_context.go

 }

+// GetAttribute returns the value for the specified attribute name in the attributes map. Returns error if not found.
+func (u UserContext) GetAttribute(attrName string) (interface{}, error) {


Why added this method?

for InvalidAttributeValueType log, we required the raw attribute value to log its original type.

msohailhussain · 2020-07-14T22:47:55Z

pkg/logging/log_messages.go

@@ -0,0 +1,60 @@
+/****************************************************************************
+ * Copyright 2019-2020, Optimizely, Inc. and contributors                   *


Is it started in 2019?

msohailhussain · 2020-07-14T22:48:23Z

pkg/logging/log_messages.go

+package logging
+
+// LogMessage defines string type for log messages
+type LogMessage string


I am not sure if this is the right location to add log messages. Let's discuss offline.

msohailhussain · 2020-07-14T23:13:24Z

pkg/decision/evaluator/condition.go

 	if condition.Type != customAttributeType {
-		return false, fmt.Errorf(`unable to evaluator condition of type "%s"`, condition.Type)
+
+		c.logger.Warning(fmt.Sprintf(string(logging.UnknownConditionType), condition.StringRepresentation))


We don't really need two Strings. Just pass the same in fmt.Errorf

I will confirm and let you know.

…o-sdk into yasir/fix-audienceeval-logs

msohailhussain · 2020-07-15T22:45:00Z

pkg/decision/evaluator/matchers/exact.go

+
+	if !user.CheckAttributeExists(m.Condition.Name) {
+		m.Logger.Debug(fmt.Sprintf(logging.NullUserAttribute.String(), m.Condition.StringRepresentation, m.Condition.Name))
+		return false, fmt.Errorf(`no attribute named "%s"`, m.Condition.Name)


Why we don't return same logging message?

…o-sdk into yasir/fix-audienceeval-logs

mikecdavis

LGTM, one nit on the matchGtOrLt refactor.

mikecdavis · 2020-08-20T19:20:21Z

pkg/decision/evaluator/matchers/gt.go

+	return matchGtOrLt(condition, user, logger, true)
+}
+
+func matchGtOrLt(condition entities.Condition, user entities.UserContext, logger logging.OptimizelyLogProducer, gtMatch bool) (bool, error) {


Nit, but If we make this a pure comparator and return (int, bool) e.g. -1, 0, 1, then we can drop the gtMatch bool which is a bit awkward. Then consumers would look like:

func GtMatcher(condition entities.Condition, user entities.UserContext, logger logging.OptimizelyLogProducer) (bool, error) { res, err := compare(condition, user, logger) if err := nil { return false, err } return res > 0, nil } func LtMatcher(condition entities.Condition, user entities.UserContext, logger logging.OptimizelyLogProducer) (bool, error) { res, err := compare(condition, user, logger) if err := nil { return false, err } return res < 0, nil }

This would match the logic in the semver PR here

msohailhussain

lgtm

yasirfolio3 added 3 commits July 10, 2020 18:07

Initial commit.

6757e72

Unit tests added.

dc4c618

fixing code duplication.

c66092e

yasirfolio3 added the wip label Jul 14, 2020

yasirfolio3 requested a review from msohailhussain July 14, 2020 09:44

Merge branch 'master' into yasir/fix-audienceeval-logs

ae28341

msohailhussain reviewed Jul 14, 2020

View reviewed changes

msohailhussain suggested changes Jul 14, 2020

View reviewed changes

msohailhussain reviewed Jul 14, 2020

View reviewed changes

yasirfolio3 added 2 commits July 15, 2020 11:55

string method added to LogMessage file.

0f1a60d

Merge branch 'yasir/fix-audienceeval-logs' of github.com:optimizely/g…

d12d565

…o-sdk into yasir/fix-audienceeval-logs

yasirfolio3 mentioned this pull request Jul 15, 2020

fix(logs): Fixing log messages for Targeted Rollouts and feature variable evaluation. #281

Closed

msohailhussain reviewed Jul 15, 2020

View reviewed changes

msohailhussain marked this pull request as ready for review July 15, 2020 22:49

msohailhussain requested a review from a team as a code owner July 15, 2020 22:49

msohailhussain removed the wip label Jul 15, 2020

msohailhussain changed the title ~~refact(audience-logs): Added and refactored audience evaluation logs. (WIP)~~ refact(audience-logs): Added and refactored audience evaluation logs. Jul 15, 2020

optimizely-codereview-bot added the 🤖 12 hrs label Jul 15, 2020

msohailhussain requested a review from pawels-optimizely July 15, 2020 23:19

Merge branch 'master' into yasir/fix-audienceeval-logs

47d6587

optimizely-codereview-bot added 🤖 16 hrs 🤖 20 hrs 🤖 24 hrs and removed 🤖 12 hrs 🤖 16 hrs 🤖 20 hrs labels Jul 16, 2020

msohailhussain and others added 2 commits August 3, 2020 10:22

Merge branch 'master' into yasir/fix-audienceeval-logs

74aa1f6

updating new info logs to debug.

af4847f

yasirfolio3 added 5 commits August 3, 2020 22:26

Merge branch 'yasir/fix-audienceeval-logs' of github.com:optimizely/g…

6b11c70

…o-sdk into yasir/fix-audienceeval-logs

tests fixed.

81e304a

fixes.

3e8c1f5

Merge branch 'master' into yasir/fix-audienceeval-logs

d692e84

linter warnings fixed.

6e1116b

msohailhussain requested a review from mikecdavis August 20, 2020 19:06

mikecdavis approved these changes Aug 20, 2020

View reviewed changes

yasirfolio3 self-assigned this Aug 21, 2020

optimizely-codereview-bot removed the 🤖 24 hrs label Aug 21, 2020

fixes.

5cdef75

msohailhussain approved these changes Aug 21, 2020

View reviewed changes

msohailhussain unassigned yasirfolio3 Aug 21, 2020

mikeproeng37 merged commit 6974ef4 into master Aug 21, 2020

mikeproeng37 deleted the yasir/fix-audienceeval-logs branch August 21, 2020 17:17

yasirfolio3 restored the yasir/fix-audienceeval-logs branch August 24, 2020 07:23

yasirfolio3 deleted the yasir/fix-audienceeval-logs branch August 24, 2020 08:08

		@@ -1,45 +0,0 @@
		/****************************************************************************

		@@ -0,0 +1,60 @@
		/****************************************************************************
		* Copyright 2019-2020, Optimizely, Inc. and contributors *

refact(audience-logs): Added and refactored audience evaluation logs. #280

refact(audience-logs): Added and refactored audience evaluation logs. #280

Uh oh!

Conversation

yasirfolio3 commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testplan

Performance

Issues

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msohailhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikecdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msohailhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yasirfolio3 commented Jul 14, 2020 •

edited

Loading