[FFM-2186] integration of the evaluation engine into the sdk client

[enver-bisevac]

No description provided.

[davejohnston]

The usage of this ProcessEvaluation feels a little strange to me, when reading the code.
It looks like the only place we use it is from the Clients BoolVariation,StringVariation functions (where we pass

c.analyticsService.PushToQueue to the relevant method of the Evaluator.

Can we separate evaluation from posting metrics, and call this from the Client?

Another option might be for the Evaluator to manage a channel which the Client could listen on. Any time an evaluation is successfully complete the evaluator can write to the channel.
The Client can have a routine somewhere reading from the channel and processing the metrics.

[davejohnston]

Overall looks good. I have some questions/comments. @conormurray95 @jcox250 it would be good to look at this from the PoV of the proxy, and any work you have added to support that.

[conormurray95]

We'll need a ticket on the proxy side to upgrade to this new version. We have code that converts the internal evaluation types back to the rest types before we cache them that we won't need anymore which is good. I can raise that

[jcox250]

I think this could potentially block forever if whatever is listening to postEvalChan isn't ready to receive. If we pass a context in to this function then we could do something like this so even if the write to the channel is blocked there's still a way to break out of this function.

data :=  PostEvalData{
	FeatureConfig: &flag,
	Target:        target,
	Variation:     &variation,
}

// We wait until we can either write to the channel or the context has been cancelled.
select {
  case e.postEvalChan <- data:
  case <-ctx.Done():
      return rest.Variation{}, ctx.Err()
      }
}

It looks like this function gets called by BoolVariation, StringVariation so I don't know if it maybe makes sense to make them take a context and then pass it down to this function?

[enver-bisevac]

I agree, but still it will block until receiver is ready, this will only solve when context is canceled for example on closing the client, right?

Maybe to create non blocking with go routine and wait groups

[jcox250]

Yea it will still block if the receiver isn't ready and can only be canceled by the client but it would at least mean we don't have a block of code that could block forever without giving the caller some way to stop it.

[jcox250]

Thinking out loud here and making some assumptions, but it looks like the purpose of the channel is to send evaluations to some caller that created the Evaluator using NewEvaluator? What if instead of using a channel we allowed them to pass in a function and then we call it instead of writing to a channel?

Something like this but with better naming of things than what I've given 😄

// 
type evalFn func (data PostEvalData)

func NewEvaluator(query Query, evalCallback evalFn) (*Evaluator, error) {
   ... code
}

func (e Evaluator) evaluate(identifier string, target *Target, kind string) (rest.Variation, error) {
   if e.query == nil {
		log.Errorf(ErrQueryProviderMissing.Error())
		return rest.Variation{}, ErrQueryProviderMissing
	}
	flag, err := e.query.GetFlag(identifier)
	if err != nil {
		return rest.Variation{}, err
	}

	if flag.Kind != kind {
		return rest.Variation{}, fmt.Errorf("%w, expected: %s, got: %s", ErrFlagKindMismatch, kind, flag.Kind)
	}

	if flag.Prerequisites != nil {
		prereq, err := e.checkPreRequisite(&flag, target)
		if err != nil || !prereq {
			return findVariation(flag.Variations, flag.OffVariation)
		}
	}
	variation, err := e.evaluateFlag(flag, target)
	if err != nil {
		return rest.Variation{}, err
	}
	if e.evalCallbackFn != nil {
		data := PostEvalData{
			FeatureConfig: &flag,
			Target:        target,
			Variation:     &variation,
		}

                e.evalCallback(data)
	}
	return variation, nil
}

[enver-bisevac]

if you look prev commit it was done in that way but @davejohnston suggested to decouple using channels and go routine
https://github.com/harness/ff-golang-server-sdk/pull/75/commits/ad78e24ffc37333123fae955004c3a8404e26a77#diff-4711ab68b7e3b590be90997cfb086cfe1e0d6456e76b35de90696561f6633962

I use func instead of interface but the logic is the same

[davejohnston]

@enver-bisevac I think there is a subtle difference in what James is proposing and the first revision. And it makes sense.
The Evaluator itself has the 'processor' rather than it being passed through from the each BoolVariation, StringVariation funcs
I'm fine with that, as it still separates the logic out - the caller of BoolVariation/IntVariation doesn't need to construct and pass in a 'processor' to each function. Instead the evaluator decides what to do (based on how it was created)

func (e Evaluator) IntVariation(identifier string, target *Target, defaultValue int, processEvaluation ProcessEvaluation) int {

	variation, err := e.evaluate(identifier, target, "int", processEvaluation)

Instead it would look like this (assuming you follow James example above where the processor is a member of the evaluator).

func (e Evaluator) IntVariation(identifier string, target *Target, defaultValue int) int {
	variation, err := e.evaluate(identifier, target, "int")

[enver-bisevac]

Ok, I agree with interface approach too, but I understand you woild like to do it in a manner of channel communication? So I will create a commit with interface as argument to factory function

[davejohnston]

LGTM