Remove kuery from uiFilters #91932

smith · 2021-02-18T20:23:35Z

Make kuery a standalone query parameter instead of part of uiFilters
Move getEsFilters helper to RUM
Move query utils from "common" to "server" (it uses esKuery from the data plugin, which is exported from server, not common.)
Move uiFiltersRt to RUM routes

References #84526.

elasticmachine · 2021-02-19T21:25:35Z

Pinging @elastic/apm-ui (Team:apm)

elasticmachine · 2021-02-19T21:25:35Z

Pinging @elastic/uptime (Team:uptime)

dgieselaar

Such a good change! 👍👍👍 I've got one question: we previously had a type guard around uiFilters, that it was not available on Setup unless the corresponding query parameter/rt is set (similar to SetupTimeRange). Looks like that was removed (not sure why). I think we should put that guard back, because it will help the removal of uiFilters altogether. How do you feel about that? I can push a commit to this branch that puts it back. It would be good to get feedback from whoever removed it as well, there may have been a good reason that I'm not aware of. I think it was @sqren?

dgieselaar · 2021-02-22T12:26:32Z

x-pack/plugins/apm/server/lib/errors/get_error_group_sample.ts

@@ -43,7 +49,7 @@ export function getErrorGroupSample({
              { term: { [ERROR_GROUP_ID]: groupId } },
              ...rangeQuery(start, end),
              ...environmentQuery(environment),
-              ...esFilter,
+              ...searchQuery(kuery),


what about kqlToQueryDSL 😬 ?

or just kqlToQuery, or queryFromKql, you get the idea.

So far these functions are all *thing*Query: rangeQuery, environmentQuery, serviceNodeNameQuery, etc. How about kqlQuery?

yeah, works for me 👍

(assuming you won't go with kqlQuery instead of kuery like Shahzad suggested)

I'm just leaving kuery as the name of the query string parameter, and kqlQuery as the name of the function that converts the KQL query text into a QueryContainer[].

x-pack/plugins/apm/server/lib/metrics/by_agent/java/index.ts

dgieselaar · 2021-02-22T12:28:56Z

x-pack/plugins/apm/server/lib/metrics/by_agent/java/thread_count/index.ts

  setup,
  serviceName,
  serviceNodeName,
 }: {
  environment?: string;
+  kuery?: string;


Should these be optional always? Can we avoid it?

Can't think of a case where it wouldn't be optional, since you can always have an empty kuery bar and it's present on most pages.

if it's empty I'd say it's still a string? But making it optional makes it easy to accidentally leave out

I'd like to revisit the "requiredness" of parameters shortly in a future PR. Since useUrlParams always returns an object with everything optional, it's a bit tricky to make something required, and there are some endpoints where kuery and environment aren't there at all. If we can make some improvements to the client-side routes to have more type information about their query parameters we can make this less error-prone.

I think we can just make kuery required in the UrlParams type. I can have a look if you want (and also make the uiFilters type change).

Please do, but this PR is already changing 144 files so I don't want to make it too much bigger.

@smith Alright, if you can create the issue I'll take a stab on a follow-up PR, that way you're not blocked by me.

Quid pro quo 😬

Pleasure doing business 🤝

shahzad31 · 2021-02-22T13:08:48Z

...ugins/apm/server/lib/rum_client/ui_filters/local_ui_filters/__snapshots__/index.test.ts.snap

-          Object {
-            "term": Object {
-              "service.environment": "test",
-            },
-          },


is this intentional?

Yes. esFilter was a property of setup but now it's removed. The places where it was used in UX have been replaced with a call to getEsFilter with the uiFilters.

shahzad31 · 2021-02-22T13:09:19Z

x-pack/plugins/apm/server/lib/rum_client/__snapshots__/queries.test.ts.snap

-          Object {
-            "term": Object {
-              "service.environment": "test",
-            },
-          },


is this intentional?

Yes, see above.

shahzad31

This looks good, though i found another issue, which is possible side effect of this #89647 or maybe it got broken later. But UX app environment filter isn't working, i mean it's not passing environment filter to API's.

smith · 2021-02-23T14:22:05Z

This looks good, though i found another issue, which is possible side effect of this #89647 or maybe it got broken later. But UX app environment filter isn't working, i mean it's not passing environment filter to API's.

@shahzad31 Thanks I'll make sure this gets fixed. Is it working on 7.12 but not master?

shahzad31 · 2021-02-23T14:28:32Z

This looks good, though i found another issue, which is possible side effect of this #89647 or maybe it got broken later. But UX app environment filter isn't working, i mean it's not passing environment filter to API's.

@shahzad31 Thanks I'll make sure this gets fixed. Is it working on 7.12 but not master?

i haven't taken a look at 7.12, but i suspect it's broken there as well.

smith · 2021-02-23T14:30:30Z

@shahzad31

i haven't taken a look at 7.12, but i suspect it's broken there as well.

In 7.12 we're only removing UI filters from the APM UI. None of the changes to environment or kuery are backported there. Environment switching on UX on dev-edge seems to work fine right now.

kibanamachine · 2021-02-23T18:22:49Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`apm`	5.3MB	5.3MB	-1.2KB

History

💔 Build #108646 failed 5d34c65
💚 Build #108496 succeeded e864f75
💔 Build #108515 failed 6e98f00
💚 Build #108232 succeeded 065c85d
💔 Build #108191 failed 60a7066

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kibanamachine · 2021-02-23T20:06:42Z

💔 Backport failed

❌ 7.x: Commit could not be cherrypicked due to conflicts

To backport manually, check out the target branch and run:
node scripts/backport --pr 91932

* Make kuery a standalone query parameter instead of part of uiFilters * Move getEsFilters helper to RUM * Move query utils from "common" to "server" (it uses esKuery from the data plugin, which is exported from server, not common.) * Move uiFiltersRt to RUM routes References elastic#84526. # Conflicts: # x-pack/plugins/apm/public/context/annotations/annotations_context.tsx

kibanamachine · 2021-02-25T20:11:21Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine · 2021-02-26T20:21:20Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine · 2021-03-01T20:31:21Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

* Make kuery a standalone query parameter instead of part of uiFilters * Move getEsFilters helper to RUM * Move query utils from "common" to "server" (it uses esKuery from the data plugin, which is exported from server, not common.) * Move uiFiltersRt to RUM routes References #84526. # Conflicts: # x-pack/plugins/apm/public/context/annotations/annotations_context.tsx

smith added 11 commits February 18, 2021 10:21

start

1a58aca

cccchanges

a38e6bd

test fixes

a81673a

fixes

73f7499

remove unused imports

733e7fa

move getEsFilter

ab0ce16

search term naming

d5a6563

move to server

6701ff7

add file

eb35e0b

move test

b5f2d0c

Merge remote-tracking branch 'upstream/master' into nls/no-kuery

d73fdae

smith added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.13.0 v8.0.0 labels Feb 19, 2021

smith marked this pull request as ready for review February 19, 2021 17:44

smith requested review from a team as code owners February 19, 2021 17:44

smith mentioned this pull request Feb 19, 2021

[APM] Remove ui filters #84526

Closed

8 tasks

smith added 2 commits February 19, 2021 15:19

Merge remote-tracking branch 'upstream/master' into nls/no-kuery

84acb75

update for error stats

8ed7688

botelastic bot added Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Feb 19, 2021

smith added 3 commits February 19, 2021 16:11

fix for comparison

60a7066

Merge remote-tracking branch 'upstream/master' into nls/no-kuery

30b7919

fix compare tests

065c85d

dgieselaar approved these changes Feb 22, 2021

View reviewed changes

dgieselaar reviewed Feb 22, 2021

View reviewed changes

x-pack/plugins/apm/server/lib/metrics/by_agent/java/index.ts Outdated Show resolved Hide resolved

dgieselaar reviewed Feb 22, 2021

View reviewed changes

shahzad31 reviewed Feb 22, 2021

View reviewed changes

smith added 3 commits February 22, 2021 11:39

extract options

4978c1f

Merge remote-tracking branch 'upstream/master' into nls/no-kuery

f0fa80b

space

e864f75

smith requested a review from shahzad31 February 22, 2021 17:54

Merge remote-tracking branch 'upstream/master' into nls/no-kuery

6e98f00

smith mentioned this pull request Feb 22, 2021

[APM] Make environment and kuery parameters required #92286

Closed

smith added 2 commits February 22, 2021 20:47

Merge remote-tracking branch 'upstream/master' into nls/no-kuery

0ab0ea9

merge fixes

5d34c65

shahzad31 approved these changes Feb 23, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into nls/no-kuery

9c397ba

smith merged commit 57dfb81 into elastic:master Feb 23, 2021

smith deleted the nls/no-kuery branch February 23, 2021 20:04

smith mentioned this pull request Feb 24, 2021

[7.x] Remove kuery from uiFilters (#91932) #92685

Merged

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 25, 2021

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove kuery from uiFilters #91932

Remove kuery from uiFilters #91932

smith commented Feb 18, 2021 •

edited

Loading

elasticmachine commented Feb 19, 2021

elasticmachine commented Feb 19, 2021

dgieselaar left a comment

dgieselaar Feb 22, 2021

dgieselaar Feb 22, 2021

smith Feb 22, 2021

dgieselaar Feb 22, 2021

dgieselaar Feb 22, 2021

smith Feb 22, 2021

dgieselaar Feb 22, 2021

smith Feb 22, 2021

dgieselaar Feb 22, 2021

smith Feb 22, 2021

dgieselaar Feb 22, 2021

smith Feb 22, 2021

dgieselaar Feb 22, 2021

dgieselaar Feb 22, 2021

smith Feb 22, 2021

dgieselaar Feb 22, 2021

shahzad31 Feb 22, 2021

smith Feb 22, 2021

shahzad31 Feb 22, 2021

smith Feb 22, 2021

shahzad31 left a comment

smith commented Feb 23, 2021

shahzad31 commented Feb 23, 2021

smith commented Feb 23, 2021

kibanamachine commented Feb 23, 2021

kibanamachine commented Feb 23, 2021

kibanamachine commented Feb 25, 2021

kibanamachine commented Feb 26, 2021

kibanamachine commented Mar 1, 2021

Remove kuery from uiFilters #91932

Remove kuery from uiFilters #91932

Conversation

smith commented Feb 18, 2021 • edited Loading

elasticmachine commented Feb 19, 2021

elasticmachine commented Feb 19, 2021

dgieselaar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzad31 left a comment

Choose a reason for hiding this comment

smith commented Feb 23, 2021

shahzad31 commented Feb 23, 2021

smith commented Feb 23, 2021

kibanamachine commented Feb 23, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented Feb 23, 2021

💔 Backport failed

kibanamachine commented Feb 25, 2021

kibanamachine commented Feb 26, 2021

kibanamachine commented Mar 1, 2021

smith commented Feb 18, 2021 •

edited

Loading