Add additional experiment metadata to Fetch Treatment response #36

krithika369 · 2022-09-16T07:27:07Z

What this PR does / why we need it:
This PR adds additional metadata information to the Fetch Treatment response.

{
    "data": {
        "experiment_id": 2,
        "experiment_name": "expertiment-2-sw",
        "metadata": { // Newly added field
            "experiment_type": "Switchback",
            "experiment_version": 2,
            "switchback_window_id": 1 // Only set for switchback experiments, starts from 0.
        },
        "treatment": {
            "configuration": {
                "test": "val"
            },
            "name": "treatment-b",
            "traffic": 0
        }
    }
}

The version field is now added to the database model for experiments, to reliably store the version of the experiment. Consequently, this field is also added to the Experiment Details view.

Summary of Changes

The main changes are summarised below.

Schema

api/proto/experiment.proto - Add experiment version info in the Pubsub message
api/schema.yaml - Add experiment version info in the Experiment structure, SelectedTreatmentMetadata into the treatment response structure

Management Service

management-service/database/db-migrations/000003_experiment_version.*.sql - Migrations for the version column in the experiments table. The experiment_history table already has this field.
management-service/models/experiment.go - Include the version info when converting the DB model to other formats
management-service/services/experiment_history_service.go - When creating the history record, we can now just copy the version from the current experiment record.

Treatment Service

treatment-service/controller/treatment.go - Return additional experiment metadata in the final response when an experiment is matched
treatment-service/services/treatment_service.go - Return the switchback window Id (where applicable) from the GetTreatment method
treatment-service/models/typeconverter.go - Cater to the newly added version field in the experiment data

UI

ui/src/components/version_badge/VersionBadge.js - Add new component for use in experiment / experiment history details view
ui/src/experiments/details/ExperimentDetailsView.js, ui/src/experiments/history/details/ExperimentHistoryDetailsView.js - Include version info as a badge

Miscellaneous Bugfix

A recent PR (#33) refactored the segmenter card layout in the Edit Project Settings form. This caused a small regression - when a segmenter has multiple options for variable mappings, it should be expanded by default, in the Selected Segmenters column. If not, when a new card is dragged to the Selected Segmenters column, there is no visual feedback to the users that some info must be filled (the form submit will be blocked by the yup validation but we can't see where the error is either). This is addressed by adding back the initialIsOpen prop in ui/src/settings/components/form/components/segmenter_section/SegmenterCard.js.

…t response schema

…sion

krithika369 · 2022-09-16T07:39:13Z

Makefile

-	cd clients/management/ && mockery --name=ClientInterface --output=../../clients/testutils/mocks --filename=ManagementClientInterface.go
-	cd clients/treatment/ && mockery --name=TreatmentClientInterface --output=../../clients/testutils/mocks --filename=TreatmentClientInterface.go
+	cd clients/management/ && mockery --name=ClientInterface --output=../../clients/testutils/mocks/management --filename=ManagementClientInterface.go
+	cd clients/treatment/ && mockery --name=ClientInterface --output=../../clients/testutils/mocks/treatment --filename=TreatmentClientInterface.go


Renaming the interface for consistency and moving both to nested packages.

krithika369 · 2022-09-16T07:43:35Z

management-service/database/db-migrations/000003_experiment_version.up.sql

@@ -0,0 +1,9 @@
+ALTER TABLE experiments ADD version integer NOT NULL DEFAULT 1;


The default value automatically takes care of those experiments that have no history.

terryyylim

Overall LGTM, left some small comments. Thanks @krithika369 ! 🚀

terryyylim · 2022-09-20T07:10:50Z

api/schema.yaml

@@ -21,6 +21,23 @@ components:
        configuration:
          type: object
          description: Custom configuration associated with the given treatment
+    SelectedTreatmentMetadata:


Since the parent schema is already prefixed with SelectedTreatment, should we keep this concise with Metadata?

That might not be advisable, since the schema definitions are shared. The field inside SelectedTreatment is just called metadata though.

terryyylim · 2022-09-20T07:16:09Z

management-service/database/db-migrations/000003_experiment_version.up.sql

+    SELECT t1.id AS id, count(*) AS num_history
+    FROM experiments t1 INNER JOIN experiment_history t2 ON t1.id = t2.experiment_id
+    GROUP BY t1.id
+) UPDATE experiments SET version = num_history+1


nit: I think it's neater to start UPDATE on a newline.

terryyylim · 2022-09-20T07:16:56Z

management-service/controller/experiment_controller_test.go

@@ -81,7 +81,8 @@ func (s *ExperimentControllerTestSuite) SetupSuite() {
 			"type": "",
 			"start_time": "0001-01-01T00:00:00Z",
 			"updated_at": "0001-01-01T00:00:00Z",
-			"updated_by": ""
+			"updated_by": "",
+			"version": 0


nit: Since we expect versions to start from 1, shall we use 1 instead for these tests?

I intentionally left that field (and previously other properties too) empty - they don't play a role in the test cases. So they are empty values by default.

terryyylim · 2022-09-20T07:19:58Z

plugins/turing/runner/experiment_runner_test.go

+								},
+								"metadata": {
+									"experiment_version": 1,
+									"experiment_type": "a/b"


I think the expected value should be "A/B" right? https://github.com/caraml-dev/xp/blob/main/api/schema.yaml#L368

It's only a mocked test, but I updated it to match the treatment service response anyway. Thanks!

terryyylim · 2022-09-20T07:33:28Z

ui/src/experiments/details/ExperimentDetailsView.js

@@ -105,18 +119,19 @@ const ExperimentDetailsView = ({ projectId, experimentId, ...props }) => {
                </ExperimentActions>
              </EuiPageTemplate.Header>
            </Fragment>
-          )}
+          )
+          }


Formatting looks funny here 🤔

Something was broken with my local hooks, fixed it now.

terryyylim · 2022-09-20T07:33:54Z

ui/src/experiments/details/ExperimentDetailsView.js


          <Router primary={false}>
            <Redirect from="/" to="details" noThrow />
            <ExperimentConfigView path="details" experiment={data.data} />
            <ListExperimentHistoryView path="history" experiment={data.data} />
            <EditExperimentView path="edit" experimentSpec={data.data} />
          </Router>
-        </Fragment>
+        </Fragment >


Weird spaces at the back here.

krithika369 · 2022-09-20T10:07:17Z

Thanks for the review @terryyylim! Will merge this once the CI passes.

Krithika Sundararajan added 3 commits September 16, 2022 00:19

Add switchback_window_id and experiment_version to the fetch treatmen…

a9da254

…t response schema

Add version field to experiments table

38bfade

Treatment service changes for switchback window id and experiment ver…

075e6ce

…sion

krithika369 self-assigned this Sep 16, 2022

krithika369 marked this pull request as draft September 16, 2022 07:27

Fix plugin test case

79f37b4

krithika369 commented Sep 16, 2022

View reviewed changes

Fix file formatting

b664319

krithika369 marked this pull request as ready for review September 16, 2022 07:58

krithika369 requested a review from a team September 16, 2022 07:58

krithika369 changed the title ~~[WIP] Add additional experiment metadata to Fetch Treatment response~~ Add additional experiment metadata to Fetch Treatment response Sep 16, 2022

terryyylim approved these changes Sep 20, 2022

View reviewed changes

Krithika Sundararajan added 2 commits September 20, 2022 17:59

Address PR comments

e8cb0ed

Address PR comments - UI formatting

e176f67

krithika369 merged commit 5483d6b into caraml-dev:main Sep 20, 2022

krithika369 deleted the switchback_window_id branch September 20, 2022 10:08

krithika369 mentioned this pull request Sep 28, 2022

Add treatment metadata to BQ / Kafka logs #41

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional experiment metadata to Fetch Treatment response #36

Add additional experiment metadata to Fetch Treatment response #36

krithika369 commented Sep 16, 2022 •

edited

Loading

krithika369 Sep 16, 2022

krithika369 Sep 16, 2022

terryyylim left a comment

terryyylim Sep 20, 2022

krithika369 Sep 20, 2022 •

edited

Loading

terryyylim Sep 20, 2022

terryyylim Sep 20, 2022

krithika369 Sep 20, 2022

terryyylim Sep 20, 2022

krithika369 Sep 20, 2022

terryyylim Sep 20, 2022

krithika369 Sep 20, 2022

terryyylim Sep 20, 2022

krithika369 commented Sep 20, 2022

		@@ -0,0 +1,9 @@
		ALTER TABLE experiments ADD version integer NOT NULL DEFAULT 1;

Add additional experiment metadata to Fetch Treatment response #36

Add additional experiment metadata to Fetch Treatment response #36

Conversation

krithika369 commented Sep 16, 2022 • edited Loading

Summary of Changes

Schema

Management Service

Treatment Service

UI

Miscellaneous Bugfix

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryyylim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krithika369 Sep 20, 2022 • edited Loading

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

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

krithika369 commented Sep 20, 2022

krithika369 commented Sep 16, 2022 •

edited

Loading

krithika369 Sep 20, 2022 •

edited

Loading