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: add a metrics dashboard for changefeeds #34427

Merged
merged 1 commit into from Jan 31, 2019

Conversation

danhhz
Copy link
Contributor

@danhhz danhhz commented Jan 30, 2019

These metrics have proven useful for introspecting changefeed behavior,
so adding them as a dashboard in case any users hit something that needs
debugging. Also will be useful for users to see what's going on in the
cluster's changefeeds.

Release note (admin ui change): CHANGEFEED metrics are now exposed in
the UI.

@danhhz danhhz requested a review from a team as a code owner January 30, 2019 21:06
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@danhhz
Copy link
Contributor Author

danhhz commented Jan 30, 2019

This is all entirely out of my wheelhouse and I entirely cargo-culted it from other dashboards, so definitely do a careful review, please.

The graph names and labels could probably use some wordsmithing (cc @rolandcrosby maybe?). Also a scrollbar is showing up in the dropdown now. I tried to look into it, but couldn't get the dropdown to stay open while i chrome inspector'd it and figured it'd be easier to ask.

screen shot 2019-01-30 at 12 57 55 pm

import { GraphDashboardProps } from "./dashboardUtils";

export default function (props: GraphDashboardProps) {
const { storeSources } = props;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indent with 2 spaces (sadly we don't have an autoformatter configured)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@rolandcrosby
Copy link

It's great to have a first stab at this -- thanks Dan! A few notes:

  • What are the meanings of the different timings? I'd suggest different wording but I'm not sure what I'm looking at. The wording on the other two graphs is fine.
  • Meta/docs issue: we should make sure that important changefeed metrics are documented (as in what they mean) and listed in our guides to setting up monitoring/Prometheus.
  • How does this behave when there are multiple changefeeds? Are all the numbers aggregated across all changefeeds?
  • Is there an easy way that we could add a table listing the currently active changefeeds? I know that information should be available on the jobs page but this might also be a good place to surface it.

@danhhz
Copy link
Contributor Author

danhhz commented Jan 30, 2019

@vilterp Any thoughts on what to do about this?

screen shot 2019-01-30 at 3 33 09 pm

@vilterp
Copy link
Contributor

vilterp commented Jan 30, 2019

Hm, I don't remember what we did last time. Any prop on the dropdown that just lets you make it taller without having to scroll?

@danhhz
Copy link
Contributor Author

danhhz commented Jan 30, 2019

@rolandcrosby

What are the meanings of the different timings? I'd suggest different wording but I'm not sure what I'm looking at. The wording on the other two graphs is fine.

Gotcha, let's sync offline on the timings names. I agree they're not great, but I've thought about it and don't have anything better yet.

Meta/docs issue: we should make sure that important changefeed metrics are documented (as in what they mean) and listed in our guides to setting up monitoring/Prometheus.

Changefeeds and admin ui actually have different docs writers, so first step is figuring out who 😀

How does this behave when there are multiple changefeeds? Are all the numbers aggregated across all changefeeds?

All aggregated. It's too expensive to store this per-changefeed, unfortunately.

Is there an easy way that we could add a table listing the currently active changefeeds? I know that information should be available on the jobs page but this might also be a good place to surface it.

I was able to pattern match this page, but that would likely require some original work (and visual design). If it falls to me to do it, I don't think it'll make the release.

These metrics have proven useful for introspecting changefeed behavior,
so adding them as a dashboard in case any users hit something that needs
debugging. Also will be useful for users to see what's going on in the
cluster's changefeeds.

Release note (admin ui change): `CHANGEFEED` metrics are now exposed in
the UI.
@rolandcrosby
Copy link

For 2.2 we don't need to add anything besides the timeseries graphs to this page. We can revisit in a future release. Aside from the naming changes we discussed offline this LGTM.

@danhhz
Copy link
Contributor Author

danhhz commented Jan 31, 2019

@rolandcrosby updated based on our convo, feel pretty good about the structure. Happy to push a wordsmithing PR if any changes fall out of documenting it.

screen shot 2019-01-31 at 2 41 45 pm

@danhhz
Copy link
Contributor Author

danhhz commented Jan 31, 2019

Thanks for the reviews!

@vilterp and I dug into the scrollbar in the dashboard dropdown but didn't find anything yet. He gave me the go ahead to merge this as-is and get it in the monday alpha, but I'll file an issue to revisit. (And @piyush-singh, he wanted me to give you a heads up about this.)

bors r=vilterp,rolandcrosby

craig bot pushed a commit that referenced this pull request Jan 31, 2019
34427: ui: add a metrics dashboard for changefeeds r=vilterp,rolandcrosby a=danhhz

These metrics have proven useful for introspecting changefeed behavior,
so adding them as a dashboard in case any users hit something that needs
debugging. Also will be useful for users to see what's going on in the
cluster's changefeeds.

Release note (admin ui change): `CHANGEFEED` metrics are now exposed in
the UI.

Co-authored-by: Daniel Harrison <daniel.harrison@gmail.com>
@craig
Copy link
Contributor

craig bot commented Jan 31, 2019

Build succeeded

@craig craig bot merged commit 0326b5e into cockroachdb:master Jan 31, 2019
@danhhz danhhz deleted the cdc_adminui branch January 31, 2019 23:53
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.

None yet

4 participants