fix: experiment variation results order (#845)

pkg/eventcounter/api/api.go

@@ -18,7 +18,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"sort"
 	"strings"
 	"time"
 
@@ -231,10 +230,10 @@ func (s *eventCounterService) convertEvaluationCounts(
 		vcsMap[row.VariationID] = vc
 	}
 	vcs := make([]*ecproto.VariationCount, 0, len(vcsMap))
-	for _, vc := range vcsMap {
-		vcs = append(vcs, vc)
+	// we should keep the order of variationIDs
+	for _, vid := range variationIDs {
+		vcs = append(vcs, vcsMap[vid])
 	}
-	sort.SliceStable(vcs, func(i, j int) bool { return vcs[i].VariationId < vcs[j].VariationId })
 	return vcs
 }
 
@@ -1012,10 +1011,10 @@ func (s *eventCounterService) convertGoalCounts(
 		vcsMap[row.VariationID] = vc
 	}
 	vcs := make([]*ecproto.VariationCount, 0, len(vcsMap))
-	for _, vc := range vcsMap {
-		vcs = append(vcs, vc)
+	// we should keep the order of variationIDs
+	for _, vid := range variationIDs {
+		vcs = append(vcs, vcsMap[vid])
 	}
-	sort.SliceStable(vcs, func(i, j int) bool { return vcs[i].VariationId < vcs[j].VariationId })
 	return vcs
 }
 

pkg/experimentcalculator/experimentcalc/experiment_calculator.go

@@ -19,7 +19,6 @@ import (
 	"context"
 	_ "embed"
 	"errors"
-	"sort"
 	"sync"
 	"time"
 
@@ -347,26 +346,29 @@ func (e ExperimentCalculator) appendVariationResult(
 	goalResult *eventcounter.GoalResult,
 	srcVrs []*eventcounter.VariationResult,
 ) {
-	sort.SliceStable(goalResult.VariationResults, func(i, j int) bool {
-		return goalResult.VariationResults[i].VariationId < goalResult.VariationResults[j].VariationId
-	})
-	sort.SliceStable(srcVrs, func(i, j int) bool {
-		return srcVrs[i].VariationId < srcVrs[j].VariationId
-	})
 	for i := 0; i < len(goalResult.VariationResults); i++ {
-		goalResult.VariationResults[i].ExperimentCount = copyVariationCount(srcVrs[i].ExperimentCount)
-		goalResult.VariationResults[i].EvaluationCount = copyVariationCount(srcVrs[i].EvaluationCount)
+		variationResult := getVariationResult(srcVrs, goalResult.VariationResults[i].VariationId)
+		if variationResult == nil {
+			e.logger.Error("Variation result not found",
+				log.FieldsFromImcomingContext(ctx).AddFields(
+					zap.String("variation_id", goalResult.VariationResults[i].VariationId),
+				)...,
+			)
+			continue
+		}
+		goalResult.VariationResults[i].ExperimentCount = copyVariationCount(variationResult.ExperimentCount)
+		goalResult.VariationResults[i].EvaluationCount = copyVariationCount(variationResult.EvaluationCount)
 
-		goalResult.VariationResults[i].CvrProb = copyDistributionSummary(srcVrs[i].CvrProb)
-		goalResult.VariationResults[i].CvrProbBest = copyDistributionSummary(srcVrs[i].CvrProbBest)
-		goalResult.VariationResults[i].CvrProbBeatBaseline = copyDistributionSummary(srcVrs[i].CvrProbBeatBaseline)
+		goalResult.VariationResults[i].CvrProb = copyDistributionSummary(variationResult.CvrProb)
+		goalResult.VariationResults[i].CvrProbBest = copyDistributionSummary(variationResult.CvrProbBest)
+		goalResult.VariationResults[i].CvrProbBeatBaseline = copyDistributionSummary(variationResult.CvrProbBeatBaseline)
 
 		goalResult.VariationResults[i].GoalValueSumPerUserProb =
-			copyDistributionSummary(srcVrs[i].GoalValueSumPerUserProb)
+			copyDistributionSummary(variationResult.GoalValueSumPerUserProb)
 		goalResult.VariationResults[i].GoalValueSumPerUserProbBest =
-			copyDistributionSummary(srcVrs[i].GoalValueSumPerUserProbBest)
+			copyDistributionSummary(variationResult.GoalValueSumPerUserProbBest)
 		goalResult.VariationResults[i].GoalValueSumPerUserProbBeatBaseline =
-			copyDistributionSummary(srcVrs[i].GoalValueSumPerUserProbBeatBaseline)
+			copyDistributionSummary(variationResult.GoalValueSumPerUserProbBeatBaseline)
 
 		goalResult.VariationResults[i].EvaluationUserCountTimeseries = &eventcounter.Timeseries{
 			Timestamps: []int64{timestamp},
@@ -542,6 +544,15 @@ func listEndAt(startAt, endAt, now int64) []int64 {
 	return timestamps
 }
 
+func getVariationResult(vcs []*eventcounter.VariationResult, id string) *eventcounter.VariationResult {
+	for _, vc := range vcs {
+		if vc.VariationId == id {
+			return vc
+		}
+	}
+	return nil
+}
+
 func copyVariationCount(from *eventcounter.VariationCount) *eventcounter.VariationCount {
 	return &eventcounter.VariationCount{
 		VariationId:             from.VariationId,

test/e2e/eventcounter/eventcounter_test.go

@@ -418,8 +418,8 @@ func TestGrpcExperimentResult(t *testing.T) {
 			if len(gr.VariationResults) != 2 {
 				t.Fatalf("the number of variation results is not correct: %d", len(gr.VariationResults))
 			}
-			vsA := getVariationResult(gr.VariationResults, experiment.Variations[0].Id)
-			vsB := getVariationResult(gr.VariationResults, experiment.Variations[1].Id)
+			vsA := gr.VariationResults[0]
+			vsB := gr.VariationResults[1]
 			// These counts are based on the number of events sent earlier
 			if vsA.EvaluationCount.EventCount != 4 || // variation A
 				vsA.EvaluationCount.UserCount != 3 ||
@@ -657,8 +657,8 @@ func TestExperimentResult(t *testing.T) {
 			if len(gr.VariationResults) != 2 {
 				t.Fatalf("the number of variation results is not correct: %d", len(gr.VariationResults))
 			}
-			vsA := getVariationResult(gr.VariationResults, experiment.Variations[0].Id)
-			vsB := getVariationResult(gr.VariationResults, experiment.Variations[1].Id)
+			vsA := gr.VariationResults[0]
+			vsB := gr.VariationResults[1]
 			// These counts are based on the number of events sent earlier
 			if vsA.EvaluationCount.EventCount != 4 || // variation A
 				vsA.EvaluationCount.UserCount != 3 ||
@@ -1588,15 +1588,6 @@ func getVariationCount(vcs []*ecproto.VariationCount, id string) *ecproto.Variat
 	return nil
 }
 
-func getVariationResult(vcs []*ecproto.VariationResult, id string) *ecproto.VariationResult {
-	for _, vc := range vcs {
-		if vc.VariationId == id {
-			return vc
-		}
-	}
-	return nil
-}
-
 func newExperimentClient(t *testing.T) experimentclient.Client {
 	t.Helper()
 	creds, err := rpcclient.NewPerRPCCredentials(*serviceTokenPath)