-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add field selector to list experiments API #44
Add field selector to list experiments API #44
Conversation
8677027
to
e8f635c
Compare
@@ -578,6 +588,35 @@ func (svc *experimentService) filterExperimentStatusFriendly(query *gorm.DB, sta | |||
return query.Where(orPredicates) | |||
} | |||
|
|||
func (svc *experimentService) filterStartEndTimeValues(query *gorm.DB, params ListExperimentsParams) (*gorm.DB, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a helper method out of this because the linter was throwing out an excessive cyclomatic complexity warning for the ListExperiments
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
3ab3658
to
200917a
Compare
- start_time | ||
- end_time | ||
- updated_at | ||
- treatments | ||
Experiment: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the change in this PR, all fields from Experiment
should be considered to be not required, like we do for Treatment
.
@@ -172,7 +166,7 @@ func (svc *experimentService) ListExperiments( | |||
// Pagination | |||
var pagingResponse *pagination.Paging | |||
var count int64 | |||
err := pagination.ValidatePaginationParams(params.Page, params.PageSize) | |||
err = pagination.ValidatePaginationParams(params.Page, params.PageSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the behavior same as the list treatments api so that pagination is only applied if:
- One of the panigation props is set
- Fields is nil
This will allow bypassing pagination entirely if the Fields selector is used and for things like querying scheduled / active experiments in the standard ensembler UI for Turing, we can end up using a single API call.
Ref:
https://github.com/caraml-dev/xp/blob/main/management-service/services/treatment_service.go#L111
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my good catch! Thanks for the reminder; can't remember why I left this out 😵💫
@@ -578,6 +588,35 @@ func (svc *experimentService) filterExperimentStatusFriendly(query *gorm.DB, sta | |||
return query.Where(orPredicates) | |||
} | |||
|
|||
func (svc *experimentService) filterStartEndTimeValues(query *gorm.DB, params ListExperimentsParams) (*gorm.DB, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
6c50a04
to
4f5f1e0
Compare
@@ -811,5 +812,5 @@ func (suite *TreatmentServiceTestSuite) TestLocalStorage() { | |||
|
|||
resp, err := suite.managementServiceClient.GetExperimentWithResponse(suite.ctx, 1, 1) | |||
suite.Require().NoError(err) | |||
suite.Require().Equal(resp.JSON200.Data.Name, storage.Experiments[1][0].Experiment.Name) | |||
suite.Require().Equal(storage.Experiments[1][0].Experiment.Name, *resp.JSON200.Data.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reordered the 2 arguments because the expected argument is supposed to come before the actual one.
@@ -110,7 +117,7 @@ func (suite *LocalStorageLookupSuite) SetupTest() { | |||
Return(&http.Response{ | |||
StatusCode: 200, | |||
Header: map[string][]string{"Content-Type": {"json"}}, | |||
Body: ioutil.NopCloser(bytes.NewBufferString(`{"data" : []}`)), | |||
Body: io.NopCloser(bytes.NewBufferString(`{"data" : []}`)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced the deprecated method with its replacement.
4c3845e
to
4a4fea0
Compare
c8158d6
to
b7bccab
Compare
for k := range fieldNamesSet { | ||
fieldNames = append(fieldNames, k) | ||
} | ||
query = query.Select(fieldNames) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fieldFieldValues
function essentially specifies SELECT
values in the DB query such that only columns corresponding to the field values requested get retrieved.
if field == models.ExperimentFieldStatusFriendly { | ||
// Add ExperimentFieldStartTime and ExperimentFieldEndTime to the query as they are required for | ||
// determining the statusFriendly field; these two fields may or may not be returned depending on | ||
// what is actually specified in params.Fields | ||
fieldNamesSet[string(models.ExperimentFieldStartTime)] = struct{}{} | ||
fieldNamesSet[string(models.ExperimentFieldEndTime)] = struct{}{} | ||
|
||
fieldName = models.ExperimentFieldStatus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is added since status_friendly
does not exist in the DB as a column. In order to determine what this field value should be, we'll need to retrieve other columns, such as status
, start_time
and end_time
.
The fieldNamesSet
used to store column names ensure that only unique values are used in the query i.e. so we don't do something like SELECT start_time, status, start_time, ... FROM experiments
(in SQL)
// All experiments under a settings | ||
expResponsesList, pagingResponse, err := svc.ListExperiments(1, services.ListExperimentsParams{}) | ||
actualResponsesList, pagingResponse, err := svc.ListExperiments(1, services.ListExperimentsParams{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed this as it's clearly the actual response, and not the expected response.
…ontroller behaviour
There was a problem hiding this 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. LGTM. Thanks for this PR! Please feel free to merge once you've reviewed / addressed the comments.
} | ||
|
||
// Stores a map of unique field names which are unique column names to be selected in the db query | ||
fieldNamesSet := make(map[string]struct{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use "github.com/golang-collections/collections/set"
for this, which we commonly use elsewhere in the codebase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I tried doing that but I realised we can't extract the values contained within the Set
as a slice of strings because the underlying field (hash
) containing all the values added to the Set
is actually a private field:
type (
Set struct {
hash map[interface{}]nothing
}
nothing struct{}
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm there should be a function to get it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They seem to all return a *Set
at most 😕 https://pkg.go.dev/github.com/golang-collections/collections/set#pkg-index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative is to run Do
with a function that stores all the string values in the set into some slice.
ui/src/config.js
Outdated
@@ -36,6 +36,11 @@ export const appConfig = { | |||
: [{ href: "https://github.com/caraml-dev/xp", label: "XP User Guide" }], | |||
pagination: { | |||
defaultPageSize: 10, | |||
experimentContextPageSize: 50, | |||
}, | |||
listExperimentFields: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this should be an app-level config. Can it just be embedded in the components that use them? Reasons:
- If this were an app-config, I think it would make sense to let it drive the behavior of the components entirely (like modifying the
experimentTableFields
here could also change the columns displayed in the experiments table, which is not the case now and I think that would be over-engineering at this point). - These configs are not re-used in multiple places which could also be a hint that this can be localised.
If either of the above reasons changed, we could consider extracting them out again but perhaps we will need to think more about what influence such configs will have (bullet # 1 above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, I was thinking of #1 as a distant possibility but yeah now that you mention it, it's probably over-engineering; I've removed both configs and directly embedded the lists in the components they are used.
return ( | ||
<ExperimentContext.Provider | ||
value={{ | ||
allExperiments, | ||
isAllExperimentsLoaded, | ||
experiments, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we rename this to scheduledAndRunningExperiments
or something to be more specific? Although the ExperimentContextProvider
is only used in the Turing microfrontend components now, a more accurate name could help development in the future if we look to use it in other places.
Thanks a lot for the comments @krithika369, I've updated the PR to address them and I'll be merging this right now! |
What this PR does / why we need it:
This PR introduces a field selector to the
ListExperiments
API of the management service; previously, field selectors only existed for theListTreatments
andListSegments
APIs.The introduction of this field selector would allow more selective control over the fields of the experiment objects to return. Doing so would coincidentally allow the
ListExperimentsTable
of the UI to display larger numbers of experiments (>10) in each page, which it was previously unable to do because querying the API for an entire list of experiments would have resulted in too much data being transferred (due to the unnecessary fields/columns being returned).As such, this PR also makes a tiny change to the UI to activate the option to allow users to select the number of experiments they wish to display in each page of the table. This change also affects the
ListTreatmentsTable
andListSegmentsTable
.Modifications
api/experiments.yaml
- Addition of thefields
query field to theListExperiments
APIapi/schema.yaml
- Addition of theExperimentField
schemamanagement-service/models/experiment.go
- Modification of theToApiSchema
method to return only selected fieldsmanagement-service/pagination/pagination.go
- Modification of the existingMaxPageSize
from 10 to 50management-service/services/experiment_service.go
- Modification of theListExperiments
method to handle the fields, and minor refactoring of the start/end time filter handling into a separate helper methodui/src/components/table/BasicTable.js
- Toggling of the prop to show page options in experiment/treatment/segment tablesui/src/config.js
- Addition of new configuration to specify the number of pages/fields to query when using theListExperiments
APIui/src/turing/components/form/standard_ensembler/LinkedRoutesTable.js
- Minor refactoring of the existing process to retrieve the status (friendly) of an experiment