Skip to content

roachtest: bugfix testeng grafana link missing cluster name#107984

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
annrpom:bugfix_granfana_link
Aug 2, 2023
Merged

roachtest: bugfix testeng grafana link missing cluster name#107984
craig[bot] merged 1 commit intocockroachdb:masterfrom
annrpom:bugfix_granfana_link

Conversation

@annrpom
Copy link
Copy Markdown
Contributor

@annrpom annrpom commented Aug 1, 2023

Previously, the testeng grafana link generated after a roachtest failure directed the user to cockroachlabs.com due to a missing cluster name from the link. This patch should fix this issue by getting the cluster name from a *githubIssues.cluster.name instead of the clusterName from roachtest/cluster.go.

Fixes: #107894

Release note (bug fix): The link to testeng grafana that is generated on a roachtest failure should now properly direct to grafana.

@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Aug 1, 2023

This change is Reviewable

@annrpom
Copy link
Copy Markdown
Contributor Author

annrpom commented Aug 1, 2023 •
edited
Loading

tl;dr: my guess is that the cause of this is related to the fact that clusters are wiped before and after each test runs: #22795 (?)
not entirely sure if i should've even used this clusterName in the first place~

I noticed that using clusterName defined in roachtest/cluster.go (implemented in the previously linked PR) will return the empty string due to the reason above; so, I believe using g.cluster.name as the argument to generateHelpCommand in createPostRequest will resolve this?

I did need to put a guard in generateHelpCommand that kinda goes with the logic of "if cluster creation fails, there will be no Grafana link"

  • this was to avoid a nil ref error in case we try to get the name of a cluster when cluster is nil + a way to avoid a faulty Grafana link that takes you to cockroachlabs.com
    • (is this intuitive or will people be confused as to why the Grafana link is missing sometimes?)

I committed some better tests (i.e. putting some echotests in TestCreatePostRequest) since I realized that testing how generateHelpCommand works inside of createPostRequest helped me through this process.

@annrpom annrpom mentioned this pull request Aug 1, 2023
Closed
@renatolabs
Copy link
Copy Markdown

renatolabs commented Aug 2, 2023

I believe using g.cluster.name as the argument to generateHelpCommand in createPostRequest will resolve this?

I believe so too.

this was to avoid a nil ref error in case we try to get the name of a cluster when cluster is nil + a way to avoid a faulty Grafana link that takes you to cockroachlabs.com

Makes sense to me.

(is this intuitive or will people be confused as to why the Grafana link is missing sometimes?)

I'd say it's good enough. In practice, the cluster reference should only be nil when cluster creation fails and those errors are aggregated in issues like #105298. For every actual test failure, the Grafana link should be there as per your current logic (and its absence should continue be treated as a bug).

renatolabs
renatolabs approved these changes Aug 2, 2023
View reviewed changes
Copy link
Copy Markdown

@renatolabs renatolabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing!

@annrpom annrpom marked this pull request as ready for review August 2, 2023 19:09
@annrpom annrpom requested a review from a team as a code owner August 2, 2023 19:09
@annrpom annrpom requested review from herkolategan and smg260 and removed request for a team August 2, 2023 19:09
@annrpom
roachtest: bugfix testeng grafana link missing cluster name
abd1e0d 
Previously, the testeng grafana link generated after a roachtest
failure directed the user to cockroachlabs.com due to a missing cluster
name from the link. This patch should fix this issue by getting the cluster
name from a `*githubIssues.cluster.name` instead of the `clusterName`
from roachtest/cluster.go.

Fixes: cockroachdb#107894

Release note (bug fix): The link to testeng grafana that is generated
on a roachtest failure should now properly direct to grafana.
@annrpom annrpom force-pushed the bugfix_granfana_link branch from 4e999e4 to abd1e0d Compare August 2, 2023 19:12
@annrpom
Copy link
Copy Markdown
Contributor Author

annrpom commented Aug 2, 2023

TFTR! ('-')7

bors r=renatolabs

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 2, 2023

Build succeeded:

@craig craig bot merged commit 46255c8 into cockroachdb:master Aug 2, 2023
@annrpom annrpom deleted the bugfix_granfana_link branch September 15, 2025 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@herkolategan herkolategan Awaiting requested review from herkolategan herkolategan is a code owner automatically assigned from cockroachdb/test-eng

@smg260 smg260 Awaiting requested review from smg260 smg260 is a code owner automatically assigned from cockroachdb/test-eng

1 more reviewer

@renatolabs renatolabs renatolabs approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

roachtest: grafana link does not have cluster name

3 participants

@annrpom @cockroach-teamcity @renatolabs