[APM] Render related errors link in transaction flyout #29807

ogupte · 2019-02-01T09:04:53Z

[APM] closes #29563 by rendering related errors link with error count in transaction flyout

elasticmachine · 2019-02-01T09:51:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

.../app/TransactionDetails/Transaction/WaterfallContainer/Waterfall/TransactionFlyout/index.tsx

sorenlouv · 2019-02-01T11:06:47Z

x-pack/plugins/apm/server/lib/traces/get_trace.ts

+export interface TraceAPIResponse {
+  trace: TraceItems;
+  transactionErrorCounts: TransactionErrorCounts;
+}


I don't think TraceItems, or TransactionErrorCounts should be separate types. We can access them as TraceAPIResponse['trace'] and TraceAPIResponse['transactionErrorCounts']

x-pack/plugins/apm/server/lib/traces/get_trace.ts

sorenlouv · 2019-02-01T12:10:01Z

...ctionDetails/Transaction/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts

@@ -82,7 +84,8 @@ interface IWaterfallItemSpan extends IWaterfallItemBase {
 export type IWaterfallItem = IWaterfallItemSpan | IWaterfallItemTransaction;

 function getTransactionItem(
-  transaction: Transaction
+  transaction: Transaction,
+  transactionErrorCounts: TraceAPIResponse['transactionErrorCounts']


Can we call this errorCountPerTransaction or errorsPerTransaction?

sorenlouv · 2019-02-01T12:44:34Z

x-pack/plugins/apm/server/lib/traces/get_trace.ts

+    (
+      acc: object,
+      { key, errors: { doc_count } }: TraceErrorsAggregationBucket
+    ) => {


Maybe it's me, but I found this line a bit hard to parse. What do you think about:

(acc, bucket: TraceErrorsAggregationBucket) => {

(acc: object doesn't add anything afaict)

oh whoops. i meant to change the name before committing

sorenlouv · 2019-02-01T14:38:09Z

@ogupte Do you think it is too much for this PR to also get rid of the Transaction fetch (and getTransactionWithErrorCount), as we talked about in #29563 (comment)?

ogupte · 2019-02-01T16:28:08Z

@ogupte Do you think it is too much for this PR to also get rid of the Transaction fetch (and getTransactionWithErrorCount), as we talked about in #29563 (comment)?

I plan to add it to this PR. I just wanted feedback earlier on the solution of the core issue.

elasticmachine · 2019-02-04T07:58:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sorenlouv · 2019-02-04T14:11:48Z

x-pack/plugins/apm/server/lib/traces/get_trace.ts

+      'search',
+      errorsAggParams
+    )
+  ]).then(([traceResponse, errorsAggResponse]) => {


Thanks for changing this. ES query can sometimes end up long and confusing (especially if we need to transform the data before sending it to the client) so everywhere else we only have one query per file.
In this case it's not so big, but I'd still like that separation, since it makes it very easy to get an overview and test in separation.

sorenlouv · 2019-02-04T14:40:03Z

x-pack/plugins/apm/server/lib/traces/get_trace.ts

-  return resp.hits.hits.map(hit => hit._source);
+  return Promise.all([
+    client<Span | Transaction>('search', traceParams), // query for trace items (spans & transaction)
+    client<Span | Transaction, TraceErrorsAggResponse>( // agg for transaction error count


Span | Transaction is wrong for the error, isn't it? Should probably be never or something like that.

sorenlouv · 2019-02-04T14:46:42Z

x-pack/plugins/apm/server/lib/traces/get_trace.ts

+        };
+      },
+      {}
+    );


Personal nit: which of the two do find easier to read?

Current:

const errorsPerTransaction = errorsAggResponse.aggregations.transactions.buckets.reduce( (accumulatedErrorsPerTransaction, bucket: TraceErrorsAggBucket) => { const { key, errors: { doc_count } } = bucket; return { ...accumulatedErrorsPerTransaction, [key]: doc_count }; }, {} );

or adapted:

const errorsPerTransaction = errorsAggResponse.aggregations.transactions.buckets.reduce( (acc, bucket: TraceErrorsAggBucket) => ({ ...acc, [bucket.key]: bucket.errors.doc_count }) {} );

I realize this is very much personal pref.. I generally avoid the nested destructuring but I might be the odd one out :)
And I find acc descriptive enough most of the time. Unless I have nested reducers I'm never confused what it refers to.

I usually prefer nested destructuring followed by composition in smaller terms, but I don't feel strongly about it, so i can adapt. I'm happy to make these kinds of changes in the spirit of style cohesion and readability, but generally, I feel like code styles we want to enforce should be codified in the linting step, otherwise it can be rather subjective.

but generally, I feel like code styles we want to enforce should be codified in the linting step, otherwise it can be rather subjective

Good point 👍 This made me curious to see if there was an ESLint rule for this. The closest I got was this closed issue, so clearly there hasn't been overwhelming support for this idea.

sorenlouv · 2019-02-04T16:07:26Z

...ctionDetails/Transaction/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts

-  hits: Array<Span | Transaction>,
-  entryTransaction: Transaction
+  hits: TraceAPIResponse['trace'],
+  entryTransaction: Transaction,


When you make the other change we talked about, we will no longer have this entryTransaction. Instead we'll have a entryTransactionId

sorenlouv · 2019-02-04T16:12:38Z

...ctionDetails/Transaction/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts

@@ -256,7 +261,7 @@ export function getWaterfall(
        case 'span':


We no longer need the filter above (because we don't get error documents any longer). And the default case below is probably not that useful anymore (it seems overly defensive)

elasticmachine · 2019-02-05T06:27:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sorenlouv · 2019-02-05T09:01:48Z

x-pack/plugins/apm/server/lib/traces/get_trace_items.ts

+        bool: {
+          filter: [
+            { term: { [TRACE_ID]: traceId } },
+            { terms: { [PROCESSOR_EVENT]: ['span', 'transaction'] } },


Does this work? Can you provide an array of values to terms ?

Oh, I see you use terms instead of term. Didn't know about that. Awesome 🥇

sorenlouv · 2019-02-05T09:03:23Z

x-pack/plugins/apm/server/lib/traces/get_trace_errors_per_transaction.ts

+                }
+              }
+            }
+          }


I don't think you need the nested aggregation:

aggs: { errors: { filter: { term: { 'processor.event': 'error' } } } }

i forgot this was holdover from when all trace items were included in the returned set

sorenlouv · 2019-02-05T10:55:14Z

@ogupte While reviewing I noticed a test for getWaterfall was missing. That is something I should have added as part of the original implementation of getWaterfall but I must have forgotten it.
I added it to aid myself in reviewing, and thought we might as well use it, so I pushed it to your branch. Hope it's okay :)

sorenlouv · 2019-02-05T11:04:52Z

...ctionDetails/Transaction/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts

+      id
+    );
+    if (waterfallItemTransaction) {
+      return waterfallItemTransaction.errorCount;


A few comments about getWaterfallItemTransactionById, createGetTransactionById and createGetErrorCountByTransactionId:

createGetErrorCountByTransactionId
I don't think we need createGetErrorCountByTransactionId since we already have errorsPerTransaction which is a StringMap like itemsById. We can just return it as errorCountByTransactionId or errorsPerTransaction - whatever makes the naming consistent.

createGetTransactionById
I think createGetTransactionById can be simplified to:

function createTransactionById(hits: Array<Transaction | Span>) { const transactions = hits.filter( hit => hit.processor.event === 'transaction' ) as Transaction[]; return indexBy(transactions, 'transaction.id'); }

In this case I don't think we need getWaterfallItemTransactionById anymore.

elasticmachine · 2019-02-05T11:43:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sorenlouv · 2019-02-05T12:06:13Z

...ctionDetails/Transaction/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts

-  entryTransaction: Transaction
+  hits: TraceAPIResponse['trace'],
+  errorsPerTransaction: TraceAPIResponse['errorsPerTransaction'],
+  entryTransactionId?: Transaction['transaction']['id']


I'm not 100% decided on this, but I find it a bit easier to read more generic type annotations instead of the very custom ones. For instance:

hits: Array<Span | Transaction>, errorsPerTransaction: StringMap<number>, entryTransactionId?: string

This is totally up for debate. Just putting out some thoughts.

sorenlouv · 2019-02-05T12:57:54Z

x-pack/plugins/apm/server/lib/traces/get_trace_errors_per_transaction.ts

+  const { start, end, client, config } = setup;
+
+  const params: SearchParams = {
+    index: [config.get('apm_oss.errorIndices')],


Since this is querying the error index, should this file perhaps be located under "lib/errors"?
Not necessarily saying it should, just trying to find a system that makes sense.

sorenlouv · 2019-02-05T13:03:09Z

retest

ogupte · 2019-02-07T16:59:47Z

retest

elasticmachine · 2019-02-07T17:11:20Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-07T18:24:34Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

ogupte · 2019-02-07T19:40:58Z

retest

elasticmachine · 2019-02-07T21:35:59Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

ogupte · 2019-02-08T04:35:35Z

retest

elasticmachine · 2019-02-08T05:26:34Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…r count in transaction flyout

… queries, improved code org/readability

… files

…up the current transaction data within the trace (waterfall) data

…rrors_per_transaction.ts under 'errors'

…e waterfall helpers

…bar for related errors

elasticmachine · 2019-02-08T06:23:52Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

ogupte · 2019-02-08T06:25:32Z

retest

elasticmachine · 2019-02-08T07:16:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* [APM] closes elastic#29563 by rendering related errors link with error count in transaction flyout * [APM] improved get_trace query by narrowing indices and parallelizing queries, improved code org/readability * [APM] code improvements, split get_trace queries into separate source files * [APM] remove initial transaction details request in favor of looking up the current transaction data within the trace (waterfall) data * Add test for `getWaterfall` * Revert change to `getWaterfallItems` test * simplified aggregation, waterfall helpers code, and moved get_trace_errors_per_transaction.ts under 'errors' * improved naming and readbility of waterfall properties * removed unused routes and queries and fixed some invisible bugs in the waterfall helpers * added trace.id in addition to the transaction.id filter in the kuery bar for related errors

* [APM] closes #29563 by rendering related errors link with error count in transaction flyout * [APM] improved get_trace query by narrowing indices and parallelizing queries, improved code org/readability * [APM] code improvements, split get_trace queries into separate source files * [APM] remove initial transaction details request in favor of looking up the current transaction data within the trace (waterfall) data * Add test for `getWaterfall` * Revert change to `getWaterfallItems` test * simplified aggregation, waterfall helpers code, and moved get_trace_errors_per_transaction.ts under 'errors' * improved naming and readbility of waterfall properties * removed unused routes and queries and fixed some invisible bugs in the waterfall helpers * added trace.id in addition to the transaction.id filter in the kuery bar for related errors

* [APM] closes elastic#29563 by rendering related errors link with error count in transaction flyout * [APM] improved get_trace query by narrowing indices and parallelizing queries, improved code org/readability * [APM] code improvements, split get_trace queries into separate source files * [APM] remove initial transaction details request in favor of looking up the current transaction data within the trace (waterfall) data * Add test for `getWaterfall` * Revert change to `getWaterfallItems` test * simplified aggregation, waterfall helpers code, and moved get_trace_errors_per_transaction.ts under 'errors' * improved naming and readbility of waterfall properties * removed unused routes and queries and fixed some invisible bugs in the waterfall helpers * added trace.id in addition to the transaction.id filter in the kuery bar for related errors

ogupte self-assigned this Feb 1, 2019

ogupte requested a review from sorenlouv February 1, 2019 09:04

ogupte requested a review from a team as a code owner February 1, 2019 09:04

sorenlouv reviewed Feb 1, 2019

View reviewed changes

.../app/TransactionDetails/Transaction/WaterfallContainer/Waterfall/TransactionFlyout/index.tsx Outdated Show resolved Hide resolved

sorenlouv assigned sorenlouv and unassigned sorenlouv Feb 1, 2019

sorenlouv reviewed Feb 1, 2019

View reviewed changes

x-pack/plugins/apm/server/lib/traces/get_trace.ts Outdated Show resolved Hide resolved

sorenlouv reviewed Feb 1, 2019

View reviewed changes

ogupte force-pushed the apm-29563-related-errors-transaction-flyout branch from 349c7ae to e85a849 Compare February 4, 2019 07:20

sorenlouv reviewed Feb 4, 2019

View reviewed changes

ogupte force-pushed the apm-29563-related-errors-transaction-flyout branch from e85a849 to 0043961 Compare February 5, 2019 05:44

sorenlouv reviewed Feb 5, 2019

View reviewed changes

ogupte force-pushed the apm-29563-related-errors-transaction-flyout branch from ebbf0e0 to 97e0939 Compare February 7, 2019 16:58

ogupte and others added 10 commits February 7, 2019 21:28

[APM] closes elastic#29563 by rendering related errors link with erro…

deb8e2b

…r count in transaction flyout

[APM] improved get_trace query by narrowing indices and parallelizing…

45298fb

… queries, improved code org/readability

[APM] code improvements, split get_trace queries into separate source…

2ee57f3

… files

[APM] remove initial transaction details request in favor of looking …

86e76bb

…up the current transaction data within the trace (waterfall) data

Add test for getWaterfall

7c70d20

Revert change to getWaterfallItems test

c60ebff

simplified aggregation, waterfall helpers code, and moved get_trace_e…

f42e33e

…rrors_per_transaction.ts under 'errors'

improved naming and readbility of waterfall properties

9552031

removed unused routes and queries and fixed some invisible bugs in th…

a040942

…e waterfall helpers

added trace.id in addition to the transaction.id filter in the kuery …

e5d2f28

…bar for related errors

ogupte force-pushed the apm-29563-related-errors-transaction-flyout branch from 97e0939 to e5d2f28 Compare February 8, 2019 05:31

ogupte merged commit 7ae1b4f into elastic:master Feb 8, 2019

ogupte mentioned this pull request Feb 8, 2019

[7.x] [APM] Render related errors link in transaction flyout (#29807) #30480

Merged

ogupte mentioned this pull request Feb 8, 2019

[7.0] [APM] Render related errors link in transaction flyout (#29807) #30481

Merged

		@@ -256,7 +261,7 @@ export function getWaterfall(
		case 'span':

[APM] Render related errors link in transaction flyout #29807

[APM] Render related errors link in transaction flyout #29807

Conversation

ogupte commented Feb 1, 2019 • edited Loading

elasticmachine commented Feb 1, 2019

💚 Build Succeeded

sorenlouv Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

ogupte Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

sorenlouv commented Feb 1, 2019 • edited Loading

ogupte commented Feb 1, 2019

elasticmachine commented Feb 4, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

sorenlouv Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

sorenlouv Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

ogupte Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Feb 5, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Feb 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv commented Feb 5, 2019

sorenlouv Feb 5, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Feb 5, 2019

💔 Build Failed

Choose a reason for hiding this comment

sorenlouv Feb 5, 2019 • edited Loading

Choose a reason for hiding this comment

sorenlouv commented Feb 5, 2019

ogupte commented Feb 7, 2019

elasticmachine commented Feb 7, 2019

💔 Build Failed

elasticmachine commented Feb 7, 2019

💔 Build Failed

ogupte commented Feb 7, 2019

elasticmachine commented Feb 7, 2019

💔 Build Failed

ogupte commented Feb 8, 2019

elasticmachine commented Feb 8, 2019

💔 Build Failed

elasticmachine commented Feb 8, 2019

💔 Build Failed

ogupte commented Feb 8, 2019

elasticmachine commented Feb 8, 2019

💚 Build Succeeded

ogupte commented Feb 1, 2019 •

edited

Loading

sorenlouv Feb 1, 2019 •

edited

Loading

sorenlouv Feb 1, 2019 •

edited

Loading

ogupte Feb 1, 2019 •

edited

Loading

sorenlouv commented Feb 1, 2019 •

edited

Loading

sorenlouv Feb 4, 2019 •

edited

Loading

sorenlouv Feb 4, 2019 •

edited

Loading

ogupte Feb 4, 2019 •

edited

Loading

sorenlouv Feb 5, 2019 •

edited

Loading

sorenlouv Feb 5, 2019 •

edited

Loading

sorenlouv Feb 5, 2019 •

edited

Loading