[APM] Split dependencies API adding a new API to return comparison statistics #95407

cauemarcondes · 2021-03-25T13:25:28Z

closes #90575

Comparison enabled:

Comparison disabled:

…n-dependencies

x-pack/plugins/apm/server/lib/services/get_service_dependencies/get_metrics.ts

elasticmachine · 2021-03-29T18:12:14Z

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

x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts

ogupte

Few code style nits, but everything looks good and works locally for me!

dgieselaar · 2021-03-29T19:11:40Z

@cauemarcondes can you verify:

how does the performance of this approach compare to two parallel requests?
what happens if a dependency was available in the comparison time range, but not in the current time range?

kuisathaverat · 2021-03-30T10:34:49Z

/oblt-deploy

kuisathaverat · 2021-03-30T10:44:16Z

/oblt-deploy

kuisathaverat · 2021-03-30T11:00:47Z

sorry for the noise I was testing the build with comments

…n-dependencies

cauemarcondes · 2021-04-01T16:14:59Z

how does the performance of this approach compare to two parallel requests?

@dgieselaar Even though a single query that fetches both periods is a few ms slower, in the end, the result is a bit faster than having two queries. But the difference isn't that much, so I'd rather keeping with what I've done on this PR if it's okay for you.

dgieselaar · 2021-04-01T16:17:26Z

@cauemarcondes can you put some numbers to that and elaborate on the two things that you compared?

cauemarcondes · 2021-04-01T16:57:57Z

@cauemarcondes can you put some numbers to that and elaborate on the two things that you compared?

@dgieselaar I'm comparing the response time, for the last 15 minutes.

/dependencies: one query that fetches both current and comparison periods.
/test: Two queries one for the current period and another for the comparison periods.

Most of the time the /test, that runs two queries, takes a few seconds more to return compared to the other call.

Do you want me to run any specific test?

dgieselaar · 2021-04-01T17:03:42Z

@cauemarcondes you need to call ES directly, on a longer time range. There are many other things happening in a request that will influence the result. If you open a trace from an instrumented Kibana instance you can see that there are other ES calls that happen before that.

Edit: also, are the two queries in parallel, or one after the other? Also can you post the full ES search requests for both strategies?

cauemarcondes · 2021-04-01T17:15:29Z

Edit: also, are the two queries in parallel, or one after the other? Also can you post the full ES search requests for both strategies?

They are running in parallel with Promise.all. In these two files, there are all queries made for both strategies.

single_query.txt
two_queries.txt

dgieselaar · 2021-04-01T17:35:40Z

@cauemarcondes I would also recommend to run them through the profiler to see what the differences are.

cauemarcondes · 2021-04-01T17:43:17Z

@cauemarcondes I would also recommend to run them through the profiler to see what the differences are.

just did it:

Fetching current and comparison periods in a single query:

Fetching current period:

By comparing the two images, it clear that the single query strategy is worse than the two queries. Especially because the two queries will run in parallel.

Based on that I'd say going for the two queries strategy would be the best solution, WDYT @dgieselaar ?

cauemarcondes · 2021-04-01T20:32:53Z

what happens if a dependency was available in the comparison time range, but not in the current time range?

@dgieselaar the dependency will be shown in the table, with the comparison sparkline but without the value beside it. I think if we show the value as well the user might be confused and think that the value shown is from the current period. Alternatively, we could show it in a different color. @formgeist

...ins/apm/public/components/app/service_overview/service_overview_dependencies_table/index.tsx

x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts

…n-dependencies

cauemarcondes · 2021-04-15T15:42:43Z

@elasticmachine merge upstream

x-pack/plugins/apm/server/lib/services/get_service_dependencies/find_common_connections.test.ts

cauemarcondes · 2021-04-19T12:58:11Z

@elasticmachine merge upstream

x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts

…n-dependencies

sorenlouv · 2021-04-20T22:59:23Z

x-pack/test/apm_api_integration/tests/service_overview/dependencies/index.ts

-              "impact": 1.36961744704522,
-              "latency": 2568.40816326531,
+              "currentPeriodImpact": 1.54893576051104,
              "name": "elasticsearch",
-              "throughput": 13.0666666666667,
+              "previousPeriodImpact": 1.19757368986118,


Why did impact change from "1.36961744704522" to "1.54893576051104"?

That's because the value 1.36961744704522 was based on the entire 30 minutes of data before adding the comparison range. With the comparison time range filtering the result, it is only 15 minutes for the current period and 15 minutes for the previous period. That's why it changed to 1.54893576051104 and now it also has "previousPeriodImpact": 1.19757368986118,

sorenlouv · 2021-04-20T23:00:17Z

x-pack/test/apm_api_integration/tests/service_overview/dependencies/index.ts

@@ -441,38 +444,33 @@ export default function ApiTest({ getService }: FtrProviderContext) {
        const impactValues = sortBy(
          response.body.serviceDependencies.map((item) => ({
            name: item.name,
-            impact: item.impact,
-            latency: item.latency.value,


Was latency not used here?

Then latency values are already being checked in another test

kibana/x-pack/test/apm_api_integration/tests/service_overview/dependencies/index.ts

Line 371 in a1acc7e

it('returns the right latency values', () => {

, that's why I removed it from here and left only the impact, which is what this test must validate.

…n-dependencies

kibanamachine · 2021-04-21T14:51:50Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1583	1584	+1

Async chunks

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

id	before	after	diff
`apm`	4.2MB	4.2MB	+2.5KB

History

💚 Build #121089 succeeded a1acc7e
💔 Build #120958 failed d09f35c
💔 Build #120924 failed 6588d43
💔 Build #120805 failed cedc8de
💔 Build #120547 failed 129390c

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

cauemarcondes added 2 commits March 25, 2021 11:25

adding comparison to dependencies table

515c1d3

adding should to query

f22c24d

cauemarcondes force-pushed the apm-comparison-dependencies branch from d29b7c1 to f22c24d Compare March 25, 2021 17:45

refactoring

e005e38

cauemarcondes added apm:comparison 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 labels Mar 25, 2021

cauemarcondes marked this pull request as ready for review March 25, 2021 20:08

cauemarcondes requested a review from a team as a code owner March 25, 2021 20:08

Merge branch 'master' of github.com:elastic/kibana into apm-compariso…

254142d

…n-dependencies

ogupte reviewed Mar 29, 2021

View reviewed changes

x-pack/plugins/apm/server/lib/services/get_service_dependencies/get_metrics.ts Outdated Show resolved Hide resolved

botelastic bot added the Team:APM All issues that need APM UI Team support label Mar 29, 2021

ogupte reviewed Mar 29, 2021

View reviewed changes

x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts Outdated Show resolved Hide resolved

ogupte approved these changes Mar 29, 2021

View reviewed changes

cauemarcondes added 2 commits April 1, 2021 10:40

Merge branch 'master' of github.com:elastic/kibana into apm-compariso…

60a80b0

…n-dependencies

addressing PR comments

ed18506

dgieselaar reviewed Apr 7, 2021

View reviewed changes

...ins/apm/public/components/app/service_overview/service_overview_dependencies_table/index.tsx Outdated Show resolved Hide resolved

cauemarcondes requested a review from dgieselaar April 7, 2021 13:01

dgieselaar reviewed Apr 8, 2021

View reviewed changes

x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts Outdated Show resolved Hide resolved

dgieselaar reviewed Apr 8, 2021

View reviewed changes

x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts Show resolved Hide resolved

dgieselaar reviewed Apr 8, 2021

View reviewed changes

x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts Outdated Show resolved Hide resolved

cauemarcondes added 3 commits April 12, 2021 11:01

Merge branch 'master' of github.com:elastic/kibana into apm-compariso…

e21f3d5

…n-dependencies

refactoring comparison

dd5c277

Merge branch 'master' of github.com:elastic/kibana into apm-compariso…

c71162d

…n-dependencies

cauemarcondes requested a review from dgieselaar April 14, 2021 17:42

refactoring comparison

2edeb25

Merge branch 'master' into apm-comparison-dependencies

b21fe38

cauemarcondes requested a review from ogupte April 19, 2021 11:58

dgieselaar reviewed Apr 19, 2021

View reviewed changes

x-pack/plugins/apm/server/lib/services/get_service_dependencies/find_common_connections.test.ts Outdated Show resolved Hide resolved

Merge branch 'master' into apm-comparison-dependencies

129390c

dgieselaar reviewed Apr 19, 2021

View reviewed changes

x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts Outdated Show resolved Hide resolved

cauemarcondes added 4 commits April 19, 2021 18:36

addressing PR comments

d383b38

fixing api test

0a08d5b

Merge branch 'master' of github.com:elastic/kibana into apm-compariso…

c0f9831

…n-dependencies

stop fetching comparison data when it is disabled

cedc8de

cauemarcondes requested a review from dgieselaar April 19, 2021 23:14

cauemarcondes added 3 commits April 20, 2021 08:17

fixing unit test

6588d43

Merge branch 'master' of github.com:elastic/kibana into apm-compariso…

d09f35c

…n-dependencies

fixing TS issue

a1acc7e

sorenlouv reviewed Apr 20, 2021

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into apm-compariso…

53380ae

…n-dependencies

cauemarcondes closed this Apr 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Split dependencies API adding a new API to return comparison statistics #95407

[APM] Split dependencies API adding a new API to return comparison statistics #95407

cauemarcondes commented Mar 25, 2021 •

edited

Loading

elasticmachine commented Mar 29, 2021

ogupte left a comment

dgieselaar commented Mar 29, 2021

kuisathaverat commented Mar 30, 2021

kuisathaverat commented Mar 30, 2021

kuisathaverat commented Mar 30, 2021

cauemarcondes commented Apr 1, 2021 •

edited

Loading

dgieselaar commented Apr 1, 2021

cauemarcondes commented Apr 1, 2021

dgieselaar commented Apr 1, 2021 •

edited

Loading

cauemarcondes commented Apr 1, 2021

dgieselaar commented Apr 1, 2021

cauemarcondes commented Apr 1, 2021

cauemarcondes commented Apr 1, 2021

cauemarcondes commented Apr 15, 2021

cauemarcondes commented Apr 19, 2021

sorenlouv Apr 20, 2021

cauemarcondes Apr 21, 2021

sorenlouv Apr 20, 2021

cauemarcondes Apr 21, 2021

kibanamachine commented Apr 21, 2021

[APM] Split dependencies API adding a new API to return comparison statistics #95407

[APM] Split dependencies API adding a new API to return comparison statistics #95407

Conversation

cauemarcondes commented Mar 25, 2021 • edited Loading

elasticmachine commented Mar 29, 2021

ogupte left a comment

Choose a reason for hiding this comment

dgieselaar commented Mar 29, 2021

kuisathaverat commented Mar 30, 2021

kuisathaverat commented Mar 30, 2021

kuisathaverat commented Mar 30, 2021

cauemarcondes commented Apr 1, 2021 • edited Loading

dgieselaar commented Apr 1, 2021

cauemarcondes commented Apr 1, 2021

dgieselaar commented Apr 1, 2021 • edited Loading

cauemarcondes commented Apr 1, 2021

dgieselaar commented Apr 1, 2021

cauemarcondes commented Apr 1, 2021

cauemarcondes commented Apr 1, 2021

cauemarcondes commented Apr 15, 2021

cauemarcondes commented Apr 19, 2021

sorenlouv Apr 20, 2021

Choose a reason for hiding this comment

cauemarcondes Apr 21, 2021

Choose a reason for hiding this comment

sorenlouv Apr 20, 2021

Choose a reason for hiding this comment

cauemarcondes Apr 21, 2021

Choose a reason for hiding this comment

kibanamachine commented Apr 21, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

cauemarcondes commented Mar 25, 2021 •

edited

Loading

cauemarcondes commented Apr 1, 2021 •

edited

Loading

dgieselaar commented Apr 1, 2021 •

edited

Loading