[APM] Stacktrace heading styles #26456

ogupte · 2018-11-30T06:31:35Z

Fixes #23560 by factoring out FrameHeading from CodePreview components and only rendering CodePreview when the current stackframe has source context.

Fixes #26239 by checking for valid line number before displaying it

elasticmachine · 2018-11-30T07:05:51Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-11-30T07:30:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sorenlouv · 2018-11-30T12:22:23Z

x-pack/plugins/apm/public/components/shared/Stacktrace/FrameHeading.tsx

+  const FileDetail = isLibraryFrame
+    ? LibraryFrameFileDetail
+    : AppFrameFileDetail;
+  const lineNumber: number = get(stackframe, 'line.number');


Btw. we might be able to ditch ts-optchain and Lodash' get for idx: https://github.com/facebookincubator/idx

sorenlouv · 2018-12-01T15:22:52Z

x-pack/plugins/apm/public/components/shared/CodePreview/index.js

@@ -22,37 +22,24 @@ import python from 'react-syntax-highlighter/dist/languages/python';
 import ruby from 'react-syntax-highlighter/dist/languages/ruby';
 import { Variables } from './Variables';
 import { Context } from './Context';
+import { FrameHeading } from '../Stacktrace/FrameHeading';


Mind converting to ts?

sorenlouv · 2018-12-01T15:23:18Z

x-pack/plugins/apm/public/components/shared/Stacktrace/index.js

@@ -12,6 +12,7 @@ import { Ellipsis } from '../../shared/Icons';
 import { units, px } from '../../../style/variables';
 import { EmptyMessage } from '../../shared/EmptyMessage';
 import { EuiLink, EuiTitle } from '@elastic/eui';
+import { FrameHeading } from './FrameHeading';


Mind converting to ts?

sorenlouv · 2018-12-02T21:42:02Z

...ponents/app/TransactionDetails/Transaction/WaterfallContainer/Waterfall/SpanFlyout/index.tsx

+                      items={keys(tagContext).map(key => ({
+                        key,
+                        value: get(tagContext, key)
+                      }))}


Perhaps extract this transformation:

const items = keys(tagContext).map(key => ({ key, value: get(tagContext, key) }));

sorenlouv · 2018-12-02T21:43:01Z

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

@@ -22,9 +22,14 @@ export interface HttpContext {
  url?: string;
 }

+export interface TagsContext {


Is exporting needed (I don't see it used outside of here)

sorenlouv

All comments made.

elasticmachine · 2018-12-03T20:32:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sorenlouv · 2018-12-03T22:55:17Z

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

+        stackframes: prevItem.stackframes
+          ? [...prevItem.stackframes, stackframe]
+          : [stackframe]
+      }


Even though I originally wrote getCollapsedLibraryFrames I barely understand it now. Not because of your changes. It was just never very readable. So if you feel like simplifying/refactor/change it you are very welcome. And perhaps add a test for it 🙏

i pulled it into it's own file and added a very basic test...next, i'll amend this PR and take a try at simplifying this function

Sounds good 👍
Coincidentally a new issue was created directly related to this. Probably makes sense to refactor and fix the issue at the same time.

sorenlouv · 2018-12-03T22:59:07Z

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

+  onClick: () => void;
+}
+
+const Libraryframes: React.SFC<LibraryframesProps> = ({


Can you move this to its own file?
Generally I like the "one component per file"-ruke - unless the components are very simple/small.

sorenlouv · 2018-12-03T23:29:56Z

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

+import { FrameHeading } from './FrameHeading';
+
+export interface StackframeCollapsed extends Stackframe {
+  libraryFrame?: boolean;


Stackframe already has libraryFrame - with snake case:

kibana/x-pack/plugins/apm/typings/APMDoc.ts

Line 64 in 04a37c4

library_frame?: boolean;

We do some camelCasing here (that we should stop doing):

kibana/x-pack/plugins/apm/public/services/rest/callApi.ts

Line 36 in 04a37c4

return camelcase ? camelizeKeys(res) : res;

I'd suggest we disable the camelCasing for the spans endpoint:

kibana/x-pack/plugins/apm/public/services/rest/apm/transactions.ts

Line 14 in abeafa1

export async function loadSpans({

Like we did for the transactions endpoint:

kibana/x-pack/plugins/apm/public/services/rest/apm/transactions.ts

Lines 56 to 58 in abeafa1

{

camelcase: false

}

Eventually we can remove the camel casing altogether (I think spans (this) and errors are the only hold-ups).

i started down this path, but it ended up being a larger change than expected, since there are so many references to the camel cased properties. I'm not sure if i want to add that to this pr to create a new issue for it?

Okay, that makes sense. A new PR is probably a good idea. This PR lgtm.

…ew and only displaying code preview when stackframe has source code context

…splaying it

…ted Stackframe types

… tests

elasticmachine · 2018-12-04T03:44:33Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sorenlouv · 2018-12-04T12:19:50Z

x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/stacktraceUtils.test.ts

+      expect(result).toBe(false);
+    });
+  });
+});


Thanks 🙏

* [APM] fixes elastic#23560 by separating frame headers from code preview and only displaying code preview when stackframe has source code context * [APM] fixes elastic#26239 by checking for valid line number before displaying it * [APM] Converted CodePreview and Stacktace to typescript and consolidated Stackframe types * [APM] pull out components and util functions into own files and added tests

* [APM] fixes #23560 by separating frame headers from code preview and only displaying code preview when stackframe has source code context * [APM] fixes #26239 by checking for valid line number before displaying it * [APM] Converted CodePreview and Stacktace to typescript and consolidated Stackframe types * [APM] pull out components and util functions into own files and added tests

ogupte self-assigned this Nov 30, 2018

ogupte requested review from jasonrhodes, sorenlouv and formgeist November 30, 2018 06:31

sorenlouv reviewed Nov 30, 2018

View reviewed changes

sorenlouv reviewed Dec 1, 2018

View reviewed changes

sorenlouv reviewed Dec 2, 2018

View reviewed changes

ogupte force-pushed the apm-23560-stack-trace-heading-style branch from a152609 to fc2f2a5 Compare December 3, 2018 19:42

sorenlouv reviewed Dec 3, 2018

View reviewed changes

ogupte added 4 commits December 3, 2018 16:48

[APM] fixes elastic#23560 by separating frame headers from code previ…

81d9255

…ew and only displaying code preview when stackframe has source code context

[APM] fixes elastic#26239 by checking for valid line number before di…

562fe7b

…splaying it

[APM] Converted CodePreview and Stacktace to typescript and consolida…

03b37b5

…ted Stackframe types

[APM] pull out components and util functions into own files and added…

5012db3

… tests

ogupte force-pushed the apm-23560-stack-trace-heading-style branch from fc2f2a5 to 5012db3 Compare December 4, 2018 02:53

sorenlouv mentioned this pull request Dec 4, 2018

[APM] Library frames are not collapsed #26525

Closed

sorenlouv approved these changes Dec 4, 2018

View reviewed changes

ogupte merged commit 754aa25 into elastic:master Dec 4, 2018

ogupte mentioned this pull request Dec 4, 2018

[6.x] [APM] Stacktrace heading styles (#26456) #26668

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Stacktrace heading styles #26456

[APM] Stacktrace heading styles #26456

ogupte commented Nov 30, 2018 •

edited

Loading

elasticmachine commented Nov 30, 2018

elasticmachine commented Nov 30, 2018

sorenlouv Nov 30, 2018

sorenlouv Dec 1, 2018

sorenlouv Dec 1, 2018

sorenlouv Dec 2, 2018

sorenlouv Dec 2, 2018

sorenlouv left a comment

elasticmachine commented Dec 3, 2018

sorenlouv Dec 3, 2018 •

edited

Loading

ogupte Dec 4, 2018

sorenlouv Dec 4, 2018

sorenlouv Dec 3, 2018 •

edited

Loading

sorenlouv Dec 3, 2018

sorenlouv Dec 3, 2018

ogupte Dec 4, 2018

sorenlouv Dec 4, 2018

elasticmachine commented Dec 4, 2018

sorenlouv Dec 4, 2018

[APM] Stacktrace heading styles #26456

[APM] Stacktrace heading styles #26456

Conversation

ogupte commented Nov 30, 2018 • edited Loading

elasticmachine commented Nov 30, 2018

💔 Build Failed

elasticmachine commented Nov 30, 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

sorenlouv left a comment

Choose a reason for hiding this comment

elasticmachine commented Dec 3, 2018

💚 Build Succeeded

sorenlouv Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Dec 3, 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

elasticmachine commented Dec 4, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

ogupte commented Nov 30, 2018 •

edited

Loading

sorenlouv Dec 3, 2018 •

edited

Loading

sorenlouv Dec 3, 2018 •

edited

Loading