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 clusterviz: naming things and URLs #19643

Closed
couchand opened this issue Oct 30, 2017 · 20 comments
Closed

ui clusterviz: naming things and URLs #19643

couchand opened this issue Oct 30, 2017 · 20 comments
Assignees
Milestone

Comments

@couchand
Copy link
Contributor

couchand commented Oct 30, 2017

Currently, the time series pages are titled "Cluster Overview", and are located (along with nodes and events lists) under the path /cluster. Given the upcoming introduction of the cluster viz page, these names are likely to be confusing. The paths might be fine (if the cluster viz just becomes the root of /cluster, say), but there might be value in thinking about them as well.

What names should be applied for:

  • the cluster viz
    • page title
    • sidebar link text
    • sidebar link icon
    • path
  • the time series pages
    • page titles
    • sidebar link text
    • sidebar link icon
    • paths
@dianasaur323
Copy link
Contributor

Hmm... the annoying thing about all this is that we actually do usage tracking based on URLs. Let me check with our metrics platform to see if they have suggestions. Regardless, even if this isn't backwards compatible, we should still fix it to make it make more sense. How about:

  • Cluster Viz (remains /cluster -> I would also be okay with /overview)
    • Page Title: Cluster Overview (seems fine to me)
    • Side Bar: Overview (?)
    • Sidebar link icon @kuanluo
    • Path: /cluster/overview -> I think we can get rid of the all?
      • /cluster/overview/locality1/locality2/nodes
      • /cluster/overview/map/locality1/locality2/nodes
  • Time Series (becomes /metrics/all/overview) -> and just keep the paths as we have them
    • Page Title: Cluster Metrics (?)
    • Side Bar: Metrics
    • Side bar link icon @kuanluo
    • Paths - lets just keep everything as is?

Also, it looks like /cluster/nodes will still be around, and can be navigated to form our new cluster overview page? We might need to iterate on this...

@kuanluo
Copy link

kuanluo commented Nov 1, 2017

  • Cluster viz -> Overview for sure
  • Time Series -> Monitor?

@couchand
Copy link
Contributor Author

couchand commented Nov 1, 2017

cc @mrtracy @Amruta-Ranade @vilterp in case any of you have thoughts on these questions.

@Amruta-Ranade
Copy link
Contributor

+1 for Metrics

@couchand
Copy link
Contributor Author

couchand commented Nov 1, 2017

For context, here is our current route structure:

/ - redirects to /cluster/all/overview
  /cluster - redirects to /cluster/all/overview
    /all/:dashboardName
    /node/:nodeId/:dashboardName
    /nodes
      /:nodeId
        /logs
  /databases - redirects to /databases/tables
    /tables
    /grants
    /database/:databaseName/table/:tableName
  /jobs
  /raft - redirects to /raft/ranges
    /ranges
    /messages/all
    /messages/node/:nodeId
  /clusterviz
  /debug
  /reports
    /problemranges
      /:nodeId
    /network
    /nodes
    /certificates/:nodeId
    /range/:rangeId
      /cmdqueue

Parent routes without their own entry on the above table (like /raft/messages) are currently broken links, which separately is something we should fix (probably just by adding redirects).

@Amruta-Ranade
Copy link
Contributor

Can we have /cluster/all/visualization or something similar for the cluster viz, and retain all existing ones?

@dianasaur323
Copy link
Contributor

dianasaur323 commented Nov 1, 2017

@Amruta-Ranade my concern with sticking with /cluster/ is that it seems to conflict with the side bar? we currently have a parent path for each side bar icon. I could also envision that we would eventually add an "admin" or "config" side bar, and that might be weird to crush everything under "cluster"? Although that would be the best option for us in terms of analytics.

@Amruta-Ranade
Copy link
Contributor

Hmm..then I would vote for Metrics.

But why are we changing the parent path for the sidebar icons? 🤔
And even if we do add admin and config, they are still applicable to "cluster", right? (Administer a cluster and configure a cluster).

@dianasaur323
Copy link
Contributor

hmm.. that's true. I just personally thought it was easier to reason about - basically something only gets a side bar if it is important enough to be a parent. But I honestly don't feel too strongly about it, and would defer to @couchand

@couchand
Copy link
Contributor Author

couchand commented Nov 1, 2017

But what isn't applicable to the cluster? Couldn't "databases" or "jobs" be nested under "cluster", too? What I don't like about the name "cluster" is that it doesn't provide any selectivity, so it doesn't tell the user what's there at all.

I like the organizing principle of having one top-level route for each icon in the sidebar, it provides an easy rule that seems to make sense for primary navigation. I also think the title of the link should be the same as the path, unless there's some compelling reason not to.

We should probably maintain redirects for all pages that we move, at least for one release cycle (and maybe forever), in case anyone has links around.

Regarding analytics, there should be a way to bucket pages that moved, so we shouldn't have to lose the data.

@dianasaur323
Copy link
Contributor

yes! sounds good to me. The one thing to note is that we can't use /cluster/all/overview, we have to use /cluster/overview, since /cluster/all/overview will cause an overlap with the old metrics page. Other than that, we can bucket. It will be obnoxious, but can be done!

@couchand
Copy link
Contributor Author

couchand commented Nov 1, 2017

Ok, having read all the feedback, here's a proposal for the new route structure. Only regular user-facing pages are listed here.

  • / - redirect to /overview
    • /overview - cluster viz landing page. always shows the overview and node list, open source version has no node canvas, ccl version shows the top-level localities of the cluster on the node canvas.
      • /:localityTag/:localityTag/:localityTag/... - ccl only, shows the cluster viz page for the specified locality. Tags must nest correctly or it redirects to /overview.
    • /metrics - redirects to /metrics/overview
      • /:dashboardName - redirects to /metrics/:dashboardName/cluster
        • /cluster - the current /cluster/all/:dashboardName
        • /node - redirects to /metrics/:dashboardName/cluster
          • /:nodeId - the current /cluster/node/:nodeId/:dashboardName
    • /nodes - the page that's currently /cluster/nodes. We could add a sidebar link if we want.
    • /node - redirects to /nodes
      • /:nodeId
        • /logs
    • /databases - redirects to /databases/tables
      • /tables
      • /grants
    • /database - redirects to /databases
      • /:databaseName - a new page listing the tables in a database, or possibly just redirects to /databases
        • /table - redirects to /database/:databaseName
          • /:tableName
    • /jobs

Some questions:

  • Does the rule that every sidebar link gets a top-level route imply the converse - does every top-level route need a sidebar link?
  • How do we handle aggregates/list views vs. detail views? There's inconsistency in the existing routes. Compare /cluster/nodes > /cluster/nodes/:nodeId with /databases/tables > /databases/database/:databaseName/table/:tableName. GitHub, who does a good job naming their routes, has a similar inconsistency - see /issues > /issues/1234 with /pulls > /pull/1234. Note that I've moved /database up a level to become a sibling to /databases, and added a route for /node as a sibling of /nodes.
  • Note that I've inverted the role of the dashboard name and the node vs. cluster distinction. I think this makes more sense, as this matches how I think about navigating the time series dashboards. What is the right order of information here?

Commence bikeshedding!

@dianasaur323
Copy link
Contributor

dianasaur323 commented Nov 1, 2017

I'm a big fan of pulling nodes out. That always confused me.

Re bullet 1, I think not. I don't think every top-level route needs a sidebar link at this point. I actually envision nodes to eventually fall under something like "admin" or "configs" since we should probably be displaying more config info on those pages anyway. I think it makes sense to invert node / cluster distinction.

Last note, we'll probably have to add that "ugly" zone config page I talked about a while back (showing which configs you have right now, and potentially which ranges and tables map to those configs). Where would that fit in here?

And I'm sorry, I'm not very good at bikeshedding :(

@couchand
Copy link
Contributor Author

couchand commented Nov 6, 2017

The question came up on #19850 what the fallback behavior should be in case a user of the OSS build tries a route that's only in the CCL version. My recommendation (alluded to above, but not made explicit) is that it just redirects to a nearby valid page if one makes sense.

An alternative would be a specific 404 page mentioning that the feature is only available with an enterprise license, with a link to https://www.cockroachlabs.com/product/cockroachdb-enterprise/

@dianasaur323
Copy link
Contributor

I'm in the camp of your second proposal. That being said, I was actually thinking of doing a mini-kick-off to discuss what the user flow should look like for an OSS user hitting enterprise pages (either when their license expires or if they don't have a license to begin with). I was planning on setting something up next week once we have a final decision on exactly what the license flow will look like. If you're blocked on this though, we could do it earlier since I don't think the flow is dependent on what license we give users.

@dianasaur323
Copy link
Contributor

dianasaur323 commented Nov 9, 2017

Just a note that we should fix this before "shipping" the cluster overview. We also want to make sure to update the side navigation titles, namely from "cluster" to "metrics"

@couchand
Copy link
Contributor Author

Ok, I'm taking ownership of this issue, I'll implement the route structure in my last comment and update the side navigation, then close this.

@dianasaur323 maybe you want to take note of some of the less actionable discussion here to transfer to other relevant issues (like the Cluster Viz/CCL/licensing/etc)?

@couchand couchand assigned couchand and unassigned dianasaur323 Nov 13, 2017
@dianasaur323
Copy link
Contributor

I think we can use this issue to track the CCL stuff: #19759. Added in some additional context from the comments.

couchand added a commit to couchand/cockroach that referenced this issue Dec 13, 2017
Contributes to cockroachdb#19643.

Release note: none.
@couchand couchand modified the milestones: 2.0, Later Jan 29, 2018
@couchand couchand modified the milestones: Later, 2.0 Jan 30, 2018
@Amruta-Ranade
Copy link
Contributor

Can we/did we rename "Cluster Overview" on time series page to "Cluster Metrics"?

@couchand
Copy link
Contributor Author

@Amruta-Ranade that sounds consistent with the above plan.

Also see #18956

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

No branches or pull requests

4 participants