cockroachdb / cockroachdb/cockroach
issues: add new Diagnostics parameter in issues.Post
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
Follow Up to observations noted here: https://github.com/cockroachdb/cockroach/pull/153962#discussion_r2466328079
Roachtest uses `pkg/cmd/bazci/githubpost/issues` to handle formatting and posting new issues and commetns to github via `issues.Post` in `pkg/cmd/bazci/githubpost/issues/issues.go`.
```
// In Roachtest
github := &githubIssues{
disable: runner.config.disableIssue,
dryRun: runner.config.dryRunIssuePosting,
issuePoster: issues.Post,
teamLoader: team.DefaultLoadTeams,
}
```
Roachtest creates an `issues.PostRequest` with `Message` and `ExtraParams` which gets passed in `issues.Post`.
```
Message string
// ExtraParams contains the parameters to be included in a failure
// report, other than the defaults (git branch, test flags).
ExtraParams map[string]string
// A path to the test artifacts relative to the artifacts root. If nonempty,
// allows the poster formatter to construct a direct URL to this directory.
```
So previously, for roachtest this message was just a string representation of an error. i.e.
TODO insert example.
The following enhancements were made to improve the triaging experience by adding more information to the created github issue in the form of additional logs and or cluster node ip information.
https://github.com/cockroachdb/cockroach/issues/147360
https://github.com/cockroachdb/cockroach/pull/151850
https://github.com/cockroachdb/cockroach/issues/138356
https://github.com/cockroachdb/cockroach/pull/153962
To achieve this, the `message` string was simply appended to with this information in roachtest in `test_runner.go`
```
// getGithubMessage abstracts details on how the Github message is constructed
// TODO(wchoe): Currently, all information we want to include in the Github
// message must be passed in as a single string. This is not ideal, especially
// as we add more information or want to format based on the failure or what
// information is available. #157021 aims to address this.
func (t *testImpl) getGithubMessage(failureMsg string) string {
githubMsg := failureMsg
if githubFatalLogs := t.getGithubFatalLogs(); githubFatalLogs != "" {
githubMsg = fmt.Sprintf("%s\n%s", githubMsg, githubFatalLogs)
}
if githubIpToNodeMapping := t.getGithubIpToNodeMapping(); githubIpToNodeMapping != "" {
githubMsg = fmt.Sprintf("%s\n%s", githubMsg, githubIpToNodeMapping)
}
t.L().Printf("github message: %s", githubMsg)
return githubMsg
}
```
This is brittle / not the best for a few reasons outlined here: https://github.com/cockroachdb/cockroach/pull/153962#discussion_r2466328079
* For each additional piece of information we want to pass along, we are creating a complicated "demultiplexing" scenario in when we are formatting the github issue body i.e. each case of information being included / not included needs to be accounted for
* order matters
* complicated regex strings
* complicated control logic in the `issues` renderer
A better solution would be to pass along a map i.e. `Diagnostics map[string]string` that would have fatal node logs and ip node info as keys. Then in `issues`, each kv pair could be handled modularly instead of in a single long if else control statement.
Also calling out `ExtraParams` because that seems to be similar to what we want. I think a separate field should be added though because `ExtraParams` and the "diagnostic information" serve different purposes.
If we rely on key names, we could eliminate the complex regex dependency (especially for the ip node table) or move that check to somewhere in roachtest.
More information on the path of message in `issues`.
The journey of message in `issues`.
`issues.PostRequest.Message` and `issues.PostRequest.ExtraParams` gets processed and added to fields in `issues.TemplateData` struct i.e. `issues.TemplateData.CondensedMessage` and `issues.TemplateData.Parameters`
```
// TemplateData is the input on which an IssueFormatter operates. It has
// everything known about the test failure in a predigested form.
type TemplateData struct {
// Parameters includes relevant test or build parameters, such as
// build tags or cluster configuration
Parameters map[string]string
// The message, garnished with helpers that allow extracting the useful
// bots.
CondensedMessage CondensedMessage
}
```
Then data, which is an instance of `TemplateData`, get's passed into `formatter.Body`. `formatter` is a passed in implementation of the `IssueFormatter` interface. The `unitTestFormatterTyp` interface implementation's `IssueFormatter.Body` is called and this is what creates the rendered github issue message body.
```
r := &Renderer{}
if err := formatter.Body(r, data); err != nil {
// Failure is not an option.
_ = err
fmt.Fprintln(&r.buf, "\nFailed to render body: "+err.Error())
}
```
Jira issue: CRDB-56351
Contributor guide
Research direction
Start with pkg/cmd/bazci/githubpost/issues/issues.go and test_runner.go to trace PostRequest.Message and ExtraParams into TemplateData and formatter.Body. Follow the unitTestFormatterTyp implementation and the existing diagnostic formatting paths. Done means Diagnostics can carry fatal logs and IP-to-node information through this flow without the current message concatenation and regex-based demultiplexing.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- tooling
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100