From 618650745869fea6b8b1382d480dc24fb19f17c8 Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Fri, 16 Aug 2024 13:28:29 -0400 Subject: [PATCH 01/11] api: add cross-compare parameter parsing --- .../componentreadiness/queryparamparser.go | 51 +++++-- .../queryparamparser_test.go | 131 +++++++++++++++++- pkg/api/utils.go | 24 ++-- pkg/apis/api/componentreport/types.go | 10 +- pkg/sippyserver/metrics/metrics.go | 2 +- 5 files changed, 186 insertions(+), 32 deletions(-) diff --git a/pkg/api/componentreadiness/queryparamparser.go b/pkg/api/componentreadiness/queryparamparser.go index 3476791f9..ff7835e19 100644 --- a/pkg/api/componentreadiness/queryparamparser.go +++ b/pkg/api/componentreadiness/queryparamparser.go @@ -158,29 +158,56 @@ func parsePROptions(req *http.Request) *crtype.PullRequestOptions { return &pro } -func parseVariantOptions(req *http.Request, allJobVariants crtype.JobVariants) (crtype.RequestVariantOptions, error) { - var err error - variantOption := crtype.RequestVariantOptions{} +func parseVariantOptions(req *http.Request, allJobVariants crtype.JobVariants) (opts crtype.RequestVariantOptions, err error) { columnGroupBy := req.URL.Query().Get("columnGroupBy") - variantOption.ColumnGroupBy, err = api.VariantsStringToSet(allJobVariants, columnGroupBy) + opts.ColumnGroupBy, err = api.VariantsStringToSet(allJobVariants, columnGroupBy) if err != nil { - return variantOption, err + return } dbGroupBy := req.URL.Query().Get("dbGroupBy") - variantOption.DBGroupBy, err = api.VariantsStringToSet(allJobVariants, dbGroupBy) + opts.DBGroupBy, err = api.VariantsStringToSet(allJobVariants, dbGroupBy) if err != nil { - return variantOption, err + return } - variantOption.RequestedVariants = map[string]string{} + + opts.RequestedVariants = map[string]string{} // Only the dbGroupBy variants can be specifically requested - for _, variant := range variantOption.DBGroupBy.List() { + for _, variant := range opts.DBGroupBy.List() { if value := req.URL.Query().Get(variant); value != "" { - variantOption.RequestedVariants[variant] = value + opts.RequestedVariants[variant] = value } } includeVariants := req.URL.Query()["includeVariant"] - variantOption.IncludeVariants, err = api.IncludeVariantsToMap(allJobVariants, includeVariants) - return variantOption, err + opts.IncludeVariants, err = api.VariantListToMap(allJobVariants, includeVariants) + if err != nil { + return + } + compareVariants, err := api.VariantListToMap(allJobVariants, req.URL.Query()["compareVariant"]) + if err != nil { + return + } + + opts.VariantCrossCompare = req.URL.Query()["variantCrossCompare"] + if len(opts.VariantCrossCompare) > 0 { + // when we are cross-comparing variants, we need to construct the compareVariants map from the parameters. + // the resulting compareVariants map is includeVariants... + opts.CompareVariants = map[string][]string{} + for group, variants := range opts.IncludeVariants { + opts.CompareVariants[group] = variants + } + + // ...with overrides from compareVariant parameters. + for _, group := range opts.VariantCrossCompare { + if variants := compareVariants[group]; len(variants) > 0 { + opts.CompareVariants[group] = variants + } else { + // a group override without any variants listed means not to restrict the variants in this group. + // in that case we don't want any where clause for the group, so we just omit it from the map. + delete(opts.CompareVariants, group) + } + } + } + return } func ParseIntArg(req *http.Request, name string, defaultVal int, validator func(int) bool) (int, error) { diff --git a/pkg/api/componentreadiness/queryparamparser_test.go b/pkg/api/componentreadiness/queryparamparser_test.go index a774c0c42..01f1b70b9 100644 --- a/pkg/api/componentreadiness/queryparamparser_test.go +++ b/pkg/api/componentreadiness/queryparamparser_test.go @@ -25,7 +25,7 @@ var ( func TestParseComponentReportRequest(t *testing.T) { allJobVariants := crtype.JobVariants{Variants: map[string][]string{ - "Architecture": {"amd64", "arm64", "heterogeneous"}, + "Architecture": {"amd64", "arm64", "s390x", "ppc64le", "heterogeneous"}, "FeatureSet": {"default", "techpreview"}, "Installer": {"ipi", "upi"}, "Network": {"ovn", "sdn"}, @@ -60,8 +60,29 @@ func TestParseComponentReportRequest(t *testing.T) { IgnoreDisruption: true, }, } + // would like to test with a view that does define cross-compare variants + view417cross := view417main + view417cross.Name = "4.17-cross" + view417cross.VariantOptions = crtype.RequestVariantOptions{ + VariantCrossCompare: []string{"Topology"}, + IncludeVariants: map[string][]string{ + "Architecture": []string{"amd64"}, + "Installer": []string{"ipi", "upi"}, + "Topology": []string{"ha"}, + }, + CompareVariants: map[string][]string{ + "Architecture": []string{"amd64"}, + "Installer": []string{"ipi", "upi"}, + "Topology": []string{"single"}, + }, + // also remove Topology from columnGroupBy and dbGroupBy + ColumnGroupBy: sets.NewString("Platform", "Architecture", "Network"), + DBGroupBy: sets.NewString("Platform", "Architecture", "Network", "Suite", "FeatureSet", "Upgrade", "Installer"), + } + views := []crtype.View{ view417main, + view417cross, } now := time.Now().UTC() @@ -92,7 +113,6 @@ func TestParseComponentReportRequest(t *testing.T) { {"baseRelease", "4.15"}, {"baseStartTime", "2024-02-01T00:00:00Z"}, {"confidence", "95"}, - {"groupBy", "cloud,arch,network"}, {"columnGroupBy", "Platform,Architecture,Network"}, {"dbGroupBy", "Platform,Architecture,Network,Topology,FeatureSet,Upgrade,Installer"}, {"ignoreDisruption", "true"}, @@ -146,7 +166,6 @@ func TestParseComponentReportRequest(t *testing.T) { {"baseRelease", "4.15"}, {"baseStartTime", "ga-30d"}, {"confidence", "95"}, - {"groupBy", "cloud,arch,network"}, {"columnGroupBy", "Platform,Architecture,Network"}, {"dbGroupBy", "Platform,Architecture,Network,Topology,FeatureSet,Upgrade,Installer"}, {"ignoreDisruption", "true"}, @@ -206,6 +225,7 @@ func TestParseComponentReportRequest(t *testing.T) { "FeatureSet": {"default", "techpreview"}, "Installer": {"ipi", "upi"}, }, + CompareVariants: nil, // the view is likely not to specify compare variants at all }, baseRelease: crtype.RequestReleaseOptions{ Release: "4.16", @@ -244,6 +264,111 @@ func TestParseComponentReportRequest(t *testing.T) { }, errMessage: "params cannot be combined with view", }, + { + name: "normal query params but with variant cross-compare", + queryParams: [][]string{ + {"baseEndTime", "2024-02-28T23:59:59Z"}, + {"baseRelease", "4.15"}, + {"baseStartTime", "2024-02-01T00:00:00Z"}, + {"columnGroupBy", "Platform,Network"}, + {"dbGroupBy", "Platform,Network,FeatureSet,Upgrade,Installer"}, + {"sampleEndTime", "2024-04-11T23:59:59Z"}, + {"sampleRelease", "4.16"}, + {"sampleStartTime", "2024-04-04T00:00:05Z"}, + {"includeVariant", "Architecture:amd64"}, + {"includeVariant", "Architecture:arm64"}, + {"includeVariant", "Topology:ha"}, + {"includeVariant", "FeatureSet:default"}, + {"includeVariant", "Installer:ipi"}, + {"includeVariant", "Installer:upi"}, + {"variantCrossCompare", "Architecture"}, + {"variantCrossCompare", "Topology"}, + {"compareVariant", "Architecture:s390x"}, + {"compareVariant", "Architecture:ppc64le"}, + {"compareVariant", "Topology:single"}, + }, + variantOption: crtype.RequestVariantOptions{ + ColumnGroupBy: sets.NewString("Platform", "Network"), + DBGroupBy: sets.NewString("Platform", "Network", "FeatureSet", "Upgrade", "Installer"), + IncludeVariants: map[string][]string{ + "Architecture": {"amd64", "arm64"}, + "Topology": {"ha"}, + "FeatureSet": {"default"}, + "Installer": {"ipi", "upi"}, + }, + CompareVariants: map[string][]string{ + "Architecture": {"s390x", "ppc64le"}, + "Topology": {"single"}, + "FeatureSet": {"default"}, + "Installer": {"ipi", "upi"}, + }, + VariantCrossCompare: []string{"Architecture", "Topology"}, + RequestedVariants: map[string]string{}, + }, + baseRelease: crtype.RequestReleaseOptions{ + Release: "4.15", + Start: time.Date(2024, time.February, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, time.February, 28, 23, 59, 59, 0, time.UTC), + }, + sampleRelease: crtype.RequestReleaseOptions{ + Release: "4.16", + Start: time.Date(2024, time.April, 4, 0, 0, 5, 0, time.UTC), + End: time.Date(2024, time.April, 11, 23, 59, 59, 0, time.UTC), + }, + testIDOption: crtype.RequestTestIdentificationOptions{}, + advancedOption: crtype.RequestAdvancedOptions{ + MinimumFailure: 3, + Confidence: 95, + PityFactor: 5, + IgnoreMissing: false, + IgnoreDisruption: true, + }, + cacheOption: cache.RequestOptions{ + ForceRefresh: false, + }, + }, + { + name: "cross-compare view", + queryParams: [][]string{ + {"view", "4.17-cross"}, + }, + variantOption: crtype.RequestVariantOptions{ + ColumnGroupBy: sets.NewString("Platform", "Architecture", "Network"), + DBGroupBy: sets.NewString("Platform", "Architecture", "Network", "Suite", "FeatureSet", "Upgrade", "Installer"), + IncludeVariants: map[string][]string{ + "Architecture": {"amd64"}, + "Installer": {"ipi", "upi"}, + "Topology": {"ha"}, + }, + VariantCrossCompare: []string{"Topology"}, + CompareVariants: map[string][]string{ + "Architecture": {"amd64"}, + "Installer": {"ipi", "upi"}, + "Topology": {"single"}, + }, + }, + baseRelease: crtype.RequestReleaseOptions{ + Release: "4.16", + Start: time.Date(2024, time.May, 28, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, time.June, 27, 23, 59, 59, 0, time.UTC), + }, + sampleRelease: crtype.RequestReleaseOptions{ + Release: "4.17", + Start: time.Date(nowMinus7Days.Year(), nowMinus7Days.Month(), nowMinus7Days.Day(), 0, 0, 0, 0, time.UTC), + End: nowRoundUp, + }, + testIDOption: crtype.RequestTestIdentificationOptions{}, + advancedOption: crtype.RequestAdvancedOptions{ + MinimumFailure: 3, + Confidence: 95, + PityFactor: 5, + IgnoreMissing: false, + IgnoreDisruption: true, + }, + cacheOption: cache.RequestOptions{ + ForceRefresh: false, + }, + }, } for _, tc := range tests { diff --git a/pkg/api/utils.go b/pkg/api/utils.go index ff3a05f6c..e5a55d0c8 100644 --- a/pkg/api/utils.go +++ b/pkg/api/utils.go @@ -149,32 +149,32 @@ func VariantsStringToSet(allJobVariants crtype.JobVariants, variantsString strin return variantSet, nil } -func IncludeVariantsToMap(allJobVariants crtype.JobVariants, includeVariants []string) (map[string][]string, error) { - includeVariantsMap := map[string][]string{} +func VariantListToMap(allJobVariants crtype.JobVariants, variants []string) (map[string][]string, error) { + variantsMap := map[string][]string{} var err error - for _, includeVariant := range includeVariants { - kv := strings.Split(includeVariant, ":") + for _, variant := range variants { + kv := strings.Split(variant, ":") if len(kv) != 2 { - err = fmt.Errorf("invalid includeVariant %s", includeVariant) - return includeVariantsMap, err + err = fmt.Errorf("invalid variant %s in list", variant) + return variantsMap, err } values, ok := allJobVariants.Variants[kv[0]] if !ok { - err = fmt.Errorf("invalid variant name from includeVariant %s", includeVariant) - return includeVariantsMap, err + err = fmt.Errorf("invalid name from list variant %s", variant) + return variantsMap, err } found := false for _, v := range values { if v == kv[1] { - includeVariantsMap[kv[0]] = append(includeVariantsMap[kv[0]], kv[1]) + variantsMap[kv[0]] = append(variantsMap[kv[0]], kv[1]) found = true break } } if !found { - err = fmt.Errorf("invalid variant value from includeVariant %s", includeVariant) - return includeVariantsMap, err + err = fmt.Errorf("invalid value from list variant %s", variant) + return variantsMap, err } } - return includeVariantsMap, err + return variantsMap, err } diff --git a/pkg/apis/api/componentreport/types.go b/pkg/apis/api/componentreport/types.go index 552e2042c..182782c49 100644 --- a/pkg/apis/api/componentreport/types.go +++ b/pkg/apis/api/componentreport/types.go @@ -42,10 +42,12 @@ type RequestTestIdentificationOptions struct { } type RequestVariantOptions struct { - ColumnGroupBy sets.String `json:"column_group_by" yaml:"column_group_by"` - DBGroupBy sets.String `json:"db_group_by" yaml:"db_group_by"` - IncludeVariants map[string][]string `json:"include_variants" yaml:"include_variants"` - RequestedVariants map[string]string `json:"requested_variants" yaml:"requested_variants"` + ColumnGroupBy sets.String `json:"column_group_by" yaml:"column_group_by"` + DBGroupBy sets.String `json:"db_group_by" yaml:"db_group_by"` + IncludeVariants map[string][]string `json:"include_variants" yaml:"include_variants"` + CompareVariants map[string][]string `json:"compare_variants" yaml:"compare_variants"` + VariantCrossCompare []string `json:"variant_cross_compare" yaml:"variant_cross_compare"` + RequestedVariants map[string]string `json:"requested_variants" yaml:"requested_variants"` } // RequestOptions is a struct packaging all the options for a CR request. diff --git a/pkg/sippyserver/metrics/metrics.go b/pkg/sippyserver/metrics/metrics.go index 1c2a24e6b..aea86a136 100644 --- a/pkg/sippyserver/metrics/metrics.go +++ b/pkg/sippyserver/metrics/metrics.go @@ -311,7 +311,7 @@ func refreshComponentReadinessMetrics(client *bqclient.Client, prowURL, gcsBucke if err != nil { return err } - includeVariantsMap, err := api.IncludeVariantsToMap(allJobVariants, componentreadiness.DefaultIncludeVariants) + includeVariantsMap, err := api.VariantListToMap(allJobVariants, componentreadiness.DefaultIncludeVariants) if err != nil { return err } From 1a4be9a76b8f60e2798f02902c544c310f1753f2 Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Mon, 19 Aug 2024 08:23:14 -0400 Subject: [PATCH 02/11] api: update CR report to handle cross-compare --- .../componentreadiness/component_report.go | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/pkg/api/componentreadiness/component_report.go b/pkg/api/componentreadiness/component_report.go index 1ac64e3b7..8e682ab3c 100644 --- a/pkg/api/componentreadiness/component_report.go +++ b/pkg/api/componentreadiness/component_report.go @@ -413,15 +413,24 @@ func (c *componentReportGenerator) getCommonTestStatusQuery(allJobVariants crtyp queryString += ` AND NOT 'Disruption' in UNNEST(capabilities)` } - for k, vs := range c.IncludeVariants { + variantGroups := c.IncludeVariants + // potentially cross-compare variants for the sample + if isSample && len(c.VariantCrossCompare) > 0 { + variantGroups = c.CompareVariants + } + if variantGroups == nil { // server-side view definitions may omit a variants map + variantGroups = map[string][]string{} + } + + for group, variants := range variantGroups { queryString += " AND (" first := true - for _, v := range vs { + for _, variant := range variants { if first { - queryString += fmt.Sprintf(`jv_%s.variant_value = '%s'`, k, v) + queryString += fmt.Sprintf(`jv_%s.variant_value = '%s'`, group, variant) first = false } else { - queryString += fmt.Sprintf(` OR jv_%s.variant_value = '%s'`, k, v) + queryString += fmt.Sprintf(` OR jv_%s.variant_value = '%s'`, group, variant) } } queryString += ")" @@ -534,7 +543,10 @@ func (c *componentReportGenerator) getSampleQueryStatus( ComponentReportGenerator: c, } - componentReportTestStatus, errs := api.GetDataFromCacheOrGenerate[crtype.ReportTestStatus](c.client.Cache, c.cacheOption, api.GetPrefixedCacheKey("SampleTestStatus~", generator), generator.queryTestStatus, crtype.ReportTestStatus{}) + componentReportTestStatus, errs := api.GetDataFromCacheOrGenerate[crtype.ReportTestStatus]( + c.client.Cache, c.cacheOption, + api.GetPrefixedCacheKey("SampleTestStatus~", generator), + generator.queryTestStatus, crtype.ReportTestStatus{}) if len(errs) > 0 { return nil, errs From 6fd229096b23a1d17037e7cb5f2522e028d63909 Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Mon, 19 Aug 2024 09:54:40 -0400 Subject: [PATCH 03/11] ui: Revert "nerf cross-variant comparison until it has backend support" This reverts commit 25be350a085d1bd5b9be9e184e52fdb945dddb12. --- .../IncludeVariantCheckboxList.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/sippy-ng/src/component_readiness/IncludeVariantCheckboxList.js b/sippy-ng/src/component_readiness/IncludeVariantCheckboxList.js index 109bf4579..7b2f2cf6c 100644 --- a/sippy-ng/src/component_readiness/IncludeVariantCheckboxList.js +++ b/sippy-ng/src/component_readiness/IncludeVariantCheckboxList.js @@ -123,6 +123,18 @@ export default function IncludeVariantCheckBoxList(props) { {isCompareMode ? CompareVariantGroup(params) : IncludeVariantGroup(params)} + + + From 927f68683e5f0e0e1f5dd9a2aba9c483fe7c68ee Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Mon, 19 Aug 2024 11:44:28 -0400 Subject: [PATCH 04/11] ui: remove some noise in JS console --- sippy-ng/src/component_readiness/CompReadyUtils.js | 2 -- sippy-ng/src/component_readiness/GeneratedAt.js | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/sippy-ng/src/component_readiness/CompReadyUtils.js b/sippy-ng/src/component_readiness/CompReadyUtils.js index 930fbb54a..fe6e7dd82 100644 --- a/sippy-ng/src/component_readiness/CompReadyUtils.js +++ b/sippy-ng/src/component_readiness/CompReadyUtils.js @@ -353,11 +353,9 @@ export function getUpdatedUrlParts(vars) { ) Object.entries(vars.compareVariantsCheckedItems).forEach( ([group, variants]) => { - console.log(group, variants) // for UI purposes we may be holding compareVariants that aren't actually being compared, so they don't get wiped // out just by toggling the "Compare" button. But for the parameters we will filter these out. if (vars.variantCrossCompare.includes(group)) { - console.log('including', group, 'in compareVariants') variants.forEach((variant) => { queryParams.append('compareVariant', group + ':' + variant) }) diff --git a/sippy-ng/src/component_readiness/GeneratedAt.js b/sippy-ng/src/component_readiness/GeneratedAt.js index 6c8cc61ce..01fab3f87 100644 --- a/sippy-ng/src/component_readiness/GeneratedAt.js +++ b/sippy-ng/src/component_readiness/GeneratedAt.js @@ -14,7 +14,7 @@ export default function GeneratedAt(props) { - Generated {relativeTime(d, new Date())} + Generated {relativeTime(d, new Date())} From f01445a9c876b04df9f98ae6bec0f1cbbaaed8ef Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Mon, 19 Aug 2024 13:19:55 -0400 Subject: [PATCH 05/11] ui: fix disconnected Group By selection ...oops, I broke Group By and no one noticed so far --- sippy-ng/src/component_readiness/GroupByCheckboxList.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/sippy-ng/src/component_readiness/GroupByCheckboxList.js b/sippy-ng/src/component_readiness/GroupByCheckboxList.js index 3c068f4b4..82d87b142 100644 --- a/sippy-ng/src/component_readiness/GroupByCheckboxList.js +++ b/sippy-ng/src/component_readiness/GroupByCheckboxList.js @@ -26,15 +26,14 @@ export default function GroupByCheckboxList(props) { })) const classes = useStyles() - const [checkedItems, setCheckedItems] = useState(props.checkedItems) const handleChange = (event) => { const item = event.target.name const isChecked = event.target.checked if (isChecked) { - setCheckedItems([...checkedItems, item]) + props.setCheckedItems([...props.checkedItems, item]) } else { - setCheckedItems( - checkedItems.filter((checkedItem) => checkedItem !== item) + props.setCheckedItems( + props.checkedItems.filter((checkedItem) => checkedItem !== item) ) } } @@ -72,7 +71,7 @@ export default function GroupByCheckboxList(props) { disabled={isCompareMode} control={ Date: Wed, 21 Aug 2024 14:59:10 -0400 Subject: [PATCH 06/11] ui: encode variantCrossCompare param correctly --- sippy-ng/src/component_readiness/CompReadyUtils.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sippy-ng/src/component_readiness/CompReadyUtils.js b/sippy-ng/src/component_readiness/CompReadyUtils.js index fe6e7dd82..72096ff57 100644 --- a/sippy-ng/src/component_readiness/CompReadyUtils.js +++ b/sippy-ng/src/component_readiness/CompReadyUtils.js @@ -328,7 +328,6 @@ export function getUpdatedUrlParts(vars) { const arraysMap = { columnGroupBy: filterOutVariantCC(vars.columnGroupByCheckedItems), dbGroupBy: filterOutVariantCC(vars.dbGroupByVariants), - variantCrossCompare: vars.variantCrossCompare, } const queryParams = new URLSearchParams() @@ -362,6 +361,9 @@ export function getUpdatedUrlParts(vars) { } } ) + vars.variantCrossCompare.forEach((item) => { + queryParams.append('variantCrossCompare', item) + }) // Stringify and put the begin param character. queryParams.sort() // ensure they always stay in sorted order to prevent url history changes From 921b3de2988bbcca2270aaaae73e4147c6b4966f Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Tue, 20 Aug 2024 16:12:56 -0400 Subject: [PATCH 07/11] api: disable regression analysis for cross-compare --- pkg/api/componentreadiness/component_report.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/api/componentreadiness/component_report.go b/pkg/api/componentreadiness/component_report.go index 8e682ab3c..8d0fa9a7f 100644 --- a/pkg/api/componentreadiness/component_report.go +++ b/pkg/api/componentreadiness/component_report.go @@ -1334,8 +1334,12 @@ func (c *componentReportGenerator) generateComponentTestReport(baseStatus map[st if !ok { testStats.ReportStatus = crtype.MissingSample } else { - approvedRegression := regressionallowances.IntentionalRegressionFor(c.SampleRelease.Release, testID.ColumnIdentification, testID.TestID) - resolvedIssueCompensation, triagedIncidents = c.triagedIncidentsFor(testID) + var approvedRegression *regressionallowances.IntentionalRegression + if len(c.VariantCrossCompare) == 0 { // only really makes sense when not cross-comparing variants: + // look for corresponding regressions we can account for in the analysis + approvedRegression = regressionallowances.IntentionalRegressionFor(c.SampleRelease.Release, testID.ColumnIdentification, testID.TestID) + resolvedIssueCompensation, triagedIncidents = c.triagedIncidentsFor(testID) + } requiredConfidence := c.getRequiredConfidence(testID.TestID, testID.Variants) testStats = c.assessComponentStatus(requiredConfidence, sampleStats.TotalCount, sampleStats.SuccessCount, sampleStats.FlakeCount, baseStats.TotalCount, baseStats.SuccessCount, From 5200398cac6b3ad00fe9fef9f51c5f0b45730e30 Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Tue, 20 Aug 2024 19:37:13 -0400 Subject: [PATCH 08/11] api: cleanup some in test_details --- pkg/api/componentreadiness/test_details.go | 55 +++++++++++++++++----- pkg/api/utils.go | 11 +++++ 2 files changed, 53 insertions(+), 13 deletions(-) diff --git a/pkg/api/componentreadiness/test_details.go b/pkg/api/componentreadiness/test_details.go index e61703a18..8d9d87dfb 100644 --- a/pkg/api/componentreadiness/test_details.go +++ b/pkg/api/componentreadiness/test_details.go @@ -81,12 +81,6 @@ func (c *componentReportGenerator) GenerateJobRunTestReportStatus() (crtype.JobR // getTestDetailsQuery returns the report for a specific test + variant combo, including job run data. // This is for the bottom level most specific pages in component readiness. func (c *componentReportGenerator) getTestDetailsQuery(allJobVariants crtype.JobVariants, isSample bool) (string, string, []bigquery2.QueryParameter) { - joinVariants := "" - for v := range allJobVariants.Variants { - joinVariants += fmt.Sprintf("LEFT JOIN %s.job_variants jv_%s ON variant_registry_job_name = jv_%s.job_name AND jv_%s.variant_name = '%s'\n", - c.client.Dataset, v, v, v, v) - } - jobNameQueryPortion := normalJobNameCol if c.SampleRelease.PullRequestOptions != nil && isSample { jobNameQueryPortion = pullRequestDynamicJobNameCol @@ -113,6 +107,12 @@ func (c *componentReportGenerator) getTestDetailsQuery(allJobVariants crtype.Job INNER JOIN latest_component_mapping cm ON testsuite = cm.suite AND test_name = cm.name `, c.client.Dataset, c.client.Dataset, fmt.Sprintf(dedupedJunitTable, jobNameQueryPortion, c.client.Dataset, c.client.Dataset)) + joinVariants := "" + for variant := range allJobVariants.Variants { + v := api.CleanseSQLName(variant) + joinVariants += fmt.Sprintf("LEFT JOIN %s.job_variants jv_%s ON variant_registry_job_name = jv_%s.job_name AND jv_%s.variant_name = '%s'\n", + c.client.Dataset, v, v, v, v) + } queryString += joinVariants groupString := ` @@ -136,12 +136,32 @@ func (c *componentReportGenerator) getTestDetailsQuery(allJobVariants crtype.Job Value: c.TestID, }, } - for k, v := range c.RequestedVariants { - queryString += fmt.Sprintf(` AND jv_%s.variant_value = '%s'`, k, v) + for group, variant := range c.RequestedVariants { + queryString += fmt.Sprintf(` AND jv_%s.variant_value = '%s'`, api.CleanseSQLName(group), api.CleanseSQLName(variant)) } return queryString, groupString, commonParams } +// filterByCrossCompareVariants adds the where clause for any variants being cross-compared (which are not included in RequiredVariants). +// As a side effect, it also appends any necessary parameters for the clause. +func filterByCrossCompareVariants(crossCompare []string, variantGroups map[string][]string, params *[]bigquery2.QueryParameter) (whereClause string) { + if len(variantGroups) == 0 { + return // avoid possible nil pointer dereference + } + for _, group := range crossCompare { + if variants := variantGroups[group]; len(variants) > 0 { + group = api.CleanseSQLName(group) + paramName := "CrossVariants" + group + whereClause += fmt.Sprintf(` AND jv_%s.variant_value IN UNNEST(@%s)`, group, paramName) + *params = append(*params, bigquery2.QueryParameter{ + Name: paramName, + Value: variants, + }) + } + } + return +} + type baseJobRunTestStatusGenerator struct { commonQuery string groupByQuery string @@ -165,13 +185,17 @@ func (c *componentReportGenerator) getBaseJobRunTestStatus(commonQuery string, ComponentReportGenerator: c, } - componentReportTestStatus, errs := api.GetDataFromCacheOrGenerate[crtype.JobRunTestReportStatus](generator.ComponentReportGenerator.client.Cache, generator.cacheOption, api.GetPrefixedCacheKey("BaseJobRunTestStatus~", generator), generator.queryTestStatus, crtype.JobRunTestReportStatus{}) + jobRunTestStatus, errs := api.GetDataFromCacheOrGenerate[crtype.JobRunTestReportStatus]( + generator.ComponentReportGenerator.client.Cache, generator.cacheOption, + api.GetPrefixedCacheKey("BaseJobRunTestStatus~", generator), + generator.queryTestStatus, + crtype.JobRunTestReportStatus{}) if len(errs) > 0 { return nil, errs } - return componentReportTestStatus.BaseStatus, nil + return jobRunTestStatus.BaseStatus, nil } func (b *baseJobRunTestStatusGenerator) queryTestStatus() (crtype.JobRunTestReportStatus, []error) { @@ -207,7 +231,8 @@ type sampleJobRunTestQueryGenerator struct { func (c *componentReportGenerator) getSampleJobRunTestStatus(commonQuery string, groupByQuery string, - queryParameters []bigquery2.QueryParameter) (map[string][]crtype.JobRunTestStatusRow, []error) { + queryParameters []bigquery2.QueryParameter, +) (map[string][]crtype.JobRunTestStatusRow, []error) { generator := sampleJobRunTestQueryGenerator{ commonQuery: commonQuery, groupByQuery: groupByQuery, @@ -215,13 +240,17 @@ func (c *componentReportGenerator) getSampleJobRunTestStatus(commonQuery string, ComponentReportGenerator: c, } - componentReportTestStatus, errs := api.GetDataFromCacheOrGenerate[crtype.JobRunTestReportStatus](c.client.Cache, c.cacheOption, api.GetPrefixedCacheKey("SampleJobRunTestStatus~", generator), generator.queryTestStatus, crtype.JobRunTestReportStatus{}) + jobRunTestStatus, errs := api.GetDataFromCacheOrGenerate[crtype.JobRunTestReportStatus]( + c.client.Cache, c.cacheOption, + api.GetPrefixedCacheKey("SampleJobRunTestStatus~", generator), + generator.queryTestStatus, + crtype.JobRunTestReportStatus{}) if len(errs) > 0 { return nil, errs } - return componentReportTestStatus.SampleStatus, nil + return jobRunTestStatus.SampleStatus, nil } func (s *sampleJobRunTestQueryGenerator) queryTestStatus() (crtype.JobRunTestReportStatus, []error) { diff --git a/pkg/api/utils.go b/pkg/api/utils.go index e5a55d0c8..72093efeb 100644 --- a/pkg/api/utils.go +++ b/pkg/api/utils.go @@ -178,3 +178,14 @@ func VariantListToMap(allJobVariants crtype.JobVariants, variants []string) (map } return variantsMap, err } + +// CleanseSQLName removes all non-alphanumeric characters from a string that could be used as a SQL name (table, column, etc) +// This is useful for sanitizing dynamic queries built from user input. +func CleanseSQLName(name string) string { + return strings.Map(func(r rune) rune { + if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '-' || r == '_' { + return r + } + return -1 + }, name) +} From 6ced5df0e689dfd5891a04148170093a23465bfe Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Tue, 20 Aug 2024 20:47:21 -0400 Subject: [PATCH 09/11] api: test_details with correct querying for cross-variant --- pkg/api/componentreadiness/test_details.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/api/componentreadiness/test_details.go b/pkg/api/componentreadiness/test_details.go index 8d9d87dfb..6784a14e8 100644 --- a/pkg/api/componentreadiness/test_details.go +++ b/pkg/api/componentreadiness/test_details.go @@ -139,6 +139,11 @@ func (c *componentReportGenerator) getTestDetailsQuery(allJobVariants crtype.Job for group, variant := range c.RequestedVariants { queryString += fmt.Sprintf(` AND jv_%s.variant_value = '%s'`, api.CleanseSQLName(group), api.CleanseSQLName(variant)) } + if isSample { + queryString += filterByCrossCompareVariants(c.VariantCrossCompare, c.CompareVariants, &commonParams) + } else { + queryString += filterByCrossCompareVariants(c.VariantCrossCompare, c.IncludeVariants, &commonParams) + } return queryString, groupString, commonParams } From 76c949afcdc3de6f3a687c24954307d6f6d453cc Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Wed, 21 Aug 2024 14:59:51 -0400 Subject: [PATCH 10/11] ui: handle cross-compare state in test_details --- .../CompReadyTestReport.js | 77 ++++++++++++++----- .../src/component_readiness/CompReadyVars.js | 2 +- 2 files changed, 58 insertions(+), 21 deletions(-) diff --git a/sippy-ng/src/component_readiness/CompReadyTestReport.js b/sippy-ng/src/component_readiness/CompReadyTestReport.js index a3a3efd2b..10abf86cd 100644 --- a/sippy-ng/src/component_readiness/CompReadyTestReport.js +++ b/sippy-ng/src/component_readiness/CompReadyTestReport.js @@ -159,9 +159,9 @@ export default function CompReadyTestReport(props) { // this backhand way of recording the query dates keeps their display // from re-rendering to match the controls until the controls update the report - const [staticDates, setDates] = React.useState({}) + const [loadedParams, setLoadedParams] = React.useState({}) const datesEnv = useContext(CompReadyVarsContext) - useEffect(() => setDates(datesEnv), []) + useEffect(() => setLoadedParams(datesEnv), []) if (fetchError !== '') { return gotFetchError(fetchError) @@ -316,21 +316,50 @@ export default function CompReadyTestReport(props) { return null } - const printStats = (statsLabel, stats, from, to) => { + const printParamsAndStats = ( + statsLabel, + stats, + from, + to, + vCrossCompare, + variantSelection + ) => { const summaryDate = getSummaryDate(from, to, stats.release, versions) return ( {statsLabel} Release: {stats.release} + {summaryDate && ( + +
+   {summaryDate} + + )}
  Start Time: {from}
  End Time: {to} - {summaryDate && ( +
+ {vCrossCompare && (
-   {summaryDate} +   Variant Cross Comparison: +
    + {vCrossCompare.map((group, idx) => + variantSelection[group] ? ( +
  • + {group}:  + {variantSelection[group].join(', ')} +
    • + ) : ( +
  • + {group}: (any) +
    • + ) + )} +
)} +   Statistics:
  • Success Rate: {(stats.success_rate * 100).toFixed(2)}%
  • Successes: {stats.success_count}
    • @@ -383,7 +412,7 @@ Flakes: ${stats.flake_count}`

    {testName}

    - +

    Linked Bugs

    - - {printStats( - 'Sample (being evaluated)', - data.sample_stats, - staticDates.sampleStartTime.toString(), - staticDates.sampleEndTime.toString() - )} - {printStats( - 'Base (historical)', - data.base_stats, - staticDates.baseStartTime.toString(), - staticDates.baseEndTime.toString() - )} - + + + {printParamsAndStats( + 'Basis (historical)', + data.base_stats, + loadedParams.baseStartTime.toString(), + loadedParams.baseEndTime.toString(), + loadedParams.variantCrossCompare, + loadedParams.includeVariantsCheckedItems + )} + + + {printParamsAndStats( + 'Sample (being evaluated)', + data.sample_stats, + loadedParams.sampleStartTime.toString(), + loadedParams.sampleEndTime.toString(), + loadedParams.variantCrossCompare, + loadedParams.compareVariantsCheckedItems + )} + +