[k7/APM] Make breadcrumbs more consistent

[spalger]

See the #25884 for more general information.

APM breadcrumbs should be updated to follow the guidelines outlined in #25689 by making a few changes:

• only pass strings to EUI or we get inaccessible title attributes
  
• Remove the "request", "errors", and "Transations" breadcrumbs. The breadcrumb hierarchy should reflect the page hierarchy, and tabs don't count as "pages" in this sense, so the breadcrumbs should be something like:
  • APM - Services/Traces tabs
  • APM / {serviceId} - Service landing page, including tabs for Requests and Errors, tabs don't create breadcrumbs
  • APM / {serviceId} / {requestId/errorId} Request/Error detail page

Since I'm pretty unfamiliar with the APM code base can someone from the APM team handle these changes?

[elasticmachine]

Pinging @elastic/apm-ui

[makwarth]

Great initiative. Thanks for the ping @spalger. Had a quick look, and this change (the updated breadcrumb rules) should work well for APM as is. Target deadline is alpha2?

[sorenlouv]

@spalger

If your app already shows its own breadcrumbs you should conditionally render them by checking the k7Design uiSetting. When this setting is true you should stop rendering the breadcrumbs yourself and instead pass breadcrumbs to the BreadcrumbState module described above.
#25884

This applies to us. So should we have two breadcrumb implementations? Our own, and then the Kibana implementation? Why not switch over to the Kibana implementation entirely?

Btw. I couldn't find any mention of any BreadcrumbState module in the issue but I assume you mean the chrome.breadcrumbs API, right?

[spalger]

@makwarth yep, aiming for alpha2

@sqren Yes to both questions. We're aiming to support both because we're going to be releasing both 6.x and 7.0 prereleases for a little while. In that time we want to make sure that code developed on master can be easily backported to 6.x and that testing on master accurately reflects the version that features will ship in, which in most cases is 6.x. Supporting both headers also means that switching to the 7.0 header in master is just a uiSetting away, giving the best of both world. Once we get close to shipping 7.0 GA and stop active development on 6.x we will be able to remove support for the old navigation style.

[jasonrhodes]

We are actually treating tabs more and more like pages in APM UI. For example, we're building /services and /traces routes in #23824 and transactions/errors already work this way. And if you are on an error group page, I think it makes sense to have an "Errors" breadcrumb that, when clicked, takes you back to the service page with the Errors tab active.

I think our breadcrumbs would then be:

• APM / services
• APM / traces
• APM / services / :serviceName / transactions
• APM / services / :serviceName / errors
• APM / services / :serviceName / (transactions | errors) / :groupName (transaction group or error group)

[spalger]

@jasonrhodes would you mind making that argument in #25689? I'm just implementing the design we're trying to implement across the board for consistency and "tabs aren't pages" is pretty explicitly called out there.

CC: @cchaos

[jasonrhodes]

@spalger gotcha, will speak up there thanks

[makwarth]

@jasonrhodes Good point. I missed that the second level (/transactions or /errors in our case) would be omitted here. That will make navigation hard.

One note:
Shouldn't APM / services / :serviceName / be APM / :serviceName / ?

[jasonrhodes]

@makwarth yeah, we just started discussing this yesterday, so I just threw together a summary of what we discussed in the APM repo for reference and discussion :) https://github.com/elastic/apm/issues/20

[makwarth]

@jasonrhodes Thanks. That makes sense, but we don't need /services/ in the breadcrumbs, do we? I'm mindful of our long breadcrumbs as is.

[sorenlouv]

APM / services / :serviceName / (transactions | errors) / :groupName (transaction group or error group)

fyi: There is also transactionType:
APM / services / :serviceName / transactions / :transactionType / :transactionGroupName

You can see an example in the cloud data:
<host>/app/apm#/opbeans-python/transactions/celery/opbeans.tasks.sync_orders

[spalger]

Hey folks, did you get a chance to see the response in #25689 (comment)? Would like to get the breadcrumb changes in for alpha 2 if possible, if no one is available to work on them this week I can take a stab at it.

[jasonrhodes]

@spalger the changes for APM breadcrumbs may not be super simple, as I understand it, because we are using react-router-breadcrumbs-hoc which interprets them directly from the routing config. If we need to skip over parts of the routing config on a per page basis, we need to rethink all of that, and it will probably be somewhat coupled to some path/navigation/page changes we are planning for 7.0 -- that's why we thought punting to beta-1 made the most sense, if it's possible to wait for that?

If not, let's maybe do a call at some point this week to talk through what's involved with creating more custom breadcrumbs that don't rely on routing config.

[spalger]

Alright, since the APM breadcrumbs are pretty usable I think it'd be fine to let them slip to beta, but if you have some time today I'd be happy to take a look and see if we can find a simple way to integrate the changes for alpha 2

[formgeist]

I've been taking a look at how we might solve this, or at least throw this in for discussion, when taking into consideration the layout changes we'll make for K7 conversion anyhow.

There's still this quirk that as soon as you choose a trace you're in transactions, and there's no quick way back to the list (not considering the back link in the browser). You'll have to return to the APM homepage, and navigate to the Traces list again.

Thoughts?

Updated: Added link to Marvel prototype of the main screens in APM so you can try the navigation experience (more or less).

---

Marvel prototype of navigation

APM "Home" (Services and Traces lists)

Breadcrumb: APM

Service - Transactions list

Breadcrumb: APM / Service name

Service - Transaction detail

Breadcrumb: APM / Service name / Transaction name

Note: Adds a link back to the Transactions list in the top by the page heading.

Service - Errors list

Breadcrumb: APM / Service name

Service - Error detail

Breadcrumb: APM / Service name / Error group name (shortened)

Note: Adds a link back to the Errors list in the top along with option to move back and forward in the list from the detail page itself.

[makwarth]

Hey @formgeist, finally had time to run through these. I still find it annoying not to be able to go back to the main list views (transactions, errors, metrics). Have you discussed this with the design team? Having one more level in the breadcrumbs would (partly) fix this, and there's so much room :)
There is one other issue with transaction types though: If you go to transactions, then filter by a custom transaction type: How would the breadcrumb with the extra level work? Would you get back to transactions (showing Requests on default), or would the session remember to go back to the custom transaction type list?

Other comments:

• Error detail page: Error occurrence id is now missing (we can just add to the ui)
• Transaction detail page: Transaction type is missing (we can just add to the ui)
• I agree regarding the Services / Traces issue. We could investigate merging those tabs and have services in the left column, and traces in the right column. If it works, that would be a neat overview, but it's not a viable solution for the breadcrumbs issue as we'll like introduce more tabs at the service/traces level later.

[formgeist]

Thanks for the feedback - here's a few comments;

I still find it annoying not to be able to go back to the main list views (transactions, errors, metrics). Have you discussed this with the design team?

I believe the main suggestion was to provide a link on the detail pages to go back to the list views i.e. "View all transactions". Another, but more cumbersome approach, would be to move to sidebar navigation as this provides a layout for adding filters for the list views as well. I have a concept design in the works, which we can take a look at soon. Probably not something we'd invest in for now, but I had some examples of adding a link back on the content page.

Having one more level in the breadcrumbs would (partly) fix this, and there's so much room :)

Agreed, but I also understand how our tabs navigation shouldn't be page navigation per se, so "bending" the guidelines is probably frowned upon from the Kibana design team.

There is one other issue with transaction types though: If you go to transactions, then filter by a custom transaction type: How would the breadcrumb with the extra level work? Would you get back to transactions (showing Requests on default), or would the session remember to go back to the custom transaction type list?

I assumed we'd need to save state on the list pages in order for the "go back" browser history link to work as the user would expect it. Haven't discussed the implications with the UI team about this.

Error detail page: Error occurrence id is now missing (we can just add to the ui)
Transaction detail page: Transaction type is missing (we can just add to the ui)

Agreed, both of those could easily be added the UI.

I agree regarding the Services / Traces issue. We could investigate merging those tabs and have services in the left column, and traces in the right column. If it works, that would be a neat overview, but it's not a viable solution for the breadcrumbs issue as we'll like introduce more tabs at the service/traces level later.

IMO gets pretty messy quite quickly with the information we'd want in those tables. I still think the two are separate gateways to the underlying data and they should present different data based on their use-cases. I'm sure we'll start adding charts and more to those views soon, which isn't really scaleable if they're all on the same page.

[sorenlouv]

I assumed we'd need to save state on the list pages in order for the "go back" browser history link to work as the user would expect it. Haven't discussed the implications with the UI team about this.

If we have a "go back" link we can easily link to the correct transaction type

[makwarth]

Another, but more cumbersome approach, would be to move to sidebar navigation as this provides a layout for adding filters for the list views as well.

That's interesting. A similar approach could be to elevate the tabs, so they're present on all transaction/error/metric pages and sub-pages. Does that make sense? I think we've discussed this earlier, but unsure if we came to any pros/cons. One con is that it'll take some space at the top of the page, but I think that's fixable.

[formgeist]

@makwarth I think elevating the tabs above the page title is a no-go in K7 design, where the intention to always have the page title be present in the top.

I've made an updated proposal where we have "back" links below the query bar (in place of the tab navigation on the list pages) so navigation is consistency in that space. I've also added some badges for the transaction type. Back links also present on the Errors detail page.

Put together a Marvel prototype to show the flow (there might be inconsistencies with the list views if you go back from i.e. a page load).

---

Transaction detail page

Another transaction type example

Errors detail page

There's some prettification of the transaction type in those links that'd be nice to do, but not critical IMO, i.e. capitalization request or page loads in both the link and the badge.

Thoughts on this approach? cc @cchaos @snide for input reg. K7 design guidelines

[makwarth]

@formgeist

where the intention to always have the page title be present in the top.

the tabs whouldn't be over the page title? The tabs would be under the page title, and over the query bar. Does that make sense?

[formgeist]

@makwarth Ah, I must've misunderstood your suggestion. Still conflicts with how the tabs are intended. In this case the best we can do is to say the tabs stay on detail pages, but none of the items are selected (because you're no longer in that view) i.e.;

This seems pretty broken at least to me.

[cchaos]

@formgeist Honestly, whatever makes sense for APM. We hope all apps would be able to follow the breadcrumb guidelines, #25689, but we also know that each app is unique. Your mocks make sense to me but I don't know your space that well.

[formgeist]

@cchaos Thanks for the feedback - for now we're going to disable the old breadcrumb and go with what's in the new. We have direct URL for those "tabbed" views, which in the future might be replaced with a better navigation. As @jasonrhodes already referenced, this is our implementation to remove the existing breadcrumb #29286