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.

Sign up for GitHub

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

Jump to bottom

[APM] Immediately return terms for unbound queries #74543

Merged
merged 7 commits into from
Sep 2, 2020
Merged

[APM] Immediately return terms for unbound queries #74543

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fab5944
[APM] Immediately return terms for unbound queries
dgieselaar Aug 6, 2020
3429781
Merge branch 'master' into faster-suggestions
elasticmachine Aug 9, 2020
da836e7
Merge branch 'master' into faster-suggestions
elasticmachine Aug 17, 2020
b1bf934
Merge branch 'master' into faster-suggestions
elasticmachine Aug 26, 2020
dd0bf3f
Merge branch 'master' into faster-suggestions
elasticmachine Aug 27, 2020
bf9279b
Merge branch 'master' of github.com:elastic/kibana into faster-sugges…
dgieselaar Sep 2, 2020
4072dbf
Merge branch 'faster-suggestions' of github.com:dgieselaar/kibana int…
dgieselaar Sep 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions x-pack/plugins/apm/server/lib/environments/get_all_environments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export async function getAllEnvironments({
],
},
body: {
...(!serviceNameFilter.length ? { timeout: '1ms' } : {}),
size: 0,
query: {
bool: {
Expand All @@ -48,6 +49,7 @@ export async function getAllEnvironments({
terms: {
field: SERVICE_ENVIRONMENT,
size: 100,
...(!serviceNameFilter.length ? { min_doc_count: 0 } : {}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a comment on the actual feasibility of this approach but a general comment about conditional logic in our ES queries.

I think it's useful to co-locate logic that is meant to go together. For instance, if we add this behaviour but choose to delete it again, it'll be much easier if it's co-located (optimize for deletion).
Similarly, if there is an issue with this implementation and somebody (new) has to debug it, it's much easier if all the related lines are co-located so they don't have to study the es query in detail in case there are other places where we handle this behaviour. Additionally it's easier to write a meaningful comment that applies to all of the lines:

// return results immediately for unbound queries (query without a filter context)
if (!serviceNameFilter) {
 params.body.timeout = '1ms';
 params.aggs.environments.terms.min_doc_count = 0;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that it's easier to understand. I'll need to look into whether TypeScript likes this though. I may need to look for something that has the same sentiment, but also has type-safety.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sqren this didn't work out for me, TypeScript started complaining. I don't feel it's worth it to add things like @ts-expect-error, so I'll just leave it as is. I did add a comment to explain what it does.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I still thing it would have been worth co-locating it. It's not a big deal in a small query like this but it all adds up. I also don't like @ts-expect-error but I do like having a single coherent chunk of logic:

  if (!serviceNameFilter) {
    // @ts-expect-error
    params.body.timeout = '1ms';
    // @ts-expect-error
    params.aggs.environments.terms.min_doc_count = 0;
  }

missing: includeMissing ? ENVIRONMENT_NOT_DEFINED : undefined,
},
},
Expand All @@ -56,6 +58,7 @@ export async function getAllEnvironments({
};

const resp = await apmEventClient.search(params);

const environments =
resp.aggregations?.environments.buckets.map(
(bucket) => bucket.key as string
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/apm/server/lib/helpers/create_es_client/call_client_with_debug.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export async function callClientWithDebug({
let res: any;
let esError = null;
try {
res = apiCaller(operationName, params);
res = await apiCaller(operationName, params);
} catch (e) {
// catch error and throw after outputting debug info
esError = e;
Expand Down
2 changes: 2 additions & 0 deletions ...ck/plugins/apm/server/lib/settings/agent_configuration/__snapshots__/queries.test.ts.snap
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions x-pack/plugins/apm/server/lib/settings/agent_configuration/get_service_names.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,14 @@ export async function getServiceNames({ setup }: { setup: Setup }) {
],
},
body: {
timeout: '1ms',
size: 0,
aggs: {
services: {
terms: {
field: SERVICE_NAME,
size: 50,
min_doc_count: 0,
},
},
},
Expand Down