Adds traces overview with mock data #22628

jasonrhodes · 2018-09-03T13:30:31Z

I moved the ImpactSparkline being used in the transactions overview to use a shared ImpactBar component, which is now also used in the traces overview. I played with it being the "red" color offered by EUI because IIUC impact is an alarming thing, but happy to put it back to any of the available EUI colors.

To do:

Tests
Convert some files to typescript
File PR for EUI to export types for some table/tab components
Finalize expected data schema and align mock data to that, at least

elasticmachine · 2018-09-03T13:42:00Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-09-03T13:58:10Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jasonrhodes · 2018-09-03T23:01:34Z

retest

elasticmachine · 2018-09-03T23:16:21Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

formgeist · 2018-09-04T12:47:13Z

x-pack/plugins/apm/public/components/shared/ImpactBar/index.tsx

+
+export function ImpactBar({ value, ...rest }: Props) {
+  return (
+    <EuiProgress size="l" value={value} max={100} color="danger" {...rest} />


@jasonrhodes you're right that the Impact is important and most likely is a cause for attention and worry, but I think I might prefer having this in a more subtle colour since it's not the most important element on the page. I agree that we shouldn't go back to the calm blue or even affirmative green ("all is good"), so perhaps changing it to warning is worth the try. The blue colour that was there was a legacy design from the Opbeat days.

@formgeist I'll try warning and post the screenshot -- if that still seems too aggressive esp with it being on every row, we could use one of the greys

@jasonrhodes awesome, let's check it out and take it from there 👍

@formgeist so for some reason, EUIProgress doesn't work with "warning" (possibly an EUI bug) -- for now these look like the choices. I've left it with "subdued" (gray) for now but lmk if you like another one here:

I prefer blue but I probably just have Stockholm Syndrome from the Opbeat days

I reckon we go with primary, the blue that we're used to. Sorry for having to go over this again !

x-pack/plugins/apm/public/components/app/Main/Home.tsx

sorenlouv · 2018-09-04T12:56:20Z

x-pack/plugins/apm/public/components/app/Main/routeConfig.tsx

+
+interface RenderArgs {
+  location: StringMap<any>;
+}


I just saw the third party breadcrumb helper we use does expose some types. Can we build on top of those (haven't checked so maybe the answer is no)
https://github.com/tsumug/react-router-breadcrumbs-hoc/blob/master/types/react-router-breadcrumbs-hoc/index.d.ts

It seems like we should be able to use these but ... I have no idea how haha

sorenlouv · 2018-09-04T12:56:42Z

x-pack/plugins/apm/public/components/app/ServiceOverview/ServiceList/__test__/List.test.js

@@ -8,7 +8,7 @@ import React from 'react';
 import { mount } from 'enzyme';

 import { MemoryRouter } from 'react-router-dom';
-import List from '../index';
+import { ServiceList } from '../index';


I prefer this naming 👍

formgeist

Just a couple of minor design polish comments added.. Otherwise LGTM 👍

sorenlouv

Looks good. Just a few comments

...m/public/components/app/ServiceOverview/ServiceList/__test__/__snapshots__/List.test.js.snap

x-pack/plugins/apm/public/components/shared/ManagedTable/index.tsx

sorenlouv · 2018-09-04T13:20:04Z

x-pack/plugins/apm/public/components/shared/ManagedTable/index.tsx

+import orderBy from 'lodash.orderby';
+import React, { Component } from 'react';
+
+// this should really be imported from EUI


sorenlouv · 2018-09-04T13:20:18Z

x-pack/plugins/apm/public/components/shared/ManagedTable/index.tsx

+      />
+    );
+  }
+}


Can the other table views also use this ManagedTable?

Everything should be able to ... I want to think about the name though ... maybe PaginatedTable? You have thoughts there?

I like PaginatedTable better than ManagedTable.

... just thinking whether this is something EUI should provide out of the box.

Yeah same, was thinking we could point them to what we're doing here and see if they want to inherit it upwards

I'm going to leave this as ManagedTable right now even though I don't like that much either, but it handles sorting for you too, and could potentially handle more things if EUI doesn't pull this "uncontrolled component" version in. We can keep noodling on a better name for it if we decide to keep it here and not in EUI.

sorenlouv · 2018-09-04T13:21:46Z

x-pack/plugins/apm/public/store/reactReduxRequest/traceList.js

+
+  // if (!(serviceName && start && end && transactionType)) {
+  //   return null;
+  // }


Why is this commented out?

It breaks everything right now (nothing ever shows up) so I need to look into why and whether these are the right checks for this view or not.

Ahh yeah probably serviceName and transactionType should go.

sorenlouv · 2018-09-04T13:24:29Z

@jasonrhodes I don't think this should be merged to master until we can get rid of the mocks. Perhaps it would be useful if we instead merge dt related PRs to a apm-dt branch.

jasonrhodes · 2018-09-04T13:43:38Z

@sqren definitely agree on holding off on master, i'll make the apm-dt branch later today and change this PR base to that one

elasticmachine · 2018-09-04T13:58:51Z

Pinging @elastic/apm-ui

elasticmachine · 2018-09-20T17:17:56Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sorenlouv · 2018-09-20T14:18:33Z

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

@@ -16,4 +18,5 @@ function mapStateToProps(state = {}) {
  };
 }

+// tslint:disable-next-line:no-default-export
 export default connect(mapStateToProps)(TraceOverview);


I think we should stop using default export.

sorenlouv · 2018-09-20T18:52:18Z

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

+
+function mapStateToProps(state = {}) {
+  return {
+    traceList: selectTraceList(state),


Isn't this unused, since you're using TraceListRequest?

Yeah, copy/paste from the ServiceList -- which uses this data here (I'm not totally sure I understand what it's doing there and if we need to repeat that for the Trace List?) -->

kibana/x-pack/plugins/apm/public/components/app/ServiceOverview/view.js

Lines 22 to 29 in 531777d

async checkForHistoricalData({ serviceList }) {

if (serviceList.status === STATUS.SUCCESS && isEmpty(serviceList.data)) {

const result = await loadAgentStatus();

if (!result.dataFound) {

this.setState({ historicalDataFound: false });

}

}

}

sorenlouv · 2018-09-20T19:02:06Z

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

+      <EuiSpacer />
+      <TraceListRequest
+        urlParams={urlParams}
+        render={({ data }: { data: object[] }) => (


RRR should export types (adds mental TODO)

right now a custom selector can the structure of what 'render' returns. Perhaps render should always return:

interface ReactReduxRequestResponse<T> { data: T: status: 'LOADING' || 'SUCCESS' || 'FAILURE', args: any[] }

... and then the return value from the selector becomes the value for data

... Btw. do you prefer using RRR for data fetching and state, or just stateful components. I still can't decide what I like better.

So far I'm good with the RRR stuff. I think I'd have to see how a stateful component with an HTTP request would look and work, I guess that would be without Redux?

sorenlouv · 2018-09-20T19:05:55Z

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

+  heading = 'No data found.',
+  subheading = 'Try another time range or reset the search filter.',
+  hideSubheading = false
+}: Props) {


I'd think that the Prop interface is redundant when types can be inferred from the default values - but not sure what best practice says.

I think I may put this file back to .js and remove all of this, and deal with it later.

OK so I removed this Props interface and there were no errors, but I only added it because there were errors, so I don't know what happened there. 🤷‍♂️

sorenlouv · 2018-09-20T19:07:05Z

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

-  hideSubheading: false
-};
-
+// tslint:disable-next-line:no-default-export
 export default EmptyMessage;


I think we should get rid of default exports.

sorenlouv · 2018-09-20T19:13:19Z

x-pack/plugins/apm/public/components/shared/ImpactBar/index.tsx

+import React from 'react';
+
+// TODO: extend from EUI's EuiProgress prop interface
+interface Props extends StringMap<any> {


Looks like this already exists elastic/eui#326

Yeah it's not clear to me how to access these. I'll poke around at it a little more tonight. If you have ideas, let me know! :)

sorenlouv · 2018-09-20T19:34:32Z

x-pack/plugins/apm/public/store/reactReduxRequest/traceList.js

+  reactReduxRequest => {
+    return withInitialData(reactReduxRequest[ID]);
+  }
+);


Are there any advantages to using re-select instead of:

export function selectTraceList(state) { return withInitialData(state.reactReduxRequest[ID]); }

Honestly don't remember now. I may have not realized withInitialData was doing a memoized reselect under the hood.

elasticmachine · 2018-09-20T21:12:25Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-09-22T02:46:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jasonrhodes · 2018-09-23T18:12:11Z

package.json

@@ -236,9 +235,10 @@
    "@types/node": "^8.10.20",
    "@types/prop-types": "^15.5.3",
    "@types/puppeteer": "^1.6.2",
-    "@types/react": "^16.3.14",
+    "@types/react": "16.3.14",


Problem here: by using ^ for this package, installing another @types/* package that depends on @types/react using the * semver range makes us upgrade to the latest version that our ranges allow, so installing @types/react-router-dom auto-upgraded this package to 16.4.x, and that version breaks a lot of our types stuff since we still use React 16.3.

Switching to ~ didn't help, because even upgrading to 16.3.18 breaks tons of our kibana typescript stuff. I don't think the @types/* packages really follow semver the same way as other packages do, similarly to how linting rulesets can't really follow semver, because every single change is most likely breaking in some way.

tl;dr since newer versions of @types/react cause lots of typescript errors in kibana, we should be explicit about the version that works with kibana and specify 16.3.14 directly.

Makes sense! Let's just double check with @spalger or @epixa to see if they have any objections.

Sounds good, I'm going to merge this now since it's not actually changing anything with the real deps, and if there are objections or considerations we can revert the package.json change later if we need to. Thanks!

No objections. It seems strange that it breaks so much though.

* 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

* 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 requested review from sorenlouv and formgeist September 4, 2018 11:14

jasonrhodes added the [zube]: In Progress label Sep 4, 2018

jasonrhodes self-assigned this Sep 4, 2018

formgeist reviewed Sep 4, 2018

View reviewed changes

x-pack/plugins/apm/public/components/app/Main/Home.tsx Outdated Show resolved Hide resolved

sorenlouv reviewed Sep 4, 2018

View reviewed changes

formgeist requested changes Sep 4, 2018

View reviewed changes

sorenlouv reviewed Sep 4, 2018

View reviewed changes

jasonrhodes changed the base branch from master to apm/dt September 4, 2018 13:58

jasonrhodes added the Team:APM All issues that need APM UI Team support label Sep 4, 2018

jasonrhodes added the WIP Work in progress label Sep 4, 2018

zube bot added [zube]: Inbox WIP Work in progress and removed WIP Work in progress [zube]: In Progress labels Sep 4, 2018

jasonrhodes added 7 commits September 20, 2018 13:01

Refactored transaction overview to use new managed table component

eea7dca

Fixes tab size

e62d105

Cleans up some tests and types

893d936

Cleans up types and tests

89c9b3e

kbn bootstrap changes to x-pack yarn.lock

a805f4c

Removed jsconfig file in apm

0600e6e

Updates snapshot tests

531777d

jasonrhodes force-pushed the apm/trace-overview branch from e8eac09 to 531777d Compare September 20, 2018 17:09

sorenlouv reviewed Sep 20, 2018

View reviewed changes

Reversed bogus yarn lock change in x-pack

a1df24d

jasonrhodes added 2 commits September 21, 2018 07:55

review feedback wip

c7a39a3

Resolves typescript issues

05a7188

jasonrhodes commented Sep 23, 2018

View reviewed changes

formgeist approved these changes Sep 24, 2018

View reviewed changes

jasonrhodes merged commit f1747c8 into elastic:apm/dt Sep 24, 2018

jasonrhodes deleted the apm/trace-overview branch September 24, 2018 14:04

zube bot added [zube]: Done and removed [zube]: In Progress labels Sep 24, 2018

jasonrhodes mentioned this pull request Sep 24, 2018

[APM] Hook up trace overview to real ES data/queries #23438

Closed

	async checkForHistoricalData({ serviceList }) {
	if (serviceList.status === STATUS.SUCCESS && isEmpty(serviceList.data)) {
	const result = await loadAgentStatus();
	if (!result.dataFound) {
	this.setState({ historicalDataFound: false });
	}
	}
	}

Adds traces overview with mock data #22628

Adds traces overview with mock data #22628

Conversation

jasonrhodes commented Sep 3, 2018 • edited Loading

elasticmachine commented Sep 3, 2018

💔 Build Failed

elasticmachine commented Sep 3, 2018

💔 Build Failed

jasonrhodes commented Sep 3, 2018

elasticmachine commented Sep 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

formgeist left a comment • edited Loading

Choose a reason for hiding this comment

sorenlouv left a 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

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 commented Sep 4, 2018

jasonrhodes commented Sep 4, 2018

elasticmachine commented Sep 4, 2018

elasticmachine commented Sep 20, 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

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

jasonrhodes Sep 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Sep 20, 2018

💔 Build Failed

elasticmachine commented Sep 22, 2018

💚 Build Succeeded

jasonrhodes Sep 23, 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

jasonrhodes commented Sep 3, 2018 •

edited

Loading

sorenlouv Sep 4, 2018 •

edited

Loading

formgeist left a comment •

edited

Loading

jasonrhodes Sep 20, 2018 •

edited

Loading

jasonrhodes Sep 23, 2018 •

edited

Loading