Skip to content
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

ui: Link from statement diagnostics to details page #46923

Merged

Conversation

koorosh
Copy link
Collaborator

@koorosh koorosh commented Apr 2, 2020

Resolves: #46559

We have Statements diagnostics history page with list
of all requested diagnostics.
Before, statement fingerprint were represented as a
simple text and now it is a links to statement details
page.

One notion, that it is possible to have diagnostics for
statements which is already cleared. In this case
statement is displayed as a text instead of link.

Release note (admin ui change): Add links to statement
details from Statement Diagnostics history page.

Release justification: bug fixes and low-risk updates to new functionality

out

@koorosh koorosh requested a review from a team April 2, 2020 13:36
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@koorosh koorosh added the do-not-merge bors won't merge a PR with this label. label Apr 2, 2020
@koorosh
Copy link
Collaborator Author

koorosh commented Apr 2, 2020

We have Statements diagnostics history page with list
of all requested diagnostics.
Before, statement fingerprint were represented as a
simple text and now it is a links to statement details
page.

One notion, that it is possible to have diagnostics for
statements which is already cleared. In this case
statement is displayed as a text instead of link.

Release note (admin ui change): Add links to statement
details from Statement Diagnostics history page.

Release justification: bug fixes and low-risk updates to new functionality
Before, statements column on Statements diagnostics history page
displayed full statement and didn't have tooltips.

With current changes, statements column behave almost the same
way as on Statements page:
- statement is shortened
- tooltip is added with full statement
- in case shortened statement is the same as full statement -
then don't display tooltip at all.

Release note (admin ui change): Add tooltips with full length
statements on Statement diagnostics history page.

Release justification: bug fixes and low-risk updates to new functionality
@koorosh koorosh force-pushed the ui-statements-diagnostics-history--links branch from edbf98b to 6e210c3 Compare April 3, 2020 10:05
@koorosh
Copy link
Collaborator Author

koorosh commented Apr 3, 2020

Additional change is applied, now statements column looks and behave almost the same
way as on Statements page:

  • statement is shortened
  • tooltip is added with full statement
  • in case shortened statement is the same as full statement -
    then don't display tooltip at all.

out 12 46 08

@koorosh koorosh removed the do-not-merge bors won't merge a PR with this label. label Apr 3, 2020
@nathanstilwell nathanstilwell self-requested a review April 3, 2020 14:12
Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @koorosh and @nathanstilwell)


pkg/ui/src/views/reports/containers/statementDiagnosticsHistory/index.tsx, line 79 at r1 (raw file):

        return (
          <Link
            to={ `/statement/${implicitTxn}/${encodeURIComponent(query)}` }

Given the fact that this will be used outside of this repo, can we extract the creation of this link?

@koorosh
Copy link
Collaborator Author

koorosh commented Apr 9, 2020

can we extract the creation of this link

I would consider this as a separate more global task, probably in scope of routes and the way we provide/consume configuration.

Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

Appreciate the comment! We can address later.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @koorosh and @nathanstilwell)

@dhartunian
Copy link
Collaborator

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 9, 2020

Build succeeded

@craig craig bot merged commit cc7ce80 into cockroachdb:master Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui: Statement diagnostics history, link to statement details page
3 participants