[APM] Local UI filters

[dgieselaar]

Closes #34922.

Services:

Traces:

Transactions overview:

Transactions detail:

Errors:

Metrics:

Filter menu:

Definition of Done for APM UI

• Flag and create issues of things that are left out
  • [APM] Persist local UI filters across views #43266
  • [APM] Validate params for local UI filters config #43267
• Before/after images or gif of UI changes
• Release labels
• Designer sign-off for UI related changes
• [ ] PM approval for bigger, user-facing features

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[spalger]

retest

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[dgieselaar]

That would be ideal, but I don't think we are there now (where we have named routes & typed parameters). How would it look if we don't have named routes & typed parameters?

…

On Sun, Jul 21, 2019 at 11:14 AM Søren Louv-Jansen ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In x-pack/legacy/plugins/apm/server/lib/ui_filters/get_local_filters/index.ts <#41588 (comment)>: > + setup: Setup; + server: Server; + type: DOCUMENT_TYPE; + uiFilters: UIFilters; +}) { + const { start, end, client, config: kibanaConfig } = setup; + + const typeConfig = config[type]; + + const localFilterAggregations = await Promise.all( + typeConfig.fields.map(async field => { + let filters = await getUiFiltersES(server, omit(uiFilters, field.name)); + + if (!filters.length) { + filters = [{ match_all: {} }]; + } Why is this necessary? If getUiFiltersES is empty it returns an empty list, which is the same as matching all, right? ------------------------------ In x-pack/legacy/plugins/apm/public/components/app/ServiceOverview/ServiceList/index.tsx <#41588 (comment)>: > @@ -50,7 +51,10 @@ export const SERVICE_COLUMNS = [ sortable: true, render: (serviceName: string) => ( <EuiToolTip content={formatString(serviceName)} id="service-name-tooltip"> - <AppLink path={`/${serviceName}/transactions`}> + <AppLink + path={`/services/${serviceName}/transactions`} + documentType={DOCUMENT_TYPE.SERVICES} When passing explicit query params, do you mean hard-coding them as props to AppLink? I'm thinking something like this: <AppLink routeName="transactionList" params={{ serviceName, hostId }} > If we can infer the required and optional params from the routeName What would then happen if we add or remove a local filter from the configuration I think the route config should decide what you can pass into links - not the filter config. Those two should be decoupled imo. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#41588?email_source=notifications&email_token=AACWDXGY7CRMPBXNNR5MTETQAQSGBA5CNFSM4IFIK2F2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB7CFYEI#pullrequestreview-264526865>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACWDXEMJQRLCOHORKZI6GDQAQSGBANCNFSM4IFIK2FQ> .

[dgieselaar]

Discussed via Zoom w/ @sqren (please correct me if I missed or misrepresented anything):

• document type should be something like view or route type
• going to create links for different views (like we have TransactionLink) and add type safety (of query params) via that abstraction. When/if we'll have named routes that type safety can bubble up via the router. For now we'll use a mix of manual typings + inferred from local ui filters.
• remove optimizations as much as possible. good guideline: memoize by default in hooks/context, in all other instances, only if absolutely necessary (and proven).
• rename localFilters to localUiFilters (everywhere)
• keep the spread operator, infer types as much as possible from one central location (the filters config)
• make transaction type look like a local UI filter, but exclude it from parsing/formatting in uiFilters hook and aggregation. making it a local filter under water will have too much impact on type boundaries/UI behaviour/API calls that expect a single value.

[sorenlouv]

document type should be something like view or route type

Yes, I think we should avoid creating a new "type" and just refer to the route names we already have.
We also shouldn't have to add the route name to the links.

[sorenlouv]

Agree with all the other points 👍

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[dgieselaar]

retest

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request