-
Notifications
You must be signed in to change notification settings - Fork 98
Extract metadata from flake/failure test output for certain backstop tests #685
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
Extract metadata from flake/failure test output for certain backstop tests #685
Conversation
pkg/prowloader/testoutputmetadata.go
Outdated
| if i == 0 { | ||
| continue | ||
| } | ||
| //if results[regex.SubexpNames()[i]] != "" { |
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.
Looks like the linter objects to this line...
pkg/prowloader/testoutputmetadata.go
Outdated
| } | ||
| //if results[regex.SubexpNames()[i]] != "" { | ||
| results[regex.SubexpNames()[i]] = 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.
and likely this one too. I guess either add spaces or just pull em out if you don't need them.
pkg/prowloader/testoutputmetadata.go
Outdated
| for _, re := range regexes { | ||
| matchMaps := findAllNamedMatches(re, line) | ||
|
|
||
| fmt.Printf("%v\n", matchMaps) |
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 over debugging?
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.
Yup, thanks!
pkg/db/models/prow.go
Outdated
|
|
||
| type ProwJobRunTestOutputMetadata struct { | ||
| gorm.Model | ||
| ProwJobRunTestOutputID uint |
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.
What do you think about an index on this?
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.
Will do.
|
Were you able to do JSONB queries? Something like |
I have not tried with gorm yet just in psql. |
|
/hold This needs more work. |
|
Updated, I took a new approach of regex + string tokens, I want a flexible "parse this tag if it's there" approach (tag meaning ns/foobar or ns=foobar), and regexes seem quite poor at this, so now if the simple regex for things that should always match if we're interested in a line hits, we'll proceed to token parsing and pull those out too, whatever we can find. Some of this will pickup new values coming in openshift/origin#27559 I think this is ready for another review. /hold cancel |
pkg/prowloader/prow.go
Outdated
| for _, m := range extractedMetadata { | ||
| jsonb := pgtype.JSONB{} | ||
| if err := jsonb.Set(m); err != nil { | ||
| panic(err) |
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 log a warning instead? I don't know if we'll really ever panic on jsonb.Set but the places where we panic during syncing risks data loss since it'll stop syncing and any further tasks.
|
Looks good as best I can tell, the text processing is a little complex. I really have just one comment about the panic above. I think I would prefer is openshift-tests just returned serializable failure output (YAML?) for tests we want to store metadata about but we can tackle something like that later if we want to do it. |
|
Logging an error instead of panic added. The json output occurred to me too but I couldn't see an easy way to do it. We'd need to either find a way to parse json out of test output, or correlate different output files with each junit file and figure out how to import them at different times. It would be a cleaner way to import for sure if we could figure out how best to do it. Maybe we could add an additional tag to the junit and parse it out somehow. |
cbc29b6 to
4ae2c64
Compare
Ah I was just thinking sippy would know which tests it expects might have YAML output and try to unmarshal it (and fail quietly if it can't) |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin, stbenjam The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@dgoodwin: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
TRT-665
Begin extracting a little metadata from test flakes and failures on very specific set of backstop tests when we're importing job results. This will allow us to track trends in what alerts and pathological events specifically are causing problems over time.
Result is stored in a jsonb column:
And can be queried and grouped by normally: