DT transaction sample header #24294

jasonrhodes · 2018-10-19T21:20:26Z

Summary

Closes #20657

elasticmachine · 2018-10-19T23:34:42Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-10-20T03:15:46Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-10-22T19:03:51Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

...ugins/apm/public/components/app/TransactionDetails/Transaction/TransactionPropertiesTable.js

sorenlouv · 2018-10-22T20:32:01Z

x-pack/plugins/apm/public/components/app/TransactionDetails/Transaction/ActionMenu.tsx

+      interval: 'auto',
+      query: {
+        language: 'lucene',
+        query: `${PROCESSOR_EVENT}:transaction AND ${TRANSACTION_ID}:${transactionId}`


In v2 transaction.id is not globally unique but only within a trace. So we need to add a trace.id if it's v2.

sorenlouv · 2018-10-22T20:38:40Z

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

  transaction: Transaction;
  docType: 'transaction';
  children?: Array<IWaterfallItemSpan | IWaterfallItemTransaction>;
 }

-interface IWaterfallItemSpan extends IWaterfallItemBase {
+export interface IWaterfallItemSpan extends IWaterfallItemBase {


Can we remove the export again, if it's not used anywhere?

Removed -- is there a danger in exporting and not using? I usually tend to export as a designation "this is something I intend to be usable outside this file", sort of a "public" vs "private" ... but definitely don't care that much.

Not a big deal. I think export is a nice signal to denote whether it's an implementation detail or not.
Also: if it's unexported and unused in the file, you'll get a lint error to delete it (which is nice).

sorenlouv · 2018-10-22T20:38:59Z

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

@@ -30,13 +30,13 @@ interface IWaterfallItemBase {
  offset: number;
 }

-interface IWaterfallItemTransaction extends IWaterfallItemBase {
+export interface IWaterfallItemTransaction extends IWaterfallItemBase {


Same as below (unused export)

sorenlouv · 2018-10-22T21:15:36Z

x-pack/plugins/apm/public/components/app/TransactionDetails/index.js

  return {
    location: state.location,
-    urlParams: getUrlParams(state)
+    urlParams: getUrlParams(state),
+    waterfall: waterfall && waterfall.data ? waterfall.data : null


If waterfall is not set it's undefined. Is there a reason for explicitly changing it to null? Keeping it as undefined also makes it possible to specify it as optional:

waterfall?: WaterfallResponse

which is a bit less verbose than:

waterfall: WaterfallResponse | null

Not sure it's a good argument, but TS teams advises against using null: https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines#null-and-undefined

sorenlouv · 2018-10-22T22:31:08Z

x-pack/plugins/apm/public/utils/url.tsx

@@ -4,33 +4,40 @@
 * you may not use this file except in compliance with the Elastic License.
 */

+import { EuiLink, EuiLinkAnchorProps } from '@elastic/eui';


Yay! Thanks for converting this file! Much used. Very profit.

sorenlouv · 2018-10-22T22:54:25Z

x-pack/plugins/apm/server/lib/helpers/transaction_group_query.ts

      p95: bucket.p95.values['95.0'],
      averageResponseTime,
      transactionsPerMinute,
      impact,
-      transactionType: oc(sample).transaction.type('n/a')
+      transactionType: oc(sample).transaction.type()


What do we use transactionType, serviceName, id and traceId for on transactionGroupList page?

sorenlouv · 2018-10-22T23:12:40Z

x-pack/plugins/apm/server/lib/helpers/transaction_group_query.ts


    return {
      name: bucket.key,
-      serviceName: oc(sample).context.service.name('n/a'),
-      id: oc(sample).transaction.id('n/a'),
+      serviceName: oc(sample).context.service.name(),


If we are sure there is a sample, we can be sure there is a serviceName. So we should be able do get it without oc

sorenlouv · 2018-10-22T23:13:51Z

x-pack/plugins/apm/server/lib/helpers/transaction_group_query.ts

-      id: oc(sample).transaction.id('n/a'),
+      serviceName: oc(sample).context.service.name(),
+      id: sample.transaction.id,
+      traceId: oc(sample).trace.id(),


Like mentioned above, I think the interface should be changed to:

export interface ITransactionGroup { name: string; sample: { transactionType: string; serviceName: string; transactionId: string; traceId?: string; }; p95: number; averageResponseTime: number; transactionsPerMinute: number; impact: number; }

To separate agg data, and sample data; avoid ambiguous id and make all fields except traceId required.

sorenlouv · 2018-10-22T23:16:31Z

x-pack/plugins/apm/typings/TransactionGroup.ts

  p95: number;
  averageResponseTime: number;
  transactionsPerMinute: number;
  impact: number;
-  transactionType: string;
+  transactionType?: string;


Afaict traceId is the only one that should be optional.

elasticmachine · 2018-10-23T04:09:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

x-pack/plugins/apm/public/components/app/TraceOverview/TraceList.tsx

sorenlouv · 2018-10-23T14:12:57Z

x-pack/plugins/apm/public/components/shared/TraceLink.tsx

  }

  return (
    <KibanaLink pathname="/app/apm" {...linkProps}>
-      {props.children}
+      {children}
    </KibanaLink>
  );
 };


Nice, this looks much cleaner now ✨

sorenlouv · 2018-10-23T14:16:09Z

x-pack/plugins/apm/server/lib/helpers/transaction_group_query.ts

-  context: Transaction['context'];
-  trace?: TransactionV2['trace'];
-}
-


Again: nice!
Striking the right balance between having accurate interfaces, but the fewer the better, is hard.

sorenlouv · 2018-10-23T14:16:19Z

x-pack/plugins/apm/server/lib/helpers/transaction_group_query.ts

@@ -49,7 +42,7 @@ export const TRANSACTION_GROUP_AGGREGATES = {
    aggs: {
      sample: {
        top_hits: {
-          _source: ['context', 'transaction', 'trace'],
+          // _source: ['context', 'transaction', 'trace'],


sorenlouv · 2018-10-23T14:17:26Z

x-pack/plugins/apm/typings/TransactionGroup.ts

  p95: number;
  averageResponseTime: number;
  transactionsPerMinute: number;
  impact: number;
-  transactionType?: string;
 }


Delete unused?

sorenlouv · 2018-10-23T15:13:14Z

x-pack/plugins/apm/public/components/app/TraceOverview/TraceList.tsx

  },
  {
    field: 'averageResponseTime',
    name: 'Avg. response time',
    sortable: true,
    dataType: 'number',
-    render: (value: number) => asMillisWithDefault(value * 1000)
+    render: (value: number) => asMillisWithDefault(value)


elasticmachine · 2018-10-23T15:22:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-10-23T16:23:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-10-23T17:21:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…uery params

elasticmachine · 2018-10-23T17:51:47Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-10-23T17:58:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jasonrhodes · 2018-10-23T18:08:50Z

Merging this after talking with @sqren, we'll let these tests all pass in the feature branch one last time before merging to master.

elasticmachine · 2018-10-23T20:25:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Adds traces overview with mock data (#22628) * Updates service overview snapshots * Adds tests for ManagedTable and ImpactBar * Refactored transaction overview to use new managed table component * Removed jsconfig file in apm * [APM] Distributed tracing - Trace details (waterfall) (#22763) * [APM] Add typescript to waterfall (#23635) * [APM] Migrate get_trace and constants to Typescript (#23634) * [APM] Add types for setup_request (#23762) * [APM] Adds trace overview queries and some refactoring (#23605) * ImpactBar component to align EuiProgress usage for impact bars * Sharing some logic between transaction and trace queries * Typescript support * Quick fix ‘banana’ * [APM] Ensure backwards compatibility for v1 and v2 (#23636) * Make interfaces versioned * Rename eventType to docType * Fixes trace links on traces overview (#24089) * [APM] use react-redux-request (#24117) * Updated yarn lockfile for new yarn version * Updated dependency issues for react-router-dom types * [APM] Display transaction info on span flyout (#24189) * [APM] Display transaction info on span flyout * Brings in real location and url param data for transaction flyout * Converts flyout to TS * Adds query param state for flyouts with ts support * Updates styles and uses EuiTabs for transaction flyout * [APM] Transaction flyout * [APM] Minor docs cleanup (#24325) * [APM] Minor docs cleanup * [APM] Fix issues with v1 spans (#24332) * [APM] Add agent marks (#24361) * [APM] Typescript migration for the transaction endpoints (#24397) * [APM] DT transaction sample header (#24294) Transaction sample header completed * Fixes link target for traces overview to include trans/trace ids as query params * Converts Transaction index file to TS * Adds trace link to sample section * Refactors the trace link and applies it to two usages * Implements transaction sample action context menu * Calculates and implements duration percentage * Re-typed how transaction groups work * Fixes transaction flyout links and context menu * Removes unnecessary ms multiplication * Removes unused commented code * Finalizes infra links * Fixes some type shenanigans

* Adds traces overview with mock data (#22628) * Adds traces overview with mock data * Updates service overview snapshots * Updates based on review feedback * Adds tests for ManagedTable and ImpactBar * Refactored transaction overview to use new managed table component * Fixes tab size * Cleans up some tests and types * Cleans up types and tests * kbn bootstrap changes to x-pack yarn.lock * Removed jsconfig file in apm * Updates snapshot tests * Reversed bogus yarn lock change in x-pack * review feedback wip * Resolves typescript issues * [APM] Distributed tracing - Trace details (waterfall) (#22763) * [APM] Add typescript to waterfall (#23635) * [APM] Migrate get_trace and constants to Typescript (#23634) * Bump lock files * [APM] Add types for setup_request (#23762) * [APM] Adds trace overview queries and some refactoring (#23605) Refactoring includes: * ImpactBar component to align EuiProgress usage for impact bars * Sharing some logic between transaction and trace queries * Typescript support * Quick fix ‘banana’ * [APM] Ensure backwards compatibility for v1 and v2 (#23636) Fix tests Make interfaces versioned Fix ts issues Rename eventType to docType WIP Working prototype Fix tests Limit get_transaction by trace.id * Fixes trace links on traces overview (#24089) * [APM] use react-redux-request (#24117) * [APM] use react-redux-request * Stricter type checking * Bump yarn lock files * Fix package.json * Updated yarn lockfile for new yarn version * Updated dependency issues for react-router-dom types * [APM] Display transaction info on span flyout (#24189) * [APM] Display transaction info on span flyout * Add links * Fix tests * Eui fixes * Remove color from span type * Fix failing tests * Adds versions to transaction and span itmes when they are returned from the API (#24186) * Timeline transaction flyout (#24027) * Duplicates transaction properties table for use in flyout, with dummy values * Brings in real location and url param data for transaction flyout * Converts flyout to TS * Adds query param state for flyouts with ts support * Updates styles and uses EuiTabs for transaction flyout * Updates type references after rebase * Updated index var name per review feedback * WIP transaction flyout * WIP transaction flyout * WIP * Fixes waterfall helpers after rebasing span flyout changes * Finalizes styling for transaction flyover from timeline * Fixes layout for span flyout header * Removed accidentally committed file (should be in other branch) * Small tweaks to the sticky property layout styling * Updates snapshot tests after sticky properties converting to EUI * Final review tweaks for TS * [APM] Minor docs cleanup (#24325) * [APM] Minor docs cleanup * Fix tests * Make `text` required * Simplify documentation module * Break docs into separate files * Remove `translateAgentName` method * Formatting * [APM] Fix issues with v1 spans (#24332) * [APM] Add agent marks (#24361) * Typescript migration for the transaction endpoints (#24397) * DT transaction sample header (#24294) Transaction sample header completed * Fixes link target for traces overview to include trans/trace ids as query params * Converts Transaction index file to TS * Adds trace link to sample section * Refactors the trace link and applies it to two usages * Implements transaction sample action context menu * Calculates and implements duration percentage * Re-typed how transaction groups work * Fixes transaction flyout links and context menu * Removes unnecessary ms multiplication * Removes unused commented code * Finalizes infra links * Fixes some type shenanigans

* Adds traces overview with mock data (#22628) * Updates service overview snapshots * Adds tests for ManagedTable and ImpactBar * Refactored transaction overview to use new managed table component * Removed jsconfig file in apm * [APM] Distributed tracing - Trace details (waterfall) (#22763) * [APM] Add typescript to waterfall (#23635) * [APM] Migrate get_trace and constants to Typescript (#23634) * [APM] Add types for setup_request (#23762) * [APM] Adds trace overview queries and some refactoring (#23605) * ImpactBar component to align EuiProgress usage for impact bars * Sharing some logic between transaction and trace queries * Typescript support * Quick fix ‘banana’ * [APM] Ensure backwards compatibility for v1 and v2 (#23636) * Make interfaces versioned * Rename eventType to docType * Fixes trace links on traces overview (#24089) * [APM] use react-redux-request (#24117) * Updated yarn lockfile for new yarn version * Updated dependency issues for react-router-dom types * [APM] Display transaction info on span flyout (#24189) * [APM] Display transaction info on span flyout * Brings in real location and url param data for transaction flyout * Converts flyout to TS * Adds query param state for flyouts with ts support * Updates styles and uses EuiTabs for transaction flyout * [APM] Transaction flyout * [APM] Minor docs cleanup (#24325) * [APM] Minor docs cleanup * [APM] Fix issues with v1 spans (#24332) * [APM] Add agent marks (#24361) * [APM] Typescript migration for the transaction endpoints (#24397) * [APM] DT transaction sample header (#24294) Transaction sample header completed * Fixes link target for traces overview to include trans/trace ids as query params * Converts Transaction index file to TS * Adds trace link to sample section * Refactors the trace link and applies it to two usages * Implements transaction sample action context menu * Calculates and implements duration percentage * Re-typed how transaction groups work * Fixes transaction flyout links and context menu * Removes unnecessary ms multiplication * Removes unused commented code * Finalizes infra links * Fixes some type shenanigans

jasonrhodes changed the base branch from master to apm/dt October 19, 2018 21:20

jasonrhodes requested a review from sorenlouv October 22, 2018 11:38

jasonrhodes force-pushed the dt-transaction-sample-header branch from 5144a37 to 5331c19 Compare October 22, 2018 13:15

sorenlouv requested changes Oct 22, 2018

View reviewed changes

sorenlouv reviewed Oct 23, 2018

View reviewed changes

x-pack/plugins/apm/public/components/app/TraceOverview/TraceList.tsx Show resolved Hide resolved

sorenlouv reviewed Oct 23, 2018

View reviewed changes

sorenlouv approved these changes Oct 23, 2018

View reviewed changes

jasonrhodes added 12 commits October 23, 2018 13:25

Fixes link target for traces overview to include trans/trace ids as q…

13e2860

…uery params

Converts Transaction index file to TS

2af89fd

Adds trace link component, WIP

5f8f71e

Adds trace link to sample section

d485241

Refactors the trace link and applies it to two usages

1a10771

Implements transaction sample action context menu

3cae189

Calculates and implements duration percentage

6c13648

Re-typed how transaction groups work

a6c6d6b

Clean up

fd44103

Fixes transaction flyout links and context menu

539f06e

Removes unnecessary ms multiplication

98ca438

Removes unused commented code

e6d5b74

jasonrhodes force-pushed the dt-transaction-sample-header branch from 5bfb67e to e6d5b74 Compare October 23, 2018 17:29

Finalizes infra links

db511da

Fixes some type shenanigans

c613b2a

jasonrhodes merged commit 60f2370 into elastic:apm/dt Oct 23, 2018

jasonrhodes deleted the dt-transaction-sample-header branch October 23, 2018 18:09

jasonrhodes restored the dt-transaction-sample-header branch October 23, 2018 18:40

jasonrhodes deleted the dt-transaction-sample-header branch October 23, 2018 18:40

jasonrhodes mentioned this pull request Oct 23, 2018

[APM] Link hostname to Infra UI #23315

Closed

sorenlouv mentioned this pull request Feb 21, 2019

[APM] Moving the date picker into APM #31311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DT transaction sample header #24294

DT transaction sample header #24294

jasonrhodes commented Oct 19, 2018

elasticmachine commented Oct 19, 2018

elasticmachine commented Oct 20, 2018

elasticmachine commented Oct 22, 2018

sorenlouv Oct 22, 2018

sorenlouv Oct 22, 2018 •

edited

Loading

jasonrhodes Oct 23, 2018

sorenlouv Oct 23, 2018

sorenlouv Oct 22, 2018

sorenlouv Oct 22, 2018

sorenlouv Oct 22, 2018

sorenlouv Oct 22, 2018

sorenlouv Oct 22, 2018 •

edited

Loading

sorenlouv Oct 22, 2018 •

edited

Loading

sorenlouv Oct 22, 2018

elasticmachine commented Oct 23, 2018

sorenlouv Oct 23, 2018

sorenlouv Oct 23, 2018

sorenlouv Oct 23, 2018

sorenlouv Oct 23, 2018

sorenlouv Oct 23, 2018

elasticmachine commented Oct 23, 2018

elasticmachine commented Oct 23, 2018

elasticmachine commented Oct 23, 2018

elasticmachine commented Oct 23, 2018

elasticmachine commented Oct 23, 2018

jasonrhodes commented Oct 23, 2018

elasticmachine commented Oct 23, 2018

DT transaction sample header #24294

DT transaction sample header #24294

Conversation

jasonrhodes commented Oct 19, 2018

Summary

elasticmachine commented Oct 19, 2018

💚 Build Succeeded

elasticmachine commented Oct 20, 2018

💔 Build Failed

elasticmachine commented Oct 22, 2018

💔 Build Failed

Choose a reason for hiding this comment

sorenlouv Oct 22, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

sorenlouv Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 23, 2018

💔 Build Failed

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

Choose a reason for hiding this comment

elasticmachine commented Oct 23, 2018

💔 Build Failed

elasticmachine commented Oct 23, 2018

💚 Build Succeeded

elasticmachine commented Oct 23, 2018

💚 Build Succeeded

elasticmachine commented Oct 23, 2018

💔 Build Failed

elasticmachine commented Oct 23, 2018

💔 Build Failed

jasonrhodes commented Oct 23, 2018

elasticmachine commented Oct 23, 2018

💚 Build Succeeded

sorenlouv Oct 22, 2018 •

edited

Loading

sorenlouv Oct 22, 2018 •

edited

Loading

sorenlouv Oct 22, 2018 •

edited

Loading