add validation and new upi logger type #319

leonlnj · 2023-02-16T12:06:39Z

Description

This PR aims at

Adding some validation to UPI routers for invalid component configuration
Bugfix existing experiment response that doesn't include experiment and treatment name defined in runner interface
New ResultLogger=UPI and removed ConsoleLogger from API schema
- Adding RouterLog publishing to UPI router (existing implementation to BQ/Kafka is removed)
  - Currently UPI Router will only publish to Kafka

Task

API Server
Router
SDK
UI is not scoped in this PR

Key Modification

API Schema

api/api/specs/routers.yaml - added UPI logger type and removed console

Validation

api/turing/validation/validator.go - Standard ensembler and UPI logger validation for UPI Router

API Server

api/turing/cluster/servicebuilder/router.go - construction of env var for Router. Logic to which kafka topic is predefined here.

Patching Experiment response

engines/router/missionctl/experiment/experiment.go - where the experiment and treatment name was never set before

Patching e2e

api/Makefile - fix cli to include env creds
api/config-dev-exp-engine.yaml - patch config to incorporate recent plugin changes + fix treatment config
api/e2e/test/config-local.yaml - add description of expected payload for cred config
api/e2e/test/router_test.go - patching to fit the correct treatment name and route name

UPI Logger

engines/router/missionctl/log/resultlog/upi_result_log.go - new UPIResultLogger struct and UPILogger interface. Kakfa and nop logger added new method to satisfy UPILogger interface. This is also the main logic on how RouterLog is constructed and the test validate that the fields are set in expected manner.

engines/router/missionctl/log/resultlog/kafka.go - other than adding new method for UPILogger interface, existing method is moved to writeToKafka which takes in a generic proto, how the message is published remain unchanged. Both write (for http) and WriteUPIRouterLog (for upi) call the same underlying method.

engines/router/missionctl/log/resultlog/resultlog.go - one of the key changes is to redefined TuringResultLogEntry (its weird how it was defined to be turing.TuringResultLogMessage and then added pointer receiver). The proto is added as field and passed as it is when required.

type TuringResultLogEntry turing.TuringResultLogMessage
type TuringResultLogEntry struct {
	resultLogMessage turing.TuringResultLogMessage
}

The previous defined, will not recognize the type as a proto, probably due to type tracing of custom-type (Syntax: Type Definitions and Concept: Underlying Types)

The original proto can assigned to a proto.message but not TuringResultLogEntry

Because of this, the type was never recognized as a proto, and to accomodate this change, json.marshal is modified to protojson marshaller. The output are intact (as per unit test), only the marshaller is required to change to produce the same output.

engines/router/missionctl/server/application.go- Construct the logger and pass it to UPILogger.

engines/router/missionctl/server/upi/server.go - Note that UPILogger has its own ResultLogger and does not refer to the global module as per HTTP. The logger is injected and the resultlogger is part of the missionControl, this structural change is to allow dependency injection of logger implementation (no need for monkey patch) and it allows the removal of logUtils and global reference of ResultLog module (currently depending on it without being explicitly anywhere). Result logging is using the ResultLogger injected now. Added implementation to get experiment response using fiber context

engines/router/missionctl/server/upi/logutils.go - deleted and beloing to part of engines/router/missionctl/log/resultlog/upi_result_log.go, in the rightful log module.

SDK

Validation and UPI Logger init

…in control

…hing

leonlnj · 2023-02-27T06:00:40Z

api/config-dev-exp-engine.yaml

        variants_configuration:
          control:
            traffic: 0.85
            treatment_configuration:
-              foo: bar
-              route_name: treatment-a


previously the treatment is control but it return a route_name of treatment-a, vice versa with treatment-1.

Minor refactoring to fix that and simplify the config

leonlnj · 2023-02-27T06:02:17Z

api/e2e/test/routers_suite_test.go

@@ -34,6 +34,7 @@ var cfg config.Config
 var defaultDeploymentIntervals = []interface{}{"10m", "5s"}
 var defaultDeletionIntervals = []interface{}{"20s", "2s"}
 var arbitraryUpdateIntervals = []interface{}{"10s", "1s"}
+var istioVirtualServiceIntervals = []interface{}{"60s", "5s"}


increased and shifted, as 30s is too short for local setup and 60s works better

leonlnj · 2023-02-27T06:10:04Z

api/e2e/test/router_test.go

 							routerE.POST("/v1/predict").
 								WithHeaders(defaultPredictHeaders).
 								WithJSON(json.RawMessage(`{"client": {"id": 4}}`)).
 								Expect().
 								Status(http.StatusOK).
-								JSON().Equal(json.RawMessage(`{"version": "treatment-a"}`))


this was actually flawed previously. "4" calls treatment "control" which has route_name=treatment-a

Since the exp config is patched to pair route and treatment consistently, id=4 will call control and return control

leonlnj · 2023-02-27T06:15:32Z

api/turing/validation/validator_test.go

@@ -22,6 +23,15 @@ import (
 	routerConfig "github.com/caraml-dev/turing/engines/router/missionctl/config"
 )

+var dummyConfig json.RawMessage
+
+func getDefaultValidator() (*validator.Validate, error) {


extracting common mock construction for re-use across whole test

leonlnj · 2023-02-27T06:16:58Z

api/turing/validation/validator_test.go

 }

-func (tt routerConfigTestCase) RouterConfig(protocol routerConfig.Protocol) *request.RouterConfig {


no idea why protocol is in method parameter (written by me 😅 previously i think), removed and refer to input args instead for consistency

leonlnj · 2023-02-27T06:18:19Z

engines/router/missionctl/config/config.go

@@ -36,6 +36,8 @@ const (
 	ConsoleLogger ResultLogger = "CONSOLE"


note: console logger is only removed from API, the implementation isn't remove as it can be potentially useful for debugging

leonlnj · 2023-02-27T06:19:32Z

engines/router/missionctl/experiment/experiment.go

@@ -92,6 +94,8 @@ func NewResponse(expPlan *runner.Treatment, expPlanErr error) *Response {
 		experimentResponse.Error = expPlanErr.Error()
 	} else {
 		experimentResponse.Configuration = expPlan.Config


treatment and experiment name was never returned prior

For context on why these fields were not being logged previously - the experiment config returned by xp and other internal engines typically also included the experiment name and treatment name in the config. So we didn't care too much about logging these values explicitly (rather, these fields were only used for things like metrics). But I agree it makes sense to include them in the logs (especially since we're not supposed to make assumptions about what the config includes and what it doesn't).

(Including these fields in func (r *Response) Body() { ... would've been a breaking change for the users though - glad we don't see the need to do that yet.)

leonlnj · 2023-02-27T06:40:41Z

engines/router/missionctl/server/upi/server.go

 	}
 }

 func (us *Server) Run(listener net.Listener) {
 	s := grpc.NewServer()
-	//TODO: the unmarshalling can be done more efficiently by using partial deserialization


initially optimization could had been done to speed up calls, however it would be required for logging purposes, hence there is no more need to explore this

leonlnj · 2023-02-27T06:43:27Z

sdk/e2e/09_deploy_router_with_std_ensembler_test.py

@@ -116,5 +116,5 @@ def test_deploy_router_with_std_ensembler():
        json={"client": {"id": 4}},
    )
    assert response.status_code == 200
-    expected_response = {"version": "treatment-a"}
+    expected_response = {"version": "control"}


patching due to correction of the experiment engine config

leonlnj · 2023-02-27T06:45:33Z

sdk/e2e/11_deploy_router_upi_traffic_split_test.py

@@ -153,7 +155,7 @@ def test_deploy_router_upi_traffic_split():
            variable_pb2.Variable(
                name="client_id",
                type=type_pb2.TYPE_STRING,
-                string_value="1",


although named treatment this was actually control, if traffic rules malfunction and a control via default route, would had pass this anyway

leonlnj · 2023-02-27T06:46:26Z

sdk/e2e/11_deploy_router_upi_traffic_split_test.py

@@ -64,7 +64,7 @@ def test_deploy_router_upi_traffic_split():
                    field_source=FieldSource.PREDICTION_CONTEXT,
                    field="client_id",
                    operator="in",
-                    values=["1"],
+                    values=["7"],


1 would return control, which is the default route and doesn't test traffic rules

leonlnj · 2023-02-27T06:47:56Z

sdk/e2e/11_deploy_router_upi_traffic_split_test.py

@@ -175,7 +177,7 @@ def test_deploy_router_upi_traffic_split():
            variable_pb2.Variable(
                name="client_id",
                type=type_pb2.TYPE_STRING,
-                string_value="1234",
+                string_value="12",


both 7 and 12 returns treatment. Hence how traffic rules is tested, is that two request both should get treatment, but the one not fulfilling traffic rule will get control, which is 12. 1234 previously returns treatment which calls control route

leonlnj · 2023-02-27T06:49:42Z

sdk/e2e/12_deploy_router_upi_std_ensembler_test.py

@@ -126,8 +126,9 @@ def test_deploy_router_upi_std_ensembler():
    channel = grpc.insecure_channel(retrieved_router.endpoint)
    stub = upi_pb2_grpc.UniversalPredictionServiceStub(channel)

-    logging.info("send request that satisfy treatment-a")
+    logging.info("send request that satisfy control")


other than the traffic rules splt which has some flaw and the others are merely adapting to the new corrected config

leonlnj · 2023-02-27T06:51:10Z

sdk/turing/router/config/router_version.py

@@ -47,8 +47,9 @@ def __init__(
        self.status = RouterStatus(status)
        self.name = name
        self.monitoring_url = monitoring_url
-        self.log_config = RouterVersionLogConfig(**kwargs.get("log_config"))


because LogConfig validates against the Protocol, the RouterConfig has to be initiated first

pradithya

Thanks @leonlnj I left some comments below

engines/router/missionctl/log/resultlog/upi_result_log.go

engines/router/missionctl/server/upi/server.go

pradithya · 2023-02-27T07:50:42Z

sdk/e2e/12_deploy_router_upi_std_ensembler_test.py

@@ -168,4 +169,4 @@ def test_deploy_router_upi_std_ensembler():
    logging.info(f"received response {response}")

    assert response.prediction_result_table == request.prediction_table
-    assert response.metadata.models[0].name == "control"


Can you add assertion in this test and test_deploy_router_upi_traffic_split that validate experiment_name and treatment_name are properly populated

this is not possible with the current proprietary engine, it doesn't return those fields.

pradithya · 2023-02-27T08:23:36Z

engines/router/missionctl/log/resultlog/kafka.go

+	)
+}
+
+func (l *KafkaLogger) WriteUPIRouterLog(routerLog *upiv1.RouterLog) error {


It feels a bit out of place. Can we instead have a separate KafkaLogger (e.g. UPIKafkaLogger) and embed the original KafkaLogger within it and override the write method?

hmm the challenge with this, is that a UPI wrapper for each logger needs to be created, including the nop logger, since write is redeclared twice as the parameter is different. I can change the interface to a proto.message but all the loggers for HTTP need some refactor and type assert back to TuringResultLog. I prefer to do this is another MR if necessary.

// TuringResultLogger is an abstraction for the underlying result logger type TuringResultLogger interface { write(*TuringResultLogEntry) error }

is that a UPI wrapper for each logger needs to be created, including the nop logger

I don't think this is needed. Since the logging orchestrator (UPIResultLogger) in itself in a separate implementation, the logger in the below initialization need not adhere to the TuringResultLogger interface anymore, right? That logger can be the UPIKafkaLogger which implements only the UPILogger interface, but makes use of the existing Kafka logger under the hood to write the given messge.

resultLogger, err := resultlog.InitUPIResultLogger(cfg.AppConfig.Name, logger)

yep UPIKafkaLogger is fine, but I would need UPINopLogger else there be this clash. Will just add it anyway 😅

// write is a nop method that satisfies the NopLogger interface func (*NopLogger) write(turLogEntry *TuringResultLogEntry) error { return nil } // write is a nop method that satisfies the UPILogger interface func (*NopLogger) write(routerLog *upiv1.RouterLog) error { return nil }

pradithya

LGTM!

krithika369

LGTM. Thanks, @leonlnj ! Left some small comments.

krithika369 · 2023-03-01T22:07:08Z

api/Makefile

 	@echo "Building proprietary experiment engine image..."
 	cd ../engines/experiment/examples/plugins/hardcoded && make build-local-proprietary-exp-plugin-image
 	@echo "Building router image..."
 	cd ../engines/router && make build-local-router-image DOCKER_REGISTRY=localhost:5000 OVERWRITE_VERSION=latest
+	@echo "Generating cluster cred"
+	sh ../infra/docker-compose/dev/extract_creds.sh


Thanks for adding this. Amidst the testing improvements and chart changes (move to the umbrella chart), one thing we could do is to let the e2e tests in the component repo stop depending on the chart at all and use this setup instead, which could catch these problems earlier. We'll add this to the list of improvements. CC: @deadlycoconuts

krithika369 · 2023-03-01T22:32:07Z

api/turing/config/config.go

+// UPIConfig captures the defaults used by UPI Router
+type UPIConfig struct {
+	// KafkaBrokers is broker which all Router will write to when UPI logging is enabled
+	KafkaBrokers string


Are we expecting this UPIConfig to hold more values eventually? Instead, should we just put this under the existing KafkaConfig ? For the HTTP_JSON routers, we don't have to use this value (since the broker info will be supplied by the users).

Hmm I saw kafkaConfig which contain MaxMessageBytes and CompressionType as defaults applied to all Kafka, the broker is applied specifically for UPI Routers only, so I didnt reuse it.

Are we expecting this UPIConfig to hold more values eventually?

Yes, its one of possible feature to enable finer-grain logging control, so we can reduce the amount of the logs if user doesn't want them.

api/turing/validation/validator.go

api/turing/validation/validator_test.go

krithika369 · 2023-03-01T23:35:13Z

engines/router/missionctl/experiment/experiment.go

@@ -92,6 +94,8 @@ func NewResponse(expPlan *runner.Treatment, expPlanErr error) *Response {
 		experimentResponse.Error = expPlanErr.Error()
 	} else {
 		experimentResponse.Configuration = expPlan.Config


For context on why these fields were not being logged previously - the experiment config returned by xp and other internal engines typically also included the experiment name and treatment name in the config. So we didn't care too much about logging these values explicitly (rather, these fields were only used for things like metrics). But I agree it makes sense to include them in the logs (especially since we're not supposed to make assumptions about what the config includes and what it doesn't).

(Including these fields in func (r *Response) Body() { ... would've been a breaking change for the users though - glad we don't see the need to do that yet.)

krithika369 · 2023-03-02T00:33:44Z

engines/router/missionctl/log/resultlog/kafka.go

+	)
+}
+
+func (l *KafkaLogger) WriteUPIRouterLog(routerLog *upiv1.RouterLog) error {


is that a UPI wrapper for each logger needs to be created, including the nop logger

I don't think this is needed. Since the logging orchestrator (UPIResultLogger) in itself in a separate implementation, the logger in the below initialization need not adhere to the TuringResultLogger interface anymore, right? That logger can be the UPIKafkaLogger which implements only the UPILogger interface, but makes use of the existing Kafka logger under the hood to write the given messge.

resultLogger, err := resultlog.InitUPIResultLogger(cfg.AppConfig.Name, logger)

… router

add validation and new upi logger type

f3ad06c

leonlnj self-assigned this Feb 17, 2023

leonlnj added 23 commits February 20, 2023 10:29

add exp info to upi response

3f40e17

populate empty metadata before experiment response

de288b5

patch e2e and upi exp implementation

7ebee9c

patch upi e2e request

c452472

update manifest for prop engine

f658ac1

patch sdk e2e

3237633

patch sdk e2e

3a29175

patch e2e

239d706

patch sdk e2e

0860d93

add upi validation and upi logger to sdk

3d38195

shift upi logger flag as standalone to faciliate future more fine gra…

b3b169b

…in control

refactor result_logger and kafka logger to be bit more generic

f4e64df

update api server go mod

044ef83

update experiment engine go mod

0a0bc08

update go mod for experiment example

4905342

add regex and get router details from app name

7cc5eee

add implementation for router_log creation, test still need some patc…

455102c

…hing

refactor upi log test

6cef240

patch mock interface for kafka test

659eef1

move logger implementation outside upi init

3a8a66f

patch test using monkey patch

6f347de

bugfix kafka test

910b20b

add write upi logs to nop logger

f46ab8a

leonlnj commented Feb 27, 2023

View reviewed changes

leonlnj requested review from pradithya, tiopramayudi and a team February 27, 2023 06:53

leonlnj marked this pull request as ready for review February 27, 2023 06:53

pradithya requested changes Feb 27, 2023

View reviewed changes

leonlnj and others added 3 commits February 28, 2023 16:46

address mr comment and create new upi_kafka logger

06d402e

remoove upi kafka

06b26b3

Merge branch 'caraml-dev:main' into upi_router_logs

41096b1

pradithya approved these changes Mar 1, 2023

View reviewed changes

krithika369 approved these changes Mar 2, 2023

View reviewed changes

refactor upilogger, create upi kafka and nop, add validation for http…

85f380e

… router

leonlnj merged commit 5df71f6 into caraml-dev:main Mar 2, 2023

leonlnj added the enhancement New feature or request label Mar 2, 2023

leonlnj mentioned this pull request Mar 13, 2023

Enable TuringResultLog on upi router and refactor global ResultLog #323

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add validation and new upi logger type #319

add validation and new upi logger type #319

leonlnj commented Feb 16, 2023 •

edited

Loading

leonlnj Feb 27, 2023

leonlnj Feb 27, 2023

leonlnj Feb 27, 2023

leonlnj Feb 27, 2023

leonlnj Feb 27, 2023

leonlnj Feb 27, 2023

leonlnj Feb 27, 2023

krithika369 Mar 1, 2023

leonlnj Feb 27, 2023

leonlnj Feb 27, 2023

leonlnj Feb 27, 2023

leonlnj Feb 27, 2023

leonlnj Feb 27, 2023

leonlnj Feb 27, 2023

leonlnj Feb 27, 2023

pradithya left a comment

pradithya Feb 27, 2023

leonlnj Feb 28, 2023

pradithya Feb 27, 2023

leonlnj Feb 28, 2023

krithika369 Mar 2, 2023

leonlnj Mar 2, 2023

pradithya left a comment

krithika369 left a comment

krithika369 Mar 1, 2023

krithika369 Mar 1, 2023

leonlnj Mar 2, 2023 •

edited

Loading

krithika369 Mar 1, 2023

krithika369 Mar 2, 2023

		}

		func (tt routerConfigTestCase) RouterConfig(protocol routerConfig.Protocol) *request.RouterConfig {

		@@ -36,6 +36,8 @@ const (
		ConsoleLogger ResultLogger = "CONSOLE"

add validation and new upi logger type #319

add validation and new upi logger type #319

Conversation

leonlnj commented Feb 16, 2023 • edited Loading

Description

Task

Key Modification

API Schema

Validation

API Server

Patching Experiment response

Patching e2e

UPI Logger

SDK

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

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

pradithya 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradithya left a comment

Choose a reason for hiding this comment

krithika369 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonlnj Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonlnj commented Feb 16, 2023 •

edited

Loading

leonlnj Mar 2, 2023 •

edited

Loading