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.

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

Refactor treatment service into plugin #52

Conversation

deadlycoconuts
Copy link
Contributor

@deadlycoconuts deadlycoconuts commented Nov 22, 2022

What this PR does / why we need it:
This PR is the second of a series of changes intended to offer the Treatment Service as a Turing plugin, in addition to it being deployable as a standalone service, which is the currently the only way to deploy it.

In PR #47, the plugin was refactored to be initialised with a number of additional configuration that the Treatment Service uses, though its functionality has not been modified yet. This PR introduces changes to the functionality of the plugin so that it no longer contacts the central Treatment Service (the standalone deployment) and instead initialises the necessary services that it requires to serve treatments itself (as if it were a separate Treatment Service itself).

Modifications Overview:
This PR heavily refactors the GetTreatmentForRequest interface method that's implemented by the experimentRunner struct. It shares very similar behaviour with (and is largely inspired by) the treatment controller method FetchTreatment of the central Treatment Service. In order to initialise the necessary services to assign treatments, the experiment runner is initialised with an app context, which is performed in the refactored NewExperimentRunner.

In addition to the main changes above, additional refactoring is done to the initialisation methods for the internal storage, BigQuery treatment logger and PubSub subscriber within the Treatment Service in order to allow the Google clients used in these services to be authenticated with a service account token stored in an alternate environment variable EXP_GOOGLE_APPLICATION_CREDENTIALS instead of the default GOOGLE_APPLICATION_CREDENTIALS variable (refer to caraml-dev/turing#287 for more details).

This is done so that when these services are initialised as part of a plugin, they would be able to utilise the alternate file path to locate the service account token as opposed to using the default environment variable, which currently indicates the location of the service account token of the Turing Router. Should the environment variable EXP_GOOGLE_APPLICATION_CREDENTIALS not be specified, these services would resort to reading the file path from GOOGLE_APPLICATION_CREDENTIALS - this is currently happening when these services get initialised as part of the central standalone Treatment Service.

Last but not least, some refactoring was done to make the retrieval of project settings in the init step of the Treatment Service's local storage simpler and more reader-friendly.

@deadlycoconuts deadlycoconuts self-assigned this Nov 22, 2022
@deadlycoconuts deadlycoconuts force-pushed the refactor_treatment_service_into_plugin branch 4 times, most recently from 9b28c4d to eb1ce19 Compare November 23, 2022 20:13
plugins/turing/Makefile Show resolved Hide resolved
@@ -46,16 +46,14 @@ type ExperimentManagerConfig struct {
type ExperimentRunnerConfig struct {
Endpoint string `json:"endpoint" validate:"required"`
ProjectID int `json:"project_id" validate:"required"`
Passkey string `json:"passkey" validate:"required"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the passkey as it is no longer required to validate the plugin's permissions to access the central treatment service (it no longer communicates with it).

Comment on lines -91 to -95
// Retrieve passkey using the API
project, err := em.GetProject(config.ProjectID)
if err != nil {
return json.RawMessage{}, fmt.Errorf(errorMsg, err.Error())
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this as there is no need to retrieve any passkeys.

Comment on lines -176 to -178
// Extract maxS2CellLevel and mixS2CellLevel from the segmenter configuration stored as a map[string]interface{}
segmenterConfig := make(map[string]interface{})
segmenterConfig["s2_ids"] = *treatmentServiceConfig.SegmenterConfig
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove this as it was incorrectly storing the segmenter config.

Timeout string `json:"timeout" validate:"required"`
RequestParameters []Variable `json:"request_parameters" validate:"required,dive"`
TreatmentServiceConfig *config.Config `json:"treatment_service_config" validate:"required,dive"`
}

type TreatmentServicePluginConfig struct {
Port int `json:"port" default:"8080"`
ProjectIds []string `json:"project_ids" default:""`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed ProjectIds as this information can be retrieved directly by the json config passed to GetExperimentRunnerConfig (the Turing API indirectly calls this when deploying a new Turing Router).

for _, param := range er.parameters {
if param.FieldSource == "none" || param.Field == "" {
// Parameter not configured
continue
}
val, err := request.GetValueFromRequest(reqHeader, body, request.FieldSource(param.FieldSource), param.Field)
val, err := request.GetValueFromHTTPRequest(reqHeader, body, request.FieldSource(param.FieldSource), param.Field)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this when I bumped up the dependency on github.com/caraml-dev/turing/engines/experiment.

subscribedProjectSettings, err = s.getProjectSettings(s.subscribedProjectIds)
} else {
subscribedProjectSettings, err = s.getAllProjects()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored this to simplify the project settings retrieval step.

@deadlycoconuts deadlycoconuts marked this pull request as ready for review November 23, 2022 20:39
Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

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

Left some small comments - the PR looks great, thanks for this and iteratively testing and resolving the issues. Please feel free to merge once you've reviewed / addressed the comments.

plugins/turing/Dockerfile Outdated Show resolved Hide resolved
plugins/turing/Makefile Show resolved Hide resolved
plugins/turing/runner/experiment_runner.go Show resolved Hide resolved
treatment-service/models/storage.go Outdated Show resolved Hide resolved
@deadlycoconuts deadlycoconuts force-pushed the refactor_treatment_service_into_plugin branch from eb1ce19 to 7ebb772 Compare December 6, 2022 18:26
@deadlycoconuts deadlycoconuts merged commit 4100ac4 into caraml-dev:main Dec 6, 2022
@deadlycoconuts deadlycoconuts deleted the refactor_treatment_service_into_plugin branch December 6, 2022 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants