[APM] Persist local UI filter across views #43988
Conversation
|
Btw, I think adding this makes #43339 even more important, I think it would make the experience a lot better with persistent local UI filters. |
|
@sqren should we do breadcrumbs? I don't see an easy way out there to be honest. |
💚 Build Succeeded |
| @@ -33,20 +38,31 @@ export function ServiceDetailTabs({ urlParams }: Props) { | |||
| } | |||
| }, [serviceName, start, end]); | |||
|
|
|||
| if (!serviceName) { | |||
| // this never happens, urlParams type is not accurate enough | |||
There was a problem hiding this comment.
Thanks for adding this. We can probably solve this later with some io-ts at the route boundary.
x-pack/legacy/plugins/apm/public/components/app/ServiceDetails/ServiceDetailTabs.tsx
Show resolved
Hide resolved
x-pack/legacy/plugins/apm/public/components/shared/HistoryTabs/index.tsx
Show resolved
Hide resolved
| padding: ${px(unit * 0.75)} ${px(unit)}; | ||
| ${({ isSelected }) => | ||
| !isSelected ? `color: ${theme.euiTextColor} !important;` : ''} | ||
| } |
There was a problem hiding this comment.
Overriding EuiTabLink seems wrong - especially with !important. Although you probably have good reasons. Can you add a comment why we don't use EuiTab vanilla and why it's not possible to configure it via props (eg. it has a size property that is probably similar to setting padding)
There was a problem hiding this comment.
Also, we should tag the EUI team if we think some config options are missing in EuiTab
There was a problem hiding this comment.
Added a comment. Should we pull in EUI as well to get some kind of support for links in tabs?
There was a problem hiding this comment.
Should we pull in EUI as well to get some kind of support for links in tabs?
Would be great if you can repro the problem in a codesandbox and create an issue in EUI. Then link to the issue from the comment, so we can remove the workaround when that is fixed.
x-pack/legacy/plugins/apm/public/components/shared/Links/apm/ErrorOverviewLink.tsx
Show resolved
Hide resolved
x-pack/legacy/plugins/apm/public/components/shared/Links/apm/TransactionOverviewLink.tsx
Outdated
Show resolved
Hide resolved
| @@ -7,6 +7,8 @@ | |||
| import React from 'react'; | |||
| import { Transaction } from '../../../../../typings/es_schemas/ui/Transaction'; | |||
| import { APMLink } from './APMLink'; | |||
| import { useUrlParams } from '../../../../hooks/useUrlParams'; | |||
| import { pickKeys } from '../../../../utils/pickKeys'; | |||
|
|
|||
| interface TransactionLinkProps { | |||
| transaction: Transaction | undefined; | |||
There was a problem hiding this comment.
TransactionLink is a bit different than all the others because it takes a Transaction as argument. Did you consider changing that to individual params like:
const TransactionLink = ({
serviceName,
traceId,
transactionId,
transactionName,
transactionType
}) => { ... }There was a problem hiding this comment.
Yeah, I think flattening those is an improvement, will change 👍
I think it's fine to defer breadcrumbs for now. |
There was a problem hiding this comment.
This looks really good 👍
After this PR, do we still use APMLink directly, or only the specialized link components? Would be great if we got rid of all direct usages of APMLink since it would make it much simpler to update link structures if they are all located in the same folder (links/apm/{specialized-component}).
I think we can do that yeah, we need some other links like HomeLink and SettingsLink but it should be manageable. Do we have situations where we link internally with anything other than an APMLink component? Could not find one so quickly, but maybe you know one. |
dgieselaar
force-pushed
the
persist-local-filters
branch
from
4f0c5dd to
e34e06d
Compare
| @@ -38,9 +37,9 @@ describe('ErrorGroupOverview -> List', () => { | |||
| it('should render empty state', () => { | |||
| const storeState = {}; | |||
| const wrapper = mount( | |||
| <MemoryRouter> | |||
| <MockUrlParamsProvider> | |||
There was a problem hiding this comment.
this change was necessary because List now throws if no serviceName is defined in urlParams
| @@ -112,8 +112,8 @@ export async function getErrorGroups({ | |||
| return { | |||
| message, | |||
| occurrenceCount: bucket.doc_count, | |||
| culprit: idx(source, _ => _.error.culprit), | |||
| groupId: idx(source, _ => _.error.grouping_key), | |||
| culprit: source.error.culprit, | |||
There was a problem hiding this comment.
type says both culprit and grouping_key are always defined. don't want to use idx because it makes the property optional, which causes issues for consumers.
There was a problem hiding this comment.
Great! 👍 We shouldn't use idx for non-optional properties anyway
sorenlouv
added
release_note:skip
|
Pinging @elastic/apm-ui |
| interface Props extends APMLinkExtendProps { | ||
| serviceName: string; | ||
| errorGroupId: string; | ||
| } |
There was a problem hiding this comment.
Maybe it's just the name of APMLinkExtendProps that throws me off but might be more clear to do:
interface Props extends EuiLinkAnchorProps {
serviceName: string;
errorGroupId: string;
children?: React.ReactNode;
}There was a problem hiding this comment.
APMLinkExtendProps explicitly disallows path and query to prevent prop conflicts (e.g. serviceName and errorGroupId are used to build a path, and uiFilters are used to build a query). Would be happy with a better name though, if you have any suggestions 😅
| @@ -18,6 +18,8 @@ interface Props extends EuiLinkAnchorProps { | |||
| children?: React.ReactNode; | |||
| } | |||
|
|
|||
| export type APMLinkExtendProps = Omit<Props, 'path' | 'query'>; | |||
|
|
|||
There was a problem hiding this comment.
Not necessary for this PR but looking at the TODO comment in this file we could fix it like:
export function getAPMHref(
path: string,
urlParams: IUrlParams,
query: APMQueryParams = {}
) {
const nextQuery = {
rangeFrom: urlParams.rangeFrom || TIMEPICKER_DEFAULTS.rangeFrom,
rangeTo: urlParams.rangeTo || TIMEPICKER_DEFAULTS.rangeTo,
refreshPaused: urlParams.refreshPaused || TIMEPICKER_DEFAULTS.refreshPaused,
refreshInterval:
urlParams.refreshInterval || TIMEPICKER_DEFAULTS.refreshInterval,
kuery: urlParams.kuery,
environment: urlParams.environment,
...query
};
const nextSearch = fromQuery(nextQuery);
return url.format({
pathname: core.http.basePath.prepend('/app/apm'),
hash: `${path}?${nextSearch}`
});
}I opted for inlining PERSISTENT_APM_PARAMS since it's not re-used anywhere else and this seemed a little clearer.
But again: not important for this PR.
💚 Build Succeeded |
x-pack/legacy/plugins/apm/public/components/app/TransactionDetails/Transaction/index.tsx
Outdated
Show resolved
Hide resolved
| 'host', | ||
| 'containerId', | ||
| 'podName' | ||
| ); |
There was a problem hiding this comment.
These are now duplicated:
I don't think that's a problem now, but in the future we could lift the filterNames to be defined in the route config (where this linking function could also be defined).
There was a problem hiding this comment.
Yeah this is one of the downsides of loose coupling. But hopefully we can fix the routing soon
There was a problem hiding this comment.
But hopefully we can fix the routing soon
++
💚 Build Succeeded |
* [APM] Persist local UI filter across views * Migrate remaining links * Add comment clarifying override of EUITab styling * Remove unnecessary check for existence of waterfall.traceRoot
|
Test plan:
|
Just did table + tabs, no breadcrumbs and others right now until we figure out what the right implementation is.
Service Overview => Transaction Overview (table)
Service Overview => Trace Overview (tab)
Trace Overview => Transaction Detail (table)
Trace Overview => Service Overview (tab)
Trace Overview => Service Overview (breadcrumb)Transaction Overview => Transaction Detail (table)
Transaction Overview => Error Overview (tab)
Transaction Overview => Metrics Overview (tab)
Transaction Overview => Service Overview (breadcrumb)Error Overview => Transaction Overview (tab)
Error Overview => Metrics Overview (tab)
Error Overview => Transaction Overview (breadcrumb)Error Overview => Service Overview (breadcrumb)Metrics Overview => Transaction Overview (tab)
Metrics Overview => Error Overview (tab)
Metrics Overview => Transaction Overview (breadcrumb)Metrics Overview => Service Overview (breadcrumb)Transaction Detail => Transaction Overview (breadcrumb)Transaction Detail => Transaction Overview (flyout)
Transaction Detail => Trace Overview (full trace link)