[APM] Optimize services overview #69648
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dgieselaar
added
Team:APM
dgieselaar
force-pushed
the
optimize-services-overview
branch
from
a65f141
to
c5fe710
Compare
dgieselaar
changed the title
| bool: { | ||
| filter: projection.body.query.bool.filter.concat({ | ||
| term: { | ||
| [PROCESSOR_EVENT]: 'transaction', |
There was a problem hiding this comment.
Suggested change
| [PROCESSOR_EVENT]: 'transaction', | |
| [PROCESSOR_EVENT]: ProcessorEvent.transaction, |
| @@ -19,72 +20,315 @@ import { | |||
| } from '../../helpers/setup_request'; | |||
| import { getServicesProjection } from '../../../../common/projections/services'; | |||
|
|
|||
| const MAX_NUMBER_OF_SERVICES = 500; | |||
There was a problem hiding this comment.
Shouldn't we have a settings option instead of hardcoded?
There was a problem hiding this comment.
@sqren thoughts here? We didn't have one before AFAIK.
There was a problem hiding this comment.
I think there are arguments for making it configurable but no-one ever asked for this and I'd rather not make it configurable if it's not needed so... let's leave it for now, and make it configurable if someone asks for it.
| ...projection.body.query.bool.filter, | ||
| { | ||
| terms: { | ||
| [PROCESSOR_EVENT]: ['metric', 'error', 'transaction'], |
There was a problem hiding this comment.
Suggested change
| [PROCESSOR_EVENT]: ['metric', 'error', 'transaction'], | |
| [PROCESSOR_EVENT]: [ProcessorEvent.metric, ProcessorEvent.error, ProcessorEvent.transaction], |
There was a problem hiding this comment.
I still think there is value in having a lightweight APM client.
In this case the PROCESSOR_EVENT would be added automatically based on the indicies.
| query: { | ||
| bool: { | ||
| filter: [ | ||
| ...projection.body.query.bool.filter, |
There was a problem hiding this comment.
In the getTransactionDurationAvg you used concat:
filter: projection.body.query.bool.filter.concatand here you used spread syntax.
...projection.body.query.bool.filter,I'm not against neither implementations, but maybe we could pick one so we have consistency?
|
|
||
| return aggregations.services.buckets.map((bucket) => ({ | ||
| name: bucket.key as string, | ||
| value: (bucket.agent_name.hits.hits[0]?._source as { |
There was a problem hiding this comment.
I'm not sure if it's possible, but can't we define the response type we expect when calling setup.client.search<T>? Then we could avoid this.
There was a problem hiding this comment.
We can, but then we have to create a separate variable for params and use client.search<{ agent: { name: string } }, typeof params>. Which means we lose type checks and autocompletion when we create the request.
There was a problem hiding this comment.
(As soon as you start defining generic arguments TS will no longer infer others, AFAIK).
| ...projection.body.query.bool.filter, | ||
| { | ||
| term: { | ||
| [PROCESSOR_EVENT]: 'transaction', |
There was a problem hiding this comment.
Suggested change
| [PROCESSOR_EVENT]: 'transaction', | |
| [PROCESSOR_EVENT]: ProcessorEvent.transaction, |
|
|
||
| const serviceBuckets = aggs?.services.buckets || []; | ||
| const deltaAsMinutes = (setup.end - setup.start) / 1000 / 60; |
There was a problem hiding this comment.
Maybe you could extract deltaAsMinutes to a function and use it on getTransactionRate too.
|
|
||
| const projection = getServicesProjection({ setup }); | ||
| const arrayUnionToCallable = <T extends any[]>( |
There was a problem hiding this comment.
I kind of don't get why it's needed.
There was a problem hiding this comment.
A clarifying comment would help
sorenlouv
reviewed
Jun 23, 2020
| }; | ||
|
|
||
| const getAgentName = async ({ setup, projection }: AggregationParams) => { | ||
| const response = await setup.client.search( |
There was a problem hiding this comment.
WDYT about breaking moving each query into a separate file to improve readability?
There was a problem hiding this comment.
Opted for all queries in one separate file. LMK what you think.
sorenlouv
reviewed
Jun 23, 2020
|
|
||
| const getAgentName = async ({ setup, projection }: AggregationParams) => { | ||
| const response = await setup.client.search( | ||
| mergeProjection(projection, { |
There was a problem hiding this comment.
Are we able to get rid of the projections if we remove the counts? I think that would make all queries 10x more readable.
There was a problem hiding this comment.
It would make it a little easier, as the contextual filters don't have to care about the aggregation needed to calculate a count. But there's a bunch of other stuff that a projection takes care of:
- range filter on
@timestamp - type-checking projection-specific parameters like
service.nameortransaction.type - querying the appropriate index
I think the lightweight APM client you are proposing is trying to solve at least some of these things. I think we should also take latency metrics into account which will complicate things a bit.
There was a problem hiding this comment.
Projections creates an inversion of control which makes it very hard to see what's actually going on.
range filter on @timestamp
We already have a helper for this?
querying the appropriate index
We can make a helper for this, or like you say, we can use a lightweight client.
type-checking projection-specific parameters like service.name or transaction.type
Not sure about what this is but it would be great if we can avoid IOC.
dgieselaar
force-pushed
the
optimize-services-overview
branch
from
c5fe710
to
43f55a5
Compare
sorenlouv
reviewed
Jun 24, 2020
| { | ||
| terms: { | ||
| [PROCESSOR_EVENT]: ['transaction', 'error', 'metric'], | ||
| }, |
There was a problem hiding this comment.
Another note on the inversion-of-control problem introduced by projections:
PROCESSOR_EVENT should be aligned with the queried indices. In this case the indicies are specified in the projection. It is therefore possible for the indicies and processor event filter to go out of sync without us noticing.
|
Pinging @elastic/apm-ui (Team:apm) |
sorenlouv
reviewed
Jun 27, 2020
| const totalTransactions = transactions?.doc_count || 0; | ||
| export async function getServicesItems(setup: ServicesItemsSetup) { | ||
| const params = { | ||
| projection: getServicesProjection({ setup, noEvents: true }), |
There was a problem hiding this comment.
What does noEvents mean? Is this to simulate a no-op?
There was a problem hiding this comment.
not a no-op, but with noEvents: true it doesn't make a decision about what events should be queried. It's not great, but the alternatives (having to define indices + processor.event term filters in a lot of other places) are worse IMO. It made me think though, our client and our projections could support { apm: { events: ProcessorEvent[] } as a top-level prop, and prohibit the use of index. The client's search function would then unpack apm.events to an index and a terms query.
There was a problem hiding this comment.
It made me think though, our client and our projections could support
{ apm: { events: ProcessorEvent[] }as a top-level prop, and prohibit the use ofindex. The client's search function would then unpackapm.eventsto an index and a terms query.
I think it's a good idea to prohibit the use of index and instead use apm.events to specify both the indicies to query and the processor.events to filter by.
This is similar to what I tried to suggest in #67397 - although I called it docTypes and was probably not super clear. I've renamed it to apmEvents now.
There was a problem hiding this comment.
@sqren I think I understand the need better now 👍 and IMO a top-level prop lends itself better to composition than a second function argument.
There was a problem hiding this comment.
In addition to the client idea, we should move away from the mega object setup has become and I'd like to move away from mergeProjections and the overuse of spreading: #70157
sorenlouv
reviewed
Jun 30, 2020
| getTransactionRate, | ||
| getErrorRate, | ||
| getEnvironments, | ||
| } from './get_metrics'; |
There was a problem hiding this comment.
Unless this is related to the metric based ui or agent runtime metrics we should find another term.
There was a problem hiding this comment.
Changed this to get_services_items_stats. Also pluralised the function names.
sorenlouv
reviewed
Jun 30, 2020
| ?.value ?? null, | ||
| environments: | ||
| environments.find((service) => service.name === serviceName)?.value ?? | ||
| [], |
There was a problem hiding this comment.
This suddenly got a lot more complex. Any reason why transactionRate and the others are now arrays instead of scalar values?
Shouldn't transactionRate be a numeric value in the first place, and not an array?
There was a problem hiding this comment.
Now using joinByKey to make this code easier to understand.
sorenlouv
reviewed
Jun 30, 2020
| ] = await Promise.all([ | ||
| getTransactionDurationAvg(params), | ||
| getAgentName(params), | ||
| getTransactionRate(params), |
There was a problem hiding this comment.
Since we are retrieving more than one transaction rate
Suggested change
| getTransactionRate(params), | |
| getTransactionRates(params), |
sorenlouv
reviewed
Jun 30, 2020
Comment on lines
69
to
71
| transactionsPerMinute: | ||
| transactionRate.find((service) => service.name === serviceName) | ||
| ?.value ?? 0, |
There was a problem hiding this comment.
Suggested change
| transactionsPerMinute: | |
| transactionRate.find((service) => service.name === serviceName) | |
| ?.value ?? 0, | |
| transactionsPerMinute: transactionRates?.[serviceName] ?? 0, |
sorenlouv
reviewed
Jun 30, 2020
Comment on lines
190
to
196
| return arrayUnionToCallable(aggregations.services.buckets).map((bucket) => { | ||
| const transactionsPerMinute = bucket.doc_count / deltaAsMinutes; | ||
| return { | ||
| name: bucket.key as string, | ||
| value: transactionsPerMinute, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Suggested change
| return arrayUnionToCallable(aggregations.services.buckets).map((bucket) => { | |
| const transactionsPerMinute = bucket.doc_count / deltaAsMinutes; | |
| return { | |
| name: bucket.key as string, | |
| value: transactionsPerMinute, | |
| }; | |
| }); | |
| return arrayUnionToCallable(aggregations.services.buckets).reduce< | |
| Record<string, number> | |
| >((acc, bucket) => { | |
| const transactionsPerMinute = bucket.doc_count / deltaAsMinutes; | |
| const serviceName = bucket.key as string; | |
| return { ...acc, [serviceName]: transactionsPerMinute }; | |
| }, {}); |
|
Have you talked to the ES team about this? The changes in this PR replaces simplicity and readability with performance. So we should make sure the performance gains are worth it. |
|
@elasticmachine merge upstream |
sorenlouv
reviewed
Jul 2, 2020
| indices['apm_oss.errorIndices'], | ||
| indices['apm_oss.transactionIndices'], | ||
| ], | ||
| }), |
There was a problem hiding this comment.
will this be changed by your PR for new client?
There was a problem hiding this comment.
yep! there will no longer be a need for noEvents.
sorenlouv
reviewed
Jul 2, 2020
| [...agentNames, ...transactionRates], | ||
| 'serviceName' | ||
| ); | ||
| */ |
There was a problem hiding this comment.
Thanks for the description and tests ❤️
sorenlouv
reviewed
Jul 2, 2020
| U extends UnionToIntersection<T>, | ||
| V extends keyof T & keyof U | ||
| >(items: T[], key: V): JoinedReturnType<T, U, V> { | ||
| return items.reduce<JoinedReturnType<T, U, V>>((prev, current) => { |
There was a problem hiding this comment.
nit: I find it useful to distinguish between the accumulator and the items that are being iterated.
Suggested change
| return items.reduce<JoinedReturnType<T, U, V>>((prev, current) => { | |
| return items.reduce<JoinedReturnType<T, U, V>>((acc, item) => { |
sorenlouv
reviewed
Jul 2, 2020
| const response = await client.search( | ||
| mergeProjection(projection, { | ||
| size: 0, | ||
| index: indices['apm_oss.transactionIndices'], |
There was a problem hiding this comment.
A comment more on projections: In this case I don't know how to see that the projection won't override the index.
There was a problem hiding this comment.
Maybe I'm misunderstanding - but this is a merge, so values are applied from left to right. ie, indices['apm_oss.transactionIndices'] will override whatever the projection has defined.
sorenlouv
reviewed
Jul 2, 2020
| aggs: { | ||
| services: { | ||
| terms: { | ||
| ...projection.body.aggs.services.terms, |
There was a problem hiding this comment.
Not related to this PR specifically but I want to invite you into my head and how I try to reason about the projections :)
I wonder what these terms are so I try to search for them. If I search for projection.body.aggs.services.terms, body.aggs.services.terms, aggs.services.terms or services.terms I don't get any useful results.
Then I search for projection and get a lot results:
So I give up searching for terms and instead go into manual mode and search for getTransactionDurationAverages which leads me to getServicesItems which is calling getServicesProjection where I eventually find:
I can now see that the terms I'm looking for are on line 46. Great! But I notice it's a ternary so I have to back up a bit and figure out how getServicesProjection is called. I now see that getServicesProjection is called with noEvents=true so the actual line I'm looking for is 42 : []. 🏅
It's definitely possible to find these things but it requires patience and focus and not super easy on Github (would have been easier in an editor).
There was a problem hiding this comment.
Haha, nice. Wish I could do that on Github - for these things it'll be better to check out for sure.
And I now realise that the value I found was wrong (looking in the query instead of the agg 😳)
There was a problem hiding this comment.
VSCode's github plugin might help btw, have you tried it?
There was a problem hiding this comment.
I have - a while ago. I should try it again
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/lens/smokescreen·ts.lens app lens smokescreen tests should allow creation of lens visualizationsStandard OutStack TraceBuild metrics
History
To update your PR or re-run it, just comment with: |
sorenlouv
reviewed
Jul 2, 2020
| return []; | ||
| } | ||
|
|
||
| return aggregations.services.buckets.map((bucket) => ({ |
There was a problem hiding this comment.
I've seen this in a few places and would be really nice if we could do something like:
const buckets = aggregations.services.buckets as Buckets<string, {
agent: {
name: string;
};
}>
return buckets.map(bucket => {
serviceName: bucket.key,
agentName: bucket.agent_name.hits.hits[0]?._source.agent.name
})There was a problem hiding this comment.
Alternatively:
type Bucket = <string, { agent: { name: string; }; }>;
return buckets.map(bucket: Bucket => {
serviceName: bucket.key,
agentName: bucket.agent_name.hits.hits[0]?._source.agent.name
})There was a problem hiding this comment.
I'm not sure if it's that easy, but I might be missing something. You'd also need to define agent_name, the name of the sub-aggregation, and the fact that it's a top_hits aggregation. Let me see if we still need it anyway if we resolve the document types from apm.events.
There was a problem hiding this comment.
Beautiful! Any way to narrow the source down even more? I seem to recall we could do that with the client's first generic argument: ApmEventsClient<Transaction>()
There was a problem hiding this comment.
it's based on whatever you specify in apm.events. In this case, it searches on error, metric and transaction indices, that's why it's a union type. If it were just ProcessorEvent.transaction, the type would be Transaction.
There was a problem hiding this comment.
Oh, wow. I guess that's perfect then 👍
sorenlouv
approved these changes
Jul 2, 2020
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
Jul 3, 2020
* master: (199 commits) [Telemetry] Add documentation about Application Usage (elastic#70624) [Ingest Manager] Improve agent unenrollment with unenroll action (elastic#70031) Handle timeouts on creating templates (elastic#70635) [Lens] Add ability to set colors for y-axis series (elastic#70311) [Uptime] Use elastic charts donut (elastic#70364) [Ingest Manager] Update registry URL to point to snapshot registry (elastic#70687) [Composable template] Create / Edit wizard (elastic#70220) [APM] Optimize services overview (elastic#69648) [Ingest Pipelines] Load from json (elastic#70297) [Rum Dashbaord] Rum selected service view (elastic#70579) [Uptime] Prevent duplicate requests on load for index status (elastic#70585) [ML] Changing shared module setup function parameters (elastic#70589) [Ingest Manager] Add ability to sort to agent configs and package configs (elastic#70676) [Alerting] document requirements for developing new action types (elastic#69164) Fixed adding an extra space character on selecting alert variable in action text fields (elastic#70028) [Maps] show vector tile labels on top (elastic#69444) chore(NA): upgrade to lodash@4 (elastic#69868) Add Snapshot Restore README with quick-testing steps. (elastic#70494) [EPM] Use higher priority than default templates (elastic#70640) [Maps] Fix cannot select Solid fill-color when removing fields (elastic#70621) ...
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
Jul 3, 2020
* master: [Lens] Fitting functions (elastic#69820) [Telemetry] Add documentation about Application Usage (elastic#70624) [Ingest Manager] Improve agent unenrollment with unenroll action (elastic#70031) Handle timeouts on creating templates (elastic#70635) [Lens] Add ability to set colors for y-axis series (elastic#70311) [Uptime] Use elastic charts donut (elastic#70364) [Ingest Manager] Update registry URL to point to snapshot registry (elastic#70687) [Composable template] Create / Edit wizard (elastic#70220) [APM] Optimize services overview (elastic#69648)
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
kibanamachine
added
the
backport missing
dgieselaar
added a commit
to dgieselaar/kibana
that referenced
this pull request
Jul 7, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
dgieselaar
added a commit
that referenced
this pull request
Jul 7, 2020
kibanamachine
removed
the
backport missing
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(Keeping it in draft for now as I need #69384 to test on a specific cluster).This decreases the loading time of the services overview by splitting up the requests to Elasticsearch.
On the cluster used for testing, time for
/api/apm/servicesgoes down from about ~55s to ~29s. I think that's worth the increase in complexity but open to other opinions of course.A couple of changes here:
track_total_hitsrather than a terms aggregation which is more costly.Tested by comparing data in master: