Add back bz component by deads2k · Pull Request #69 · openshift/sippy

deads2k · 2020-09-02T13:53:04Z

undoes the revert with a much cleaner interface

This reverts commit 953f785.

This reverts commit 792761b.

This reverts commit 9ccca71.

…ypes

openshift-ci-robot · 2020-09-02T13:53:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deads2k
To complete the pull request process, please assign bparees
You can assign the PR to them by writing /assign @bparees in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mfojtik · 2020-09-02T14:15:25Z

+)
+
+func bugLink(bug bugsv1.Bug) string {
+	return fmt.Sprintf("<a target=\"_blank\" href=%s>%d</a> ", bug.Url, bug.ID)


nit: href="%s"

mfojtik · 2020-09-02T14:22:03Z

+	Failures       int          `json:"failures"`
+	Flakes         int          `json:"flakes"`
+	PassPercentage float64      `json:"passPercentage"`
+	BugList        []bugsv1.Bug `json:"BugList"`


nit: bugList ?

mfojtik · 2020-09-02T14:24:17Z

-	for t := range failedTestNamesAcrossAllJobRuns {
-		if util.IgnoreTestRegex.MatchString(t) {
+	for testName := range failedTestNamesAcrossAllJobRuns {
+		if util.IgnoreTestRegex.MatchString(testName) {


can this be just util.IsTestIgnored() ?

can this be just util.IsTestIgnored() ?

this shouldn't leak out. I tightened it up in the interim

mfojtik · 2020-09-02T14:27:36Z

+	LastChangeTime time.Time `json:"last_change_time"`
+	Summary        string    `json:"summary"`
+	TargetRelease  []string  `json:"target_release"`
+	Component      []string  `json:"component"`


nit: Components ?

this is driven by the ci-seach api:
https://search.ci.openshift.org/v2/search?search=storage&type=bug

bugInfo": { "id": 1867757, "status": "VERIFIED", "resolution": "", "severity": "medium", "priority": "unspecified", "summary": "Rebase node-registrar sidebar with latest version", "keywords": [ ], "whiteboard": "", "cf_internal_whiteboard": "", "creator": "hekumar@redhat.com", "component": [ "Storage" ], "assigned_to": "hekumar@redhat.com", "creation_time": "2020-08-10T17:07:59Z", "last_change_time": "2020-08-25T01:42:05Z", "cf_environment": "", "target_release": [ "4.6.0" ] }

(which in turn is a more or less direct mapping of the BZ api)

mfojtik · 2020-09-02T14:27:51Z

+	Status         string    `json:"status"`
+	LastChangeTime time.Time `json:"last_change_time"`
+	Summary        string    `json:"summary"`
+	TargetRelease  []string  `json:"target_release"`


wonder why is this an []string, can you have more than 1 target release?

yes. the BZ api returns it as an array. though i haven't figured out how set more than one in the UI.

mfojtik · 2020-09-02T14:28:33Z

+// BugzillaBug matches the bugzilla API.  We cannot change this and should consider writing a converter instead of having
+// a serialization we don't own.
+type BugzillaBug struct {
+	ID             int64     `json:"id"`


in all bugzilla libs I saw the ID was an int :-)

mfojtik · 2020-09-02T14:30:02Z

+		c.cache[testName] = []bugsv1.Bug{}
+		batchCount++
+
+		if batchCount > 50 {


nit:

if batchCount <=50 { continue }

makes less nesting

mfojtik · 2020-09-02T14:31:35Z

+	searchUrl := "https://search.ci.openshift.org/v2/search"
+	resp, err := http.PostForm(searchUrl, v)
+	if err != nil {
+		e := fmt.Errorf("error during bug search against %s: %s", searchUrl, err)


deads2k · 2020-09-02T19:43:55Z

@mfojtik I propose dealing with all those as followups.

bparees · 2020-09-02T20:49:27Z

+	Successes          int     `json:"successes"`
+	Failures           int     `json:"failures"`
+	TestPassPercentage float64 `json:"testPassPercentage"`
+	// TestResults holds the values for individual runs of this test sorted by.....


don't leave me in suspense......i assume they're storted by pass rate?

it's not really individual runs of the test, right? isn't it an array of tests, each entry being a testname and the totals/percentages(across all runs of that test) for that test name?

don't leave me in suspense......i assume they're storted by pass rate?

it's not really individual runs of the test, right? isn't it an array of tests, each entry being a testname and the totals/percentages(across all runs of that test) for that test name?

I have no idea. Your API ;)

I have no idea. Your API ;)

not any more..... anyway that's what it was last i knew.

bparees · 2020-09-02T20:54:33Z

+	// TotalRuns is the number of runs this Job has run total.
+	TotalRuns int `json:"totalRuns"`
+
+	// Failures are a full list of the failures by this BZ component in the given job.


Suggested change

// Failures are a full list of the failures by this BZ component in the given job.

// Failures are a full list of the failures caused by this BZ component in the given job.

bparees · 2020-09-02T20:55:12Z

-	         <a href="#TestImpactingComponents">Test Impacting Components</a>
+	         <br/>
+	         <a href="#TestImpactingComponents">Test Impacting Components</a> |
+	         <a href="#BZToWorstJobImpact">Bugzilla Components to Worst Job Impact</a>


thank you, i was going to ask for you to fix that formatting :)

bparees · 2020-09-02T21:10:03Z

+	s := fmt.Sprintf(`
+	<table class="table">
+		<tr>
+			<th colspan=4 class="text-center"><a class="text-dark" title="Bugzilla components ranked by maximum fail percentage of any job." id="BZToWorstJobImpact" href="#BZToWorstJobImpact">Bugzilla Components to Worst Job Impact</a></th>


Suggested change

<th colspan=4 class="text-center"><a class="text-dark" title="Bugzilla components ranked by maximum fail percentage of any job." id="BZToWorstJobImpact" href="#BZToWorstJobImpact">Bugzilla Components to Worst Job Impact</a></th>

<th colspan=4 class="text-center"><a class="text-dark" title="Bugzilla components ranked by maximum fail percentage of any job." id="BZToWorstJobImpact" href="#BZToWorstJobImpact">Job Impacting BZ Components</a></th>

(and elsewhere)

Bugzilla components ranked by maximum fail percentage of any job

suggestion:

Bugzilla components ranked by computing which job they contribute the most failure percentage to and then sorting the components by each one's worst job performance.

but i wonder if this would be clearer if you just showed which job was the top failing one for the component, in the top line results, and the expansion just showed "other jobs" or "all jobs"

bparees · 2020-09-02T21:22:06Z

+			<th colspan=4 class="text-center"><a class="text-dark" title="Bugzilla components ranked by maximum fail percentage of any job." id="BZToWorstJobImpact" href="#BZToWorstJobImpact">Bugzilla Components to Worst Job Impact</a></th>
+		</tr>
+		<tr>
+			<th>Variant</th><th>Latest %d days</th><th/><th>Previous 7 days</th>


Suggested change

<th>Variant</th><th>Latest %d days</th><th/><th>Previous 7 days</th>

<th>Component</th><th>Latest %d days</th><th/><th>Previous 7 days</th>

bparees · 2020-09-02T21:26:28Z

+
+		prev := util.GetPrevBugzillaJobFailures(v.Name, failuresByBugzillaComponentPrev)
+		highestFailPercentage := v.JobsFailed[0].FailPercentage
+		lowestPassPercentage := 100 - highestFailPercentage


so ultimately the value you're presenting in the table is "the percentage of jobs which did not fail because of this component" right?

i appreciate it is aligned w/ how the other tables work but it also doesn't seem like the most intuitive value. Need to think about it.

i appreciate it is aligned w/ how the other tables work but it also doesn't seem like the most intuitive value. Need to think about it

aligning to the other tables was the what I was going for. It was super confusing in the reverse and I knew what it was doing.

hm, another confusing thing about this is that the top line value you're presenting isn't some aggregation of the percentage of failures this component is causing across all jobs. It's "here's the job where this component performs worst"

which i realize probably aligns with your intent (if a single component is tanking a single job, that's the thing you care about more than anything in the world, more so than another component that might be impacting all jobs but to a lesser degree....which is more or less what the "test impacting components" view is showing). But i'm pretty sure people won't understand it. I know i was confused when i started expanding jobs and trying to understand how the top line number made sense in light of the expanded jobs values.

your hover text sort of explains it. let me see if i can offer a suggestion on that to help.

bparees · 2020-09-02T21:30:56Z

+				v.Name,
+				safeBZJob,
+				lowestPassPercentage,
+				prev.JobsFailed[0].TotalRuns,


something seems off about this value. i'm seeing pretty low values reported. if it were the number of failed runs the numbers would make sense, but as a "total" they don't make sense to me. let's discuss when you're free.

or maybe it's low because you just picked the first entry? should this be a sum of all the entries?

also i don't thiink you mean to have prev here. should be the current week

bparees · 2020-09-02T21:37:02Z

+				failingJob.JobName,
+				bzJobTuple,
+				100.0-failingJob.FailPercentage,
+				failingJob.NumberOfJobRunsFailed,


since you inverted the percentage to be "passing" i think you should invert this to be successes(well, non-failures) also.

…mponent" Errorf Fixing a bug from 0ff3699 (add failures by BZ component, 2020-09-01, openshift#69) to avoid: $ go test ./... # github.com/openshift/sippy/pkg/testgridanalysis/testidentification pkg/testgridanalysis/testidentification/component.go:168:10: Errorf format %q reads arg openshift#1, but call has 0 args pkg/testgridanalysis/testidentification/component.go:176:10: Errorf format %q reads arg openshift#1, but call has 0 args ...

deads2k added 13 commits August 31, 2020 12:08

Revert "report blocking by BZ component"

cb9063e

This reverts commit 953f785.

Revert "add data for bz component sorting:"

65fbf5a

This reverts commit 792761b.

Revert "add bz, operator, and sig mappings"

b1fac27

This reverts commit 9ccca71.

create bug cache

f728a18

move testgrid apis to api

dfe046f

stop producing output data dynamically

cb0996a

ignore tests in one spot

7ca81ad

straight move the remaining api serialization

f0ace23

separate the test result intermediate types from test result output t…

5048cf7

…ypes

refine jobrunresult into intermediate and final APIs

39bbb0d

separate testgrid job information from processed data

19a9cde

isolate data gathering from test grid input from bug input

d426383

isolate the intermediate processing types

e97ddb6

openshift-ci-robot requested a review from bparees September 2, 2020 13:53

deads2k force-pushed the add-back-bz-component branch from 43ba900 to 27dadfc Compare September 2, 2020 13:53

add failures by BZ component

0ff3699

deads2k force-pushed the add-back-bz-component branch from 27dadfc to 0ff3699 Compare September 2, 2020 13:59

mfojtik reviewed Sep 2, 2020

View reviewed changes

bparees reviewed Sep 2, 2020

View reviewed changes

openshift-merge-robot merged commit 0ff3699 into openshift:master Sep 3, 2020

wking mentioned this pull request Sep 29, 2020

pkg/testgridanalysis/testidentification: Fix "not a valid bugzilla component" Errorf #112

Merged

	// Failures are a full list of the failures by this BZ component in the given job.
	// Failures are a full list of the failures caused by this BZ component in the given job.

	<th colspan=4 class="text-center"><a class="text-dark" title="Bugzilla components ranked by maximum fail percentage of any job." id="BZToWorstJobImpact" href="#BZToWorstJobImpact">Bugzilla Components to Worst Job Impact</a></th>
	<th colspan=4 class="text-center"><a class="text-dark" title="Bugzilla components ranked by maximum fail percentage of any job." id="BZToWorstJobImpact" href="#BZToWorstJobImpact">Job Impacting BZ Components</a></th>

	<th>Variant</th><th>Latest %d days</th><th/><th>Previous 7 days</th>
	<th>Component</th><th>Latest %d days</th><th/><th>Previous 7 days</th>

Conversation

deads2k commented Sep 2, 2020

Uh oh!

openshift-ci-robot commented Sep 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Sep 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mfojtik Sep 2, 2020 •

edited

Loading