[APM] Immediately return terms for unbound queries

[dgieselaar]

Closes #73787 and #58591.

I'm not entirely sold. It will for instance return terms for deleted services. I'll look at some telemetry to get a feeling of whether it's necessary.

[elasticmachine]

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

[dgieselaar]

@elasticmachine merge upstream

[dgieselaar]

@elasticmachine merge upstream

[dgieselaar]

I'll leave this open until @sqren is back next week. As said before, I'm not sold on whether this is the right approach.

[dgieselaar]

@elasticmachine merge upstream

[sorenlouv]

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;
}

[dgieselaar]

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.

[dgieselaar]

@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.

[sorenlouv]

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;
  }

[sorenlouv]

If you think the pros outweighs the cons I'm good with this change 👍

(code-wise I still prefer the co-location that I mentioned :) )

[dgieselaar]

@sqren this is a downside:

It will for instance return terms for deleted services.

I'm not sure how often that will happen though. I remember now I can look at telemetry to see how long a query like this will take. Let me get back about that.

[dgieselaar]

@elasticmachine merge upstream

[dgieselaar]

FWIW, couldn't really tell anything from the telemetry unfortunately.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 4072dbf

Build metrics

✅ unchanged

History

• 💚 Build #70815 succeeded dd0bf3f
• 💔 Build #70445 failed b1bf934
• 💚 Build #68426 succeeded da836e7
• 💚 Build #67131 succeeded 3429781
• 💚 Build #66939 succeeded fab5944

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