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: show metric on charts tooltip #111469

Merged
merged 1 commit into from Oct 2, 2023
Merged

Conversation

maryliag
Copy link
Contributor

To make it easier to identify the metric being used to generate charts, this commit adds the metric to the tooltip of all charts on the Metrics page.

Fixes #109277

This also fix the metric name for Schema Registry Registrations.

Fixes #108095

Release note (ui change): On the Metric page, now the information about which metric is used to create each chart is available on the chart's tooltip.

Release note (bug fix): Fix metric name for Schema Registry Registrations.

@maryliag maryliag added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Sep 28, 2023
@maryliag maryliag requested review from koorosh and a team September 28, 2023 23:40
@maryliag maryliag requested a review from a team as a code owner September 28, 2023 23:40
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@koorosh koorosh left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

a discussion (no related file):
I have concern about manual updating tooltips to match provided metric names.
Metric name is already provided to component so it can be reused.
I would suggest something like this:

  • extend props with if showMetricsInTooltip bool prop;
  • add following logic to InternalLineGraph#render method:
render() {
    const {
      title,
      subtitle,
      data,
      tenantSource,
      preCalcGraphSize,
      legendAsTooltip,
      showMetricsInTooltip,
    } = this.props;
    let tooltip = this.props.tooltip;
    // Extend tooltip to include metrics names
    if showMetricsInTooltip {
      if (this.metrics.length == 1) {
        tooltip = (
          <>
            {tooltip}
            <br />
            <br />
            Metrics: {this.metrics[0].name}
          </>
        )
      } else {
        tooltip = (
          <>
            {tooltip}
            <br />
            <br />
            Metrics:
            <ul>
              {this.metrics.map(m => (<li key={m.name}>{m.name}</li>))}
            </ul>
          </>
        )
      }
    }
    ...

Copy link
Collaborator

@koorosh koorosh left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 14 files at r1, 14 of 14 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @maryliag)

@maryliag
Copy link
Contributor Author

maryliag commented Oct 1, 2023

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 1, 2023

Build failed:

To make it easier to identify the metric being used to generate
charts, this commit adds the metric to the tooltip of all charts
on the Metrics page.

Fixes cockroachdb#109277

This also fix the metric name for `Schema Registry Registrations`.

Fixes cockroachdb#108095

Release note (ui change): On the Metric page, now the information
about which metric is used to create each chart is available on the chart's
tooltip.

Release note (bug fix): Fix metric name for `Schema Registry Registrations`.
@maryliag
Copy link
Contributor Author

maryliag commented Oct 2, 2023

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 2, 2023

Build succeeded:

@craig craig bot merged commit 7e93ad5 into cockroachdb:master Oct 2, 2023
8 checks passed
@blathers-crl
Copy link

blathers-crl bot commented Oct 2, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 3803d12 to blathers/backport-release-23.1-111469: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@maryliag maryliag deleted the metric-charts branch October 3, 2023 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dbconsole: display query in metric graphs ui: "Schema Registry Registrations" chart broken
3 participants