[APM] Service overview: Ensure transactionType is used for by every component #85665

smith · 2020-12-10T22:35:19Z

Fix the places where it was not being used:

Transactions table
Error rate chart
Errors table

Also fix broken column sorting on transaction table.

Fix the places where it was not being used: - Transactions table - Error rate chart - Errors table Also fix broken column sorting on transaction table.

elasticmachine · 2020-12-10T22:35:23Z

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

smith · 2020-12-14T15:10:18Z

@elasticmachine merge upstream

smith · 2020-12-14T21:15:47Z

retest

ogupte · 2020-12-14T21:56:42Z

...k/plugins/apm/public/components/app/service_overview/service_overview_errors_table/index.tsx

@@ -152,7 +153,7 @@ export function ServiceOverviewErrorsTable({ serviceName }: Props) {
    },
    status,
  } = useFetcher(() => {
-    if (!start || !end) {
+    if (!start || !end || !transactionType) {


For readability, something like:

const hasRequiredParams = start && end && transactionType; if (!hasRequiredParams) { return; }

We are already using the "or not" style so I think it's fine to keep it. At least that's consistent

ogupte

LGTM

smith · 2020-12-14T23:27:14Z

retest

smith · 2020-12-15T01:24:02Z

retest

smith · 2020-12-15T05:38:29Z

retest

formgeist · 2020-12-15T10:41:28Z

I noticed some inconsistencies where we will filter out any other type in the search bar in the Transactions view, but show other types in the suggestions even if we're scoping the Overview to request.

Overview

Transactions

sorenlouv · 2020-12-15T13:11:53Z

I noticed some inconsistencies where we will filter out any other type in the search bar in the Transactions view, but show other types in the suggestions even if we're scoping the Overview to request.

Good catch. This happens because we scope the results based on url params and we don't add the transaction type to the url in this case. Doing that would be an easy fix:

kibana/x-pack/plugins/apm/public/components/shared/KueryBar/get_bool_filter.ts

Lines 49 to 53 in 20638a6

    
           if (urlParams.transactionType) { 
        
             boolFilter.push({ 
        
               term: { [TRANSACTION_TYPE]: urlParams.transactionType }, 
        
             }); 
        
           }

That being said, since this is not a blocker, we can merge this as-is before FF and then follow up with a bug fix.

smith · 2020-12-15T23:08:28Z

@elasticmachine merge upstream

sorenlouv · 2020-12-15T23:24:46Z

x-pack/plugins/apm/public/components/app/service_overview/service_overview.test.tsx

@@ -86,6 +86,8 @@ describe('ServiceOverview', () => {
        isAggregationAccurate: true,
      },
      'GET /api/apm/services/{serviceName}/dependencies': [],
+      // eslint-disable-next-line @typescript-eslint/naming-convention
+      'GET /api/apm/services/{serviceName}/service_overview_instances': [],


What is eslint complaining about here?

Property name GET /api/apm/services/{serviceName}/error_groups must match one of the following formats: camelCase, PascalCase, snake_case, UPPER_CASE

smith · 2020-12-16T22:28:59Z

retest

smith · 2020-12-17T00:24:27Z

retest

smith · 2020-12-17T02:21:59Z

retest

* Ensure transactionType is used in overview Fix the places where it was not being used: - Transactions table - Error rate chart - Errors table Also fix broken column sorting on transaction table. * fix a sort * test fix * Snapshot update * adds missing required param transactionType to API test Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Oliver Gupte <[email protected]>

* Ensure transactionType is used in overview Fix the places where it was not being used: - Transactions table - Error rate chart - Errors table Also fix broken column sorting on transaction table. * fix a sort * test fix * Snapshot update * adds missing required param transactionType to API test Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Oliver Gupte <[email protected]> Co-authored-by: Nathan L Smith <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

kibanamachine · 2020-12-18T21:26:02Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 337bf54

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.4MB	5.4MB	+446.0B

Distributable file count

id	before	after	diff
`default`	47286	48046	+760

History

💚 Build #95309 succeeded 337bf54
💔 Build #95220 failed e2de0f7
💔 Build #95068 failed be7f8d3
💔 Build #95059 failed be7f8d3
💔 Build #95029 failed be7f8d3

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

dgieselaar · 2020-12-21T09:47:48Z

couple of APIs are not using transaction type, and a couple of requests happen twice (once without transaction type, once with): see #86614.

smith added 2 commits December 10, 2020 16:30

Ensure transactionType is used in overview

4e21dab

Fix the places where it was not being used: - Transactions table - Error rate chart - Errors table Also fix broken column sorting on transaction table.

fix a sort

73116db

smith requested a review from a team as a code owner December 10, 2020 22:35

botelastic bot added the Team:APM All issues that need APM UI Team support label Dec 10, 2020

smith added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0 labels Dec 10, 2020

kibanamachine and others added 2 commits December 14, 2020 10:10

Merge branch 'master' into nls/ttype

f516d49

Merge remote-tracking branch 'upstream/master' into nls/ttype

71ac869

ogupte reviewed Dec 14, 2020

View reviewed changes

ogupte approved these changes Dec 14, 2020

View reviewed changes

sorenlouv approved these changes Dec 15, 2020

View reviewed changes

smith added 4 commits December 15, 2020 10:16

Merge remote-tracking branch 'upstream/master' into nls/ttype

c04b539

Merge remote-tracking branch 'upstream/master' into nls/ttype

a7a4562

test fix

4e3152a

Merge remote-tracking branch 'upstream/master' into nls/ttype

63dc146

Merge branch 'master' into nls/ttype

a108339

sorenlouv reviewed Dec 15, 2020

View reviewed changes

smith added 2 commits December 16, 2020 13:47

Merge remote-tracking branch 'upstream/master' into nls/ttype

9d96a6f

Snapshot update

be7f8d3

ogupte added 3 commits December 17, 2020 13:09

Merge branch 'master' into nls/ttype

5e74652

Merge remote-tracking branch 'upstream/master' into nls/ttype

e2de0f7

adds missing required param transactionType to API test

337bf54

ogupte merged commit feecebf into elastic:master Dec 18, 2020

ogupte mentioned this pull request Dec 18, 2020

[7.x] Ensure transactionType is used in overview (#85665) #86413

Merged

ogupte mentioned this pull request Dec 18, 2020

[7.11] Ensure transactionType is used in overview (#85665) #86414

Merged

sorenlouv changed the title ~~Ensure transactionType is used in overview~~ [APM] Service overview: Ensure transactionType is used for by every component Dec 18, 2020

sorenlouv mentioned this pull request Dec 18, 2020

[APM] 7.11 Test plan #86511

Closed

dgieselaar self-assigned this Dec 21, 2020

dgieselaar added the apm:test-plan-done Pull request that was successfully tested during the test plan label Dec 21, 2020

dgieselaar removed their assignment Dec 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Service overview: Ensure transactionType is used for by every component #85665

[APM] Service overview: Ensure transactionType is used for by every component #85665

smith commented Dec 10, 2020 •

edited

Loading

elasticmachine commented Dec 10, 2020

smith commented Dec 14, 2020

smith commented Dec 14, 2020

ogupte Dec 14, 2020

sorenlouv Dec 15, 2020

ogupte left a comment

smith commented Dec 14, 2020

smith commented Dec 15, 2020

smith commented Dec 15, 2020

formgeist commented Dec 15, 2020

sorenlouv commented Dec 15, 2020 •

edited

Loading

smith commented Dec 15, 2020

sorenlouv Dec 15, 2020

smith Dec 16, 2020

smith commented Dec 16, 2020

smith commented Dec 17, 2020

smith commented Dec 17, 2020

kibanamachine commented Dec 18, 2020

dgieselaar commented Dec 21, 2020

[APM] Service overview: Ensure transactionType is used for by every component #85665

[APM] Service overview: Ensure transactionType is used for by every component #85665

Conversation

smith commented Dec 10, 2020 • edited Loading

elasticmachine commented Dec 10, 2020

smith commented Dec 14, 2020

smith commented Dec 14, 2020

ogupte Dec 14, 2020

Choose a reason for hiding this comment

sorenlouv Dec 15, 2020

Choose a reason for hiding this comment

ogupte left a comment

Choose a reason for hiding this comment

smith commented Dec 14, 2020

smith commented Dec 15, 2020

smith commented Dec 15, 2020

formgeist commented Dec 15, 2020

sorenlouv commented Dec 15, 2020 • edited Loading

smith commented Dec 15, 2020

sorenlouv Dec 15, 2020

Choose a reason for hiding this comment

smith Dec 16, 2020

Choose a reason for hiding this comment

smith commented Dec 16, 2020

smith commented Dec 17, 2020

smith commented Dec 17, 2020

kibanamachine commented Dec 18, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

Distributable file count

History

dgieselaar commented Dec 21, 2020

smith commented Dec 10, 2020 •

edited

Loading

sorenlouv commented Dec 15, 2020 •

edited

Loading