Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add validation and new upi logger type #319

Merged
merged 28 commits into from
Mar 2, 2023
Merged

add validation and new upi logger type #319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
f3ad06c
add validation and new upi logger type
leonlnj Feb 16, 2023
3f40e17
add exp info to upi response
leonlnj Feb 20, 2023
de288b5
populate empty metadata before experiment response
leonlnj Feb 20, 2023
7ebee9c
patch e2e and upi exp implementation
leonlnj Feb 21, 2023
c452472
patch upi e2e request
leonlnj Feb 21, 2023
f658ac1
update manifest for prop engine
leonlnj Feb 21, 2023
3237633
patch sdk e2e
leonlnj Feb 21, 2023
3a29175
patch sdk e2e
leonlnj Feb 21, 2023
239d706
patch e2e
leonlnj Feb 21, 2023
0860d93
patch sdk e2e
leonlnj Feb 21, 2023
3d38195
add upi validation and upi logger to sdk
leonlnj Feb 22, 2023
b3b169b
shift upi logger flag as standalone to faciliate future more fine gra…
leonlnj Feb 23, 2023
f4e64df
refactor result_logger and kafka logger to be bit more generic
leonlnj Feb 23, 2023
044ef83
update api server go mod
leonlnj Feb 23, 2023
0a0bc08
update experiment engine go mod
leonlnj Feb 23, 2023
4905342
update go mod for experiment example
leonlnj Feb 23, 2023
7cc5eee
add regex and get router details from app name
leonlnj Feb 23, 2023
455102c
add implementation for router_log creation, test still need some patc…
leonlnj Feb 24, 2023
6cef240
refactor upi log test
leonlnj Feb 26, 2023
659eef1
patch mock interface for kafka test
leonlnj Feb 26, 2023
3a8a66f
move logger implementation outside upi init
leonlnj Feb 26, 2023
6f347de
patch test using monkey patch
leonlnj Feb 26, 2023
910b20b
bugfix kafka test
leonlnj Feb 26, 2023
f46ab8a
add write upi logs to nop logger
leonlnj Feb 27, 2023
06d402e
address mr comment and create new upi_kafka logger
leonlnj Feb 28, 2023
06b26b3
remoove upi kafka
leonlnj Feb 28, 2023
41096b1
Merge branch 'caraml-dev:main' into upi_router_logs
leonlnj Mar 1, 2023
85f380e
refactor upilogger, create upi kafka and nop, add validation for http…
leonlnj Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions api/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,15 @@ version:
.PHONY: build-run-local-api
build-run-local-api:
@echo "Building proprietary experiment engine plugin..."
cd ../engines/experiment/examples/plugins/hardcoded && make proprietary-exp-plugin
cd ../engines/experiment/examples/plugins/hardcoded && make proprietary-exp-plugin GOOS=darwin
@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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@echo "Starting Turing API server in a background process..."
nohup go run turing/cmd/main.go -config=config-dev.yaml -config=config-dev-exp-engine.yaml &
nohup go run turing/cmd/main.go -config=config-dev-w-creds.yaml -config=config-dev-exp-engine.yaml &
@echo "Started Turing API server on port 8080 in a background process..."

.PHONY: clean-local-infra
Expand Down
9 changes: 8 additions & 1 deletion api/api/openapi.bundle.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1982,6 +1982,7 @@ components:
timeout: timeout
updated_at: 2000-01-23T04:56:07.000+00:00
standard_config:
lazy_routing: true
route_name_path: route_name_path
experiment_mappings:
- treatment: treatment-1
Expand Down Expand Up @@ -2154,7 +2155,7 @@ components:
default: nop
enum:
- nop
- console
- upi
- bigquery
- kafka
type: string
Expand Down Expand Up @@ -2288,6 +2289,7 @@ components:
timeout: timeout
updated_at: 2000-01-23T04:56:07.000+00:00
standard_config:
lazy_routing: true
route_name_path: route_name_path
experiment_mappings:
- treatment: treatment-1
Expand Down Expand Up @@ -2349,6 +2351,7 @@ components:
EnsemblerStandardConfig:
description: ensembler config when ensembler type is standard
example:
lazy_routing: true
route_name_path: route_name_path
experiment_mappings:
- treatment: treatment-1
Expand All @@ -2367,6 +2370,8 @@ components:
route_name_path:
nullable: true
type: string
lazy_routing:
type: boolean
type: object
EnsemblerDockerConfig:
description: ensembler config when ensembler type is docker
Expand Down Expand Up @@ -2645,6 +2650,7 @@ components:
timeout: timeout
updated_at: 2000-01-23T04:56:07.000+00:00
standard_config:
lazy_routing: true
route_name_path: route_name_path
experiment_mappings:
- treatment: treatment-1
Expand Down Expand Up @@ -2803,6 +2809,7 @@ components:
timeout: timeout
updated_at: 2000-01-23T04:56:07.000+00:00
standard_config:
lazy_routing: true
route_name_path: route_name_path
experiment_mappings:
- treatment: treatment-1
Expand Down
2 changes: 1 addition & 1 deletion api/api/specs/routers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,7 @@ components:
type: "string"
enum:
- "nop"
- "console"
- "upi"
- "bigquery"
- "kafka"
default: "nop"
Expand Down
16 changes: 9 additions & 7 deletions api/config-dev-exp-engine.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
RouterDefaults:
ExperimentEnginePlugins:
proprietary:
Image: localhost:5000/proprietary-experiment-engine-plugin:latest
PluginConfig:
Image: localhost:5000/proprietary-experiment-engine-plugin:latest
LivenessPeriodSeconds: 10
Experiment:
proprietary:
plugin_binary: ./bin/example-plugin
Expand All @@ -19,15 +21,15 @@ Experiment:
name: exp_1
variants:
- name: control
- name: treatment-1
- name: treatment-a
variants_configuration:
control:
traffic: 0.85
treatment_configuration:
foo: bar
route_name: treatment-a
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

treatment-1:
foo: foo
route_name: control
treatment-a:
traffic: 0.15
treatment_configuration:
bar: baz
route_name: control
bar: bar
route_name: treatment-a
14 changes: 14 additions & 0 deletions api/e2e/test/config-local.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,20 @@ mockTreatmentUPIServer:
cluster:
name: dev
credentials: {}
# Example of config
# credentials: {
# "k8s_config": {
# "name": "default",
# "cluster": {
# "certificate-authority-data": "...",
# "server": "https://127.0.0.1:6443"
# },
# "user": {
# "client-certificate-data": "...",
# "client-key-data": "...="
# }
# }
# }

project:
id: 1
Expand Down
18 changes: 9 additions & 9 deletions api/e2e/test/router_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,24 +31,24 @@ var _ = DeployedRouterContext("testdata/create_router_std_ensembler_proprietary_
Context("POST /v1/predict", func() {

When("called with {client: {id: 4}} in the payload", func() {
It("responds with data from the `treatment-a` route", func() {
It("responds with data from the `control` route", func() {
routerE.POST("/v1/predict").
WithHeaders(defaultPredictHeaders).
WithJSON(json.RawMessage(`{"client": {"id": 4}}`)).
Expect().
Status(http.StatusOK).
JSON().Equal(json.RawMessage(`{"version": "treatment-a"}`))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

JSON().Equal(json.RawMessage(`{"version": "control"}`))
})
})

When("called with {client: {id: 7}} in the payload", func() {
It("responds with data from the `control` route", func() {
It("responds with data from the `treatment-a` route", func() {
routerE.POST("/v1/predict").
WithHeaders(defaultPredictHeaders).
WithJSON(json.RawMessage(`{"client": {"id": 7}}`)).
Expect().
Status(http.StatusOK).
JSON().Equal(json.RawMessage(`{"version": "control"}`))
JSON().Equal(json.RawMessage(`{"version": "treatment-a"}`))
})
})
})
Expand All @@ -68,7 +68,7 @@ var _ = DeployedRouterContext("testdata/create_router_with_traffic_rules.json.tm
})

When("request satisfies the first traffic rule", func() {
It("responds with responses from `control` and `treatment-a` routes", func() {
It("responds with responses from `treatment-a` route", func() {
want = httpexpect.
NewValue(GinkgoT(), JSONPayload("testdata/responses/traffic_rules/traffic-rule-1.json")).
Object()
Expand All @@ -85,7 +85,7 @@ var _ = DeployedRouterContext("testdata/create_router_with_traffic_rules.json.tm
})

When("request satisfies the second traffic rule", func() {
It("responds with responses from `control` and `treatment-b` routes", func() {
It("responds with responses from `treatment-b` route", func() {
want = httpexpect.
NewValue(GinkgoT(), JSONPayload("testdata/responses/traffic_rules/traffic-rule-2.json")).
Object()
Expand All @@ -101,7 +101,7 @@ var _ = DeployedRouterContext("testdata/create_router_with_traffic_rules.json.tm
})

When("request satisfies no traffic rules", func() {
It("responds with responses from `control` route only", func() {
It("responds with responses from `control` route", func() {
want = httpexpect.
NewValue(GinkgoT(), JSONPayload("testdata/responses/traffic_rules/no-rules.json")).
Object()
Expand Down Expand Up @@ -209,7 +209,7 @@ var _ = DeployedRouterContext("testdata/create_router_upi_with_std_ensembler.jso
{
Name: "client_id",
Type: upiv1.Type_TYPE_STRING,
StringValue: "4",
StringValue: "7",
},
},
}
Expand Down Expand Up @@ -260,7 +260,7 @@ var _ = DeployedRouterContext("testdata/create_router_upi_with_traffic_rules.jso
{
Name: "client_id",
Type: upiv1.Type_TYPE_STRING,
StringValue: "1",
StringValue: "7",
},
},
}
Expand Down
3 changes: 1 addition & 2 deletions api/e2e/test/routers_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


type TestData struct {
config.Config
Expand All @@ -44,8 +45,6 @@ func init() {
flag.StringVar(&configFile, "config", "config.yaml", "Path to a configuration file")
}

var istioVirtualServiceIntervals = []interface{}{"30s", "1s"}

var defaultPredictHeaders = map[string]string{
"X-Mirror-Body": "true",
}
Expand Down
2 changes: 1 addition & 1 deletion api/e2e/test/testdata/create_router_upi_with_traffic_rules.json.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
"field_source": "prediction_context",
"field": "client_id",
"operator": "in",
"values": ["1"]
"values": ["7"]
}
],
"routes": ["treatment-a", "control"]
Expand Down
16 changes: 10 additions & 6 deletions api/e2e/test/testdata/responses/router_proprietary_exp_batch_predict.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@
"response": {
"experiment": {
"configuration": {
"foo": "bar",
"route_name": "treatment-a"
}
"foo": "foo",
"route_name": "control"
},
"experiment_name": "exp_1",
"name": "control"
},
"route_responses": [
{
Expand All @@ -37,9 +39,11 @@
"response": {
"experiment": {
"configuration": {
"bar": "baz",
"route_name": "control"
}
"bar": "bar",
"route_name": "treatment-a"
},
"experiment_name": "exp_1",
"name": "treatment-a"
},
"route_responses": [
{
Expand Down
8 changes: 5 additions & 3 deletions api/e2e/test/testdata/responses/router_proprietary_exp_predict.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@
"response": {
"experiment": {
"configuration": {
"foo": "bar",
"route_name": "treatment-a"
}
"foo": "foo",
"route_name": "control"
},
"experiment_name": "exp_1",
"name": "control"
},
"route_responses": [
{
Expand Down
29 changes: 15 additions & 14 deletions api/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ require (
github.com/antihax/optional v1.0.0
github.com/caraml-dev/turing/engines/experiment v0.0.0
github.com/caraml-dev/turing/engines/router v0.0.0
github.com/caraml-dev/universal-prediction-interface v0.0.0-20221026045401-50e7d79e4b73
github.com/caraml-dev/universal-prediction-interface v0.3.4
github.com/gavv/httpexpect/v2 v2.3.1
github.com/getkin/kin-openapi v0.76.0
github.com/ghodss/yaml v1.0.0
Expand All @@ -33,11 +33,11 @@ require (
github.com/pkg/errors v0.9.1
github.com/rs/cors v1.8.2
github.com/spf13/viper v1.9.0
github.com/stretchr/testify v1.8.0
github.com/stretchr/testify v1.8.1
github.com/xanzy/go-gitlab v0.32.0
go.uber.org/zap v1.21.0
golang.org/x/oauth2 v0.0.0-20220722155238-128564f6959c
google.golang.org/grpc v1.50.1
golang.org/x/oauth2 v0.0.0-20221014153046-6fdb5e3db783
google.golang.org/grpc v1.51.0
google.golang.org/protobuf v1.28.1
gopkg.in/yaml.v2 v2.4.0
gopkg.in/yaml.v3 v3.0.1
Expand All @@ -56,7 +56,8 @@ require (
)

require (
cloud.google.com/go/compute v1.7.0 // indirect
cloud.google.com/go/compute v1.14.0 // indirect
cloud.google.com/go/compute/metadata v0.2.3 // indirect
github.com/ajg/form v1.5.1 // indirect
github.com/andybalholm/brotli v1.0.3 // indirect
github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a // indirect
Expand Down Expand Up @@ -152,11 +153,11 @@ require (
github.com/prometheus/procfs v0.6.0 // indirect
github.com/sergi/go-diff v1.1.0 // indirect
github.com/sirupsen/logrus v1.9.0 // indirect
github.com/spf13/afero v1.6.0 // indirect
github.com/spf13/afero v1.9.2 // indirect
github.com/spf13/cast v1.4.1 // indirect
github.com/spf13/jwalterweatherman v1.1.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
github.com/stretchr/objx v0.4.0 // indirect
github.com/stretchr/objx v0.5.0 // indirect
github.com/subosito/gotenv v1.2.0 // indirect
github.com/tidwall/pretty v1.0.2 // indirect
github.com/uber/jaeger-client-go v2.25.0+incompatible // indirect
Expand All @@ -173,16 +174,16 @@ require (
go.mongodb.org/mongo-driver v1.1.2 // indirect
go.uber.org/atomic v1.9.0 // indirect
go.uber.org/multierr v1.8.0 // indirect
golang.org/x/crypto v0.1.0 // indirect
golang.org/x/net v0.3.1-0.20221206200815-1e63c2f08a10 // indirect
golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4 // indirect
golang.org/x/sys v0.3.0 // indirect
golang.org/x/term v0.3.0 // indirect
golang.org/x/text v0.5.0 // indirect
golang.org/x/crypto v0.5.0 // indirect
golang.org/x/net v0.5.0 // indirect
golang.org/x/sync v0.1.0 // indirect
golang.org/x/sys v0.4.0 // indirect
golang.org/x/term v0.4.0 // indirect
golang.org/x/text v0.6.0 // indirect
golang.org/x/time v0.0.0-20220210224613-90d013bbcef8 // indirect
gomodules.xyz/jsonpatch/v2 v2.2.0 // indirect
google.golang.org/appengine v1.6.7 // indirect
google.golang.org/genproto v0.0.0-20220822174746-9e6da59bd2fc // indirect
google.golang.org/genproto v0.0.0-20230104163317-caabf589fcbf // indirect
gopkg.in/errgo.v2 v2.1.0 // indirect
gopkg.in/inf.v0 v0.9.1 // indirect
gopkg.in/ini.v1 v1.63.2 // indirect
Expand Down
Loading