From 1584e45397cba1d164533c8d6547d43ae8129e71 Mon Sep 17 00:00:00 2001 From: Oliver Gupte Date: Fri, 7 Dec 2018 10:07:54 -0800 Subject: [PATCH 1/2] [APM] fixes #26525 - simplified the stackframe grouping algorithm - add support for `stackframe.exclude_from_grouping` - made the rendering more tolerant of edge cases --- .../shared/Stacktrace/LibraryFrames.tsx | 52 +- .../stacktraceUtils.test.ts.snap | 460 ++++++++++++++++++ .../__test__/stacktraceUtils.test.ts | 27 +- .../Stacktrace/__test__/stacktraces.json | 421 ++++++++++++++-- .../components/shared/Stacktrace/index.tsx | 47 +- .../shared/Stacktrace/stacktraceUtils.ts | 52 +- 6 files changed, 952 insertions(+), 107 deletions(-) create mode 100644 x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/__snapshots__/stacktraceUtils.test.ts.snap diff --git a/x-pack/plugins/apm/public/components/shared/Stacktrace/LibraryFrames.tsx b/x-pack/plugins/apm/public/components/shared/Stacktrace/LibraryFrames.tsx index bd282a6767ec7..e2bfdbe85a5a2 100644 --- a/x-pack/plugins/apm/public/components/shared/Stacktrace/LibraryFrames.tsx +++ b/x-pack/plugins/apm/public/components/shared/Stacktrace/LibraryFrames.tsx @@ -20,6 +20,26 @@ const LibraryFrameToggle = styled.div` user-select: none; `; +interface LibraryStackFrameProps { + codeLanguage?: string; + stackframe: Stackframe; +} + +const LibraryStackFrame: React.SFC = ({ + codeLanguage, + stackframe +}) => { + return hasSourceLines(stackframe) ? ( + + ) : ( + + ); +}; + interface Props { visible?: boolean; stackframes: Stackframe[]; @@ -33,6 +53,19 @@ export const LibraryFrames: React.SFC = ({ codeLanguage, onClick }) => { + if (stackframes.length === 0) { + return null; + } + + if (stackframes.length === 1) { + return ( + + ); + } + return (
@@ -44,18 +77,13 @@ export const LibraryFrames: React.SFC = ({
{visible && - stackframes.map((stackframe, i) => - hasSourceLines(stackframe) ? ( - - ) : ( - - ) - )} + stackframes.map((stackframe, i) => ( + + ))}
); diff --git a/x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/__snapshots__/stacktraceUtils.test.ts.snap b/x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/__snapshots__/stacktraceUtils.test.ts.snap new file mode 100644 index 0000000000000..767f6c287dc8d --- /dev/null +++ b/x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/__snapshots__/stacktraceUtils.test.ts.snap @@ -0,0 +1,460 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`stactraceUtils getGroupedStackframes should collapse the library frames into a set of grouped stackframes 1`] = ` +Array [ + Object { + "isLibraryFrame": false, + "stackframes": Array [ + Object { + "abs_path": "/app/server/routes.js", + "context": Object { + "post": Array [ + " req.body.lines.forEach(function (line) {", + " client.query('SELECT id FROM products WHERE id=$1', [line.id], next())", + ], + "pre": Array [ + " })", + "", + ], + }, + "exclude_from_grouping": false, + "filename": "server/routes.js", + "function": "", + "library_frame": false, + "line": Object { + "context": " client.query('SELECT id FROM customers WHERE id=$1', [req.body.customer_id], next())", + "number": 307, + }, + }, + ], + }, + Object { + "isLibraryFrame": true, + "stackframes": Array [ + Object { + "abs_path": "/app/node_modules/pg-pool/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/pg-pool/index.js", + "function": "BoundPool.", + "library_frame": true, + "line": Object { + "number": 137, + }, + }, + Object { + "abs_path": "/app/node_modules/generic-pool/lib/generic-pool.js", + "exclude_from_grouping": false, + "filename": "node_modules/generic-pool/lib/generic-pool.js", + "function": "dispense", + "library_frame": true, + "line": Object { + "number": 310, + }, + }, + Object { + "abs_path": "/app/node_modules/generic-pool/lib/generic-pool.js", + "exclude_from_grouping": false, + "filename": "node_modules/generic-pool/lib/generic-pool.js", + "function": "acquire", + "library_frame": true, + "line": Object { + "number": 391, + }, + }, + Object { + "abs_path": "/app/node_modules/pg-pool/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/pg-pool/index.js", + "function": "BoundPool.", + "library_frame": true, + "line": Object { + "number": 111, + }, + }, + Object { + "abs_path": "/app/node_modules/pg-pool/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/pg-pool/index.js", + "function": "Pool._promiseNoCallback", + "library_frame": true, + "line": Object { + "number": 75, + }, + }, + Object { + "abs_path": "/app/node_modules/pg-pool/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/pg-pool/index.js", + "function": "Pool.connect", + "library_frame": true, + "line": Object { + "number": 109, + }, + }, + ], + }, + Object { + "isLibraryFrame": false, + "stackframes": Array [ + Object { + "abs_path": "/app/server/db.js", + "context": Object { + "post": Array [ + "}", + "", + ], + "pre": Array [ + "", + "exports.client = function (cb) {", + ], + }, + "exclude_from_grouping": false, + "filename": "server/db.js", + "function": "exports.client", + "library_frame": false, + "line": Object { + "context": " pool.connect(cb)", + "number": 11, + }, + }, + Object { + "abs_path": "/app/server/routes.js", + "context": Object { + "post": Array [ + " if (err) return error(err, res)", + "", + ], + "pre": Array [ + " }", + "", + ], + }, + "exclude_from_grouping": false, + "filename": "server/routes.js", + "function": "", + "library_frame": false, + "line": Object { + "context": " db.client(function (err, client, done) {", + "number": 248, + }, + }, + ], + }, + Object { + "isLibraryFrame": true, + "stackframes": Array [ + Object { + "abs_path": "/app/node_modules/express/lib/router/layer.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/layer.js", + "function": "handle", + "library_frame": true, + "line": Object { + "number": 95, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/route.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/route.js", + "function": "next", + "library_frame": true, + "line": Object { + "number": 137, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/route.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/route.js", + "function": "dispatch", + "library_frame": true, + "line": Object { + "number": 112, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/layer.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/layer.js", + "function": "handle", + "library_frame": true, + "line": Object { + "number": 95, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "", + "library_frame": true, + "line": Object { + "number": 281, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "process_params", + "library_frame": true, + "line": Object { + "number": 335, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "next", + "library_frame": true, + "line": Object { + "number": 275, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "handle", + "library_frame": true, + "line": Object { + "number": 174, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "router", + "library_frame": true, + "line": Object { + "number": 47, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/layer.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/layer.js", + "function": "handle", + "library_frame": true, + "line": Object { + "number": 95, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "trim_prefix", + "library_frame": true, + "line": Object { + "number": 317, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "", + "library_frame": true, + "line": Object { + "number": 284, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "process_params", + "library_frame": true, + "line": Object { + "number": 335, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "next", + "library_frame": true, + "line": Object { + "number": 275, + }, + }, + ], + }, + Object { + "isLibraryFrame": false, + "stackframes": Array [ + Object { + "abs_path": "/app/server.js", + "context": Object { + "post": Array [ + " }", + "", + ], + "pre": Array [ + "app.use('/api', function (req, res, next) {", + " if (Math.random() > opbeansRedirectProbability) {", + ], + }, + "exclude_from_grouping": false, + "filename": "server.js", + "function": "", + "library_frame": false, + "line": Object { + "context": " return next()", + "number": 88, + }, + }, + ], + }, + Object { + "isLibraryFrame": true, + "stackframes": Array [ + Object { + "abs_path": "/app/node_modules/express/lib/router/layer.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/layer.js", + "function": "handle", + "library_frame": true, + "line": Object { + "number": 95, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "trim_prefix", + "library_frame": true, + "line": Object { + "number": 317, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "", + "library_frame": true, + "line": Object { + "number": 284, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "process_params", + "library_frame": true, + "line": Object { + "number": 335, + }, + }, + ], + }, + Object { + "isLibraryFrame": true, + "stackframes": Array [ + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": true, + "filename": "node_modules/express/lib/router/index.js", + "function": "next", + "library_frame": true, + "line": Object { + "number": 275, + }, + }, + ], + }, + Object { + "isLibraryFrame": true, + "stackframes": Array [ + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "", + "library_frame": true, + "line": Object { + "number": 635, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "next", + "library_frame": true, + "line": Object { + "number": 260, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "handle", + "library_frame": true, + "line": Object { + "number": 174, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "router", + "library_frame": true, + "line": Object { + "number": 47, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/layer.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/layer.js", + "function": "handle", + "library_frame": true, + "line": Object { + "number": 95, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "trim_prefix", + "library_frame": true, + "line": Object { + "number": 317, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "", + "library_frame": true, + "line": Object { + "number": 284, + }, + }, + Object { + "abs_path": "/app/node_modules/express/lib/router/index.js", + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "function": "process_params", + "library_frame": true, + "line": Object { + "number": 335, + }, + }, + ], + }, +] +`; diff --git a/x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/stacktraceUtils.test.ts b/x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/stacktraceUtils.test.ts index 217b94a3548e3..a8b55bc138a8d 100644 --- a/x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/stacktraceUtils.test.ts +++ b/x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/stacktraceUtils.test.ts @@ -5,21 +5,30 @@ */ import { Stackframe } from '../../../../../typings/APMDoc'; -import { getCollapsedLibraryFrames, hasSourceLines } from '../stacktraceUtils'; +import { getGroupedStackframes, hasSourceLines } from '../stacktraceUtils'; import stacktracesMock from './stacktraces.json'; const stackframeMockWithSource = stacktracesMock[0]; const stackframeMockWithoutSource = stacktracesMock[1]; describe('stactraceUtils', () => { - describe('getCollapsedLibraryFrames', () => { - it('should collapse the library frames into a set of grouped, nested stackframes', () => { - const result = getCollapsedLibraryFrames(stacktracesMock as Stackframe[]); - expect(result.length).toBe(3); - expect(result[0].libraryFrame).toBe(false); - expect(result[1].libraryFrame).toBe(true); - expect(result[1].stackframes).toHaveLength(2); // two nested stackframes - expect(result[2].libraryFrame).toBe(false); + describe('getGroupedStackframes', () => { + it('should collapse the library frames into a set of grouped stackframes', () => { + const result = getGroupedStackframes(stacktracesMock as Stackframe[]); + expect(result).toMatchSnapshot(); + }); + + it('should handle empty stackframes', () => { + const result = getGroupedStackframes([] as Stackframe[]); + expect(result).toHaveLength(0); + }); + + it('should handle one stackframe', () => { + const result = getGroupedStackframes([ + stacktracesMock[0] + ] as Stackframe[]); + expect(result).toHaveLength(1); + expect(result[0].stackframes).toHaveLength(1); }); }); diff --git a/x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/stacktraces.json b/x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/stacktraces.json index 5723d2e4ce4ae..f29df22cb08b0 100644 --- a/x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/stacktraces.json +++ b/x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/stacktraces.json @@ -1,67 +1,416 @@ [ { + "library_frame": false, + "exclude_from_grouping": false, + "filename": "server/routes.js", + "abs_path": "/app/server/routes.js", + "line": { + "number": 307, + "context": " client.query('SELECT id FROM customers WHERE id=$1', [req.body.customer_id], next())" + }, "function": "", - "libraryFrame": false, - "excludeFromGrouping": false, "context": { - "pre": ["", "app.get('/log-error', function (req, res) {"], + "pre": [ + " })", + "" + ], "post": [ - " if (err) {", - " res.status(500).send('could not capture error: ' + err.message)" + " req.body.lines.forEach(function (line) {", + " client.query('SELECT id FROM products WHERE id=$1', [line.id], next())" ] + } + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/pg-pool/index.js", + "abs_path": "/app/node_modules/pg-pool/index.js", + "line": { + "number": 137 + }, + "function": "BoundPool." + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/generic-pool/lib/generic-pool.js", + "abs_path": "/app/node_modules/generic-pool/lib/generic-pool.js", + "line": { + "number": 310 + }, + "function": "dispense" + }, + { + "exclude_from_grouping": false, + "library_frame": true, + "filename": "node_modules/generic-pool/lib/generic-pool.js", + "abs_path": "/app/node_modules/generic-pool/lib/generic-pool.js", + "line": { + "number": 391 + }, + "function": "acquire" + }, + { + "exclude_from_grouping": false, + "library_frame": true, + "filename": "node_modules/pg-pool/index.js", + "abs_path": "/app/node_modules/pg-pool/index.js", + "line": { + "number": 111 }, + "function": "BoundPool." + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/pg-pool/index.js", + "abs_path": "/app/node_modules/pg-pool/index.js", "line": { - "number": 17, - "context": " apm.captureError(new Error('foo'), function (err) {" + "number": 75 }, - "filename": "server/coffee.js", - "absPath": "/app/server/coffee.js" + "function": "Pool._promiseNoCallback" }, { - "function": "get", - "libraryFrame": true, - "excludeFromGrouping": false, - "context": {}, + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/pg-pool/index.js", + "abs_path": "/app/node_modules/pg-pool/index.js", "line": { - "number": 123, - "context": "" + "number": 109 }, - "filename": "express/get.js", - "absPath": "/node_modules/express/get.js" + "function": "Pool.connect" }, { - "function": "use", - "libraryFrame": true, - "excludeFromGrouping": false, + "library_frame": false, + "exclude_from_grouping": false, + "filename": "server/db.js", + "abs_path": "/app/server/db.js", + "line": { + "number": 11, + "context": " pool.connect(cb)" + }, + "function": "exports.client", "context": { - "pre": ["", "app.use('/log-error', function (req, res) {"], + "pre": [ + "", + "exports.client = function (cb) {" + ], "post": [ - " if (err) {", - " res.status(500).send('could not capture error: ' + err.message)" + "}", + "" ] + } + }, + { + "library_frame": false, + "exclude_from_grouping": false, + "filename": "server/routes.js", + "abs_path": "/app/server/routes.js", + "line": { + "number": 248, + "context": " db.client(function (err, client, done) {" + }, + "function": "", + "context": { + "pre": [ + " }", + "" + ], + "post": [ + " if (err) return error(err, res)", + "" + ] + } + }, + { + "exclude_from_grouping": false, + "library_frame": true, + "filename": "node_modules/express/lib/router/layer.js", + "abs_path": "/app/node_modules/express/lib/router/layer.js", + "line": { + "number": 95 + }, + "function": "handle" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/route.js", + "abs_path": "/app/node_modules/express/lib/router/route.js", + "line": { + "number": 137 + }, + "function": "next" + }, + { + "exclude_from_grouping": false, + "library_frame": true, + "filename": "node_modules/express/lib/router/route.js", + "abs_path": "/app/node_modules/express/lib/router/route.js", + "line": { + "number": 112 + }, + "function": "dispatch" + }, + { + "exclude_from_grouping": false, + "library_frame": true, + "filename": "node_modules/express/lib/router/layer.js", + "abs_path": "/app/node_modules/express/lib/router/layer.js", + "line": { + "number": 95 + }, + "function": "handle" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "abs_path": "/app/node_modules/express/lib/router/index.js", + "filename": "node_modules/express/lib/router/index.js", + "line": { + "number": 281 + }, + "function": "" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 335 + }, + "function": "process_params" + }, + { + "exclude_from_grouping": false, + "library_frame": true, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 275 + }, + "function": "next" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 174 + }, + "function": "handle" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 47 + }, + "function": "router" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/layer.js", + "abs_path": "/app/node_modules/express/lib/router/layer.js", + "line": { + "number": 95 }, + "function": "handle" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", "line": { - "number": 234, - "context": " apm.captureError(new Error('foo'), function (err) {" + "number": 317 }, - "filename": "express/use.js", - "absPath": "/node_modules/express/use.js" + "function": "trim_prefix" }, { - "function": "handleCoffee", - "libraryFrame": false, - "excludeFromGrouping": false, + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 284 + }, + "function": "" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "abs_path": "/app/node_modules/express/lib/router/index.js", + "filename": "node_modules/express/lib/router/index.js", + "line": { + "number": 335 + }, + "function": "process_params" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 275 + }, + "function": "next" + }, + { + "exclude_from_grouping": false, + "library_frame": false, + "filename": "server.js", + "abs_path": "/app/server.js", + "line": { + "number": 88, + "context": " return next()" + }, + "function": "", "context": { - "pre": ["", ""], + "pre": [ + "app.use('/api', function (req, res, next) {", + " if (Math.random() > opbeansRedirectProbability) {" + ], "post": [ - " reply(getCoffee(req.id));" + " }", + "" ] + } + }, + { + "exclude_from_grouping": false, + "library_frame": true, + "filename": "node_modules/express/lib/router/layer.js", + "abs_path": "/app/node_modules/express/lib/router/layer.js", + "line": { + "number": 95 }, + "function": "handle" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 317 + }, + "function": "trim_prefix" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 284 + }, + "function": "" + }, + { + "exclude_from_grouping": false, + "library_frame": true, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 335 + }, + "function": "process_params" + }, + { + "library_frame": true, + "exclude_from_grouping": true, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 275 + }, + "function": "next" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 635 + }, + "function": "" + }, + { + "exclude_from_grouping": false, + "library_frame": true, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 260 + }, + "function": "next" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 174 + }, + "function": "handle" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 47 + }, + "function": "router" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/layer.js", + "abs_path": "/app/node_modules/express/lib/router/layer.js", + "line": { + "number": 95 + }, + "function": "handle" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 317 + }, + "function": "trim_prefix" + }, + { + "exclude_from_grouping": false, + "library_frame": true, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", + "line": { + "number": 284 + }, + "function": "" + }, + { + "library_frame": true, + "exclude_from_grouping": false, + "filename": "node_modules/express/lib/router/index.js", + "abs_path": "/app/node_modules/express/lib/router/index.js", "line": { - "number": 45, - "context": " handleCoffee(req => {" + "number": 335 }, - "filename": "server/handleCoffee.js", - "absPath": "/app/server/handleCoffee.js" + "function": "process_params" } ] diff --git a/x-pack/plugins/apm/public/components/shared/Stacktrace/index.tsx b/x-pack/plugins/apm/public/components/shared/Stacktrace/index.tsx index e272d67680c6f..55a83abaeb79a 100644 --- a/x-pack/plugins/apm/public/components/shared/Stacktrace/index.tsx +++ b/x-pack/plugins/apm/public/components/shared/Stacktrace/index.tsx @@ -7,20 +7,17 @@ import { EuiTitle } from '@elastic/eui'; import { isEmpty } from 'lodash'; import React, { PureComponent } from 'react'; +import { Stackframe } from '../../../../typings/APMDoc'; import { CodePreview } from '../../shared/CodePreview'; import { EmptyMessage } from '../../shared/EmptyMessage'; // @ts-ignore import { Ellipsis } from '../../shared/Icons'; import { FrameHeading } from './FrameHeading'; import { LibraryFrames } from './LibraryFrames'; -import { - getCollapsedLibraryFrames, - hasSourceLines, - StackframeCollapsed -} from './stacktraceUtils'; +import { getGroupedStackframes, hasSourceLines } from './stacktraceUtils'; interface Props { - stackframes?: StackframeCollapsed[]; + stackframes?: Stackframe[]; codeLanguage?: string; } @@ -44,7 +41,7 @@ export class Stacktrace extends PureComponent { } const hasAnyAppFrames = this.props.stackframes.some( - frame => !frame.libraryFrame + frame => !frame.library_frame ); if (!hasAnyAppFrames) { @@ -71,30 +68,32 @@ export class Stacktrace extends PureComponent {

Stack traces

 - {getCollapsedLibraryFrames(stackframes).map((item, i) => { - if (!item.libraryFrame) { - if (hasSourceLines(item)) { + {getGroupedStackframes(stackframes).map( + ({ isLibraryFrame, stackframes: groupedStackframes }, i) => { + if (isLibraryFrame) { return ( - this.toggle(i)} /> ); } - return ; + return groupedStackframes.map((stackframe, idx) => + hasSourceLines(stackframe) ? ( + + ) : ( + + ) + ); } - - return ( - this.toggle(i)} - /> - ); - })} + )} ); } diff --git a/x-pack/plugins/apm/public/components/shared/Stacktrace/stacktraceUtils.ts b/x-pack/plugins/apm/public/components/shared/Stacktrace/stacktraceUtils.ts index 37df3f6c2162b..ac7485ac400c9 100644 --- a/x-pack/plugins/apm/public/components/shared/Stacktrace/stacktraceUtils.ts +++ b/x-pack/plugins/apm/public/components/shared/Stacktrace/stacktraceUtils.ts @@ -7,35 +7,35 @@ import { get, isEmpty } from 'lodash'; import { Stackframe } from '../../../../typings/APMDoc'; -export interface StackframeCollapsed extends Stackframe { - libraryFrame?: boolean; - stackframes?: Stackframe[]; +interface StackframesGroup { + isLibraryFrame: boolean; + stackframes: Stackframe[]; } -export function getCollapsedLibraryFrames( +export function getGroupedStackframes( stackframes: Stackframe[] -): StackframeCollapsed[] { - return stackframes.reduce((acc: any, stackframe: StackframeCollapsed) => { - if (!stackframe.libraryFrame) { - return [...acc, stackframe]; - } - - // current stackframe is library frame - const prevItem: StackframeCollapsed = acc[acc.length - 1]; - if (!get(prevItem, 'libraryFrame')) { - return [...acc, { libraryFrame: true, stackframes: [stackframe] }]; - } - - return [ - ...acc.slice(0, -1), - { - ...prevItem, - stackframes: prevItem.stackframes - ? [...prevItem.stackframes, stackframe] - : [stackframe] - } - ]; - }, []); +): StackframesGroup[] { + if (stackframes.length === 0) { + return []; + } + + const isLibraryFrame = Boolean(stackframes[0].library_frame); + + let i = 0; + while ( + i < stackframes.length && + isLibraryFrame === stackframes[i].library_frame && + !stackframes[i].exclude_from_grouping + ) { + i++; + } + + const stackFrameEndIndex = i || 1; + + return [ + { isLibraryFrame, stackframes: stackframes.slice(0, stackFrameEndIndex) }, + ...getGroupedStackframes(stackframes.slice(stackFrameEndIndex)) + ]; } export function hasSourceLines(stackframe: Stackframe) { From 6977f302655559b400a32726f69278c11c7abc34 Mon Sep 17 00:00:00 2001 From: Oliver Gupte Date: Mon, 10 Dec 2018 23:11:59 -0800 Subject: [PATCH 2/2] Made improvements to code readability and added more meaningful test cases --- ...braryFrames.tsx => LibraryStackFrames.tsx} | 2 +- .../__test__/stacktraceUtils.test.ts | 136 ++++++++++++++++++ .../components/shared/Stacktrace/index.tsx | 24 ++-- .../shared/Stacktrace/stacktraceUtils.ts | 31 ++-- 4 files changed, 165 insertions(+), 28 deletions(-) rename x-pack/plugins/apm/public/components/shared/Stacktrace/{LibraryFrames.tsx => LibraryStackFrames.tsx} (97%) diff --git a/x-pack/plugins/apm/public/components/shared/Stacktrace/LibraryFrames.tsx b/x-pack/plugins/apm/public/components/shared/Stacktrace/LibraryStackFrames.tsx similarity index 97% rename from x-pack/plugins/apm/public/components/shared/Stacktrace/LibraryFrames.tsx rename to x-pack/plugins/apm/public/components/shared/Stacktrace/LibraryStackFrames.tsx index e2bfdbe85a5a2..2d857a39b0e6c 100644 --- a/x-pack/plugins/apm/public/components/shared/Stacktrace/LibraryFrames.tsx +++ b/x-pack/plugins/apm/public/components/shared/Stacktrace/LibraryStackFrames.tsx @@ -47,7 +47,7 @@ interface Props { onClick: () => void; } -export const LibraryFrames: React.SFC = ({ +export const LibraryStackFrames: React.SFC = ({ visible, stackframes, codeLanguage, diff --git a/x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/stacktraceUtils.test.ts b/x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/stacktraceUtils.test.ts index a8b55bc138a8d..8b47b082b0c2e 100644 --- a/x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/stacktraceUtils.test.ts +++ b/x-pack/plugins/apm/public/components/shared/Stacktrace/__test__/stacktraceUtils.test.ts @@ -18,6 +18,142 @@ describe('stactraceUtils', () => { expect(result).toMatchSnapshot(); }); + it('should group stackframes when `library_frame` is identical and `exclude_from_grouping` is false', () => { + const stackframes = [ + { + library_frame: false, + exclude_from_grouping: false, + filename: 'file-a.txt' + }, + { + library_frame: false, + exclude_from_grouping: false, + filename: 'file-b.txt' + }, + { + library_frame: true, + exclude_from_grouping: false, + filename: 'file-c.txt' + }, + { + library_frame: true, + exclude_from_grouping: false, + filename: 'file-d.txt' + } + ] as Stackframe[]; + + const result = getGroupedStackframes(stackframes); + + expect(result).toEqual([ + { + isLibraryFrame: false, + stackframes: [ + { + exclude_from_grouping: false, + filename: 'file-a.txt', + library_frame: false + }, + { + exclude_from_grouping: false, + filename: 'file-b.txt', + library_frame: false + } + ] + }, + { + isLibraryFrame: true, + stackframes: [ + { + exclude_from_grouping: false, + filename: 'file-c.txt', + library_frame: true + }, + { + exclude_from_grouping: false, + filename: 'file-d.txt', + library_frame: true + } + ] + } + ]); + }); + + it('should not group stackframes when `library_frame` is the different', () => { + const stackframes = [ + { + library_frame: false, + exclude_from_grouping: false, + filename: 'file-a.txt' + }, + { + library_frame: true, + exclude_from_grouping: false, + filename: 'file-b.txt' + } + ] as Stackframe[]; + const result = getGroupedStackframes(stackframes); + expect(result).toEqual([ + { + isLibraryFrame: false, + stackframes: [ + { + exclude_from_grouping: false, + filename: 'file-a.txt', + library_frame: false + } + ] + }, + { + isLibraryFrame: true, + stackframes: [ + { + exclude_from_grouping: false, + filename: 'file-b.txt', + library_frame: true + } + ] + } + ]); + }); + + it('should not group stackframes when `exclude_from_grouping` is true', () => { + const stackframes = [ + { + library_frame: false, + exclude_from_grouping: false, + filename: 'file-a.txt' + }, + { + library_frame: false, + exclude_from_grouping: true, + filename: 'file-b.txt' + } + ] as Stackframe[]; + const result = getGroupedStackframes(stackframes); + expect(result).toEqual([ + { + isLibraryFrame: false, + stackframes: [ + { + exclude_from_grouping: false, + filename: 'file-a.txt', + library_frame: false + } + ] + }, + { + isLibraryFrame: false, + stackframes: [ + { + exclude_from_grouping: true, + filename: 'file-b.txt', + library_frame: false + } + ] + } + ]); + }); + it('should handle empty stackframes', () => { const result = getGroupedStackframes([] as Stackframe[]); expect(result).toHaveLength(0); diff --git a/x-pack/plugins/apm/public/components/shared/Stacktrace/index.tsx b/x-pack/plugins/apm/public/components/shared/Stacktrace/index.tsx index 55a83abaeb79a..ddbc5b747799a 100644 --- a/x-pack/plugins/apm/public/components/shared/Stacktrace/index.tsx +++ b/x-pack/plugins/apm/public/components/shared/Stacktrace/index.tsx @@ -13,7 +13,7 @@ import { EmptyMessage } from '../../shared/EmptyMessage'; // @ts-ignore import { Ellipsis } from '../../shared/Icons'; import { FrameHeading } from './FrameHeading'; -import { LibraryFrames } from './LibraryFrames'; +import { LibraryStackFrames } from './LibraryStackFrames'; import { getGroupedStackframes, hasSourceLines } from './stacktraceUtils'; interface Props { @@ -21,17 +21,15 @@ interface Props { codeLanguage?: string; } -interface StateLibraryframes { - [i: number]: boolean; -} - interface State { - libraryframes: StateLibraryframes; + visibilityMap: { + [i: number]: boolean; + }; } export class Stacktrace extends PureComponent { public state = { - libraryframes: {} + visibilityMap: {} }; public componentDidMount() { @@ -46,18 +44,18 @@ export class Stacktrace extends PureComponent { if (!hasAnyAppFrames) { // If there are no app frames available, always show the only existing group - this.setState({ libraryframes: { 0: true } }); + this.setState({ visibilityMap: { 0: true } }); } } public toggle = (i: number) => - this.setState(({ libraryframes }) => { - return { libraryframes: { ...libraryframes, [i]: !libraryframes[i] } }; + this.setState(({ visibilityMap }) => { + return { visibilityMap: { ...visibilityMap, [i]: !visibilityMap[i] } }; }); public render() { const { stackframes = [], codeLanguage } = this.props; - const { libraryframes } = this.state as State; + const { visibilityMap } = this.state as State; if (isEmpty(stackframes)) { return ; @@ -72,9 +70,9 @@ export class Stacktrace extends PureComponent { ({ isLibraryFrame, stackframes: groupedStackframes }, i) => { if (isLibraryFrame) { return ( - this.toggle(i)} diff --git a/x-pack/plugins/apm/public/components/shared/Stacktrace/stacktraceUtils.ts b/x-pack/plugins/apm/public/components/shared/Stacktrace/stacktraceUtils.ts index ac7485ac400c9..0798d0a2d78b7 100644 --- a/x-pack/plugins/apm/public/components/shared/Stacktrace/stacktraceUtils.ts +++ b/x-pack/plugins/apm/public/components/shared/Stacktrace/stacktraceUtils.ts @@ -12,6 +12,17 @@ interface StackframesGroup { stackframes: Stackframe[]; } +function getNextGroupIndex(stackframes: Stackframe[]): number { + const isLibraryFrame = Boolean(stackframes[0].library_frame); + const groupEndIndex = + stackframes.findIndex( + stackframe => + isLibraryFrame !== Boolean(stackframe.library_frame) || + Boolean(stackframe.exclude_from_grouping) + ) || 1; + return groupEndIndex === -1 ? stackframes.length : groupEndIndex; +} + export function getGroupedStackframes( stackframes: Stackframe[] ): StackframesGroup[] { @@ -19,22 +30,14 @@ export function getGroupedStackframes( return []; } - const isLibraryFrame = Boolean(stackframes[0].library_frame); - - let i = 0; - while ( - i < stackframes.length && - isLibraryFrame === stackframes[i].library_frame && - !stackframes[i].exclude_from_grouping - ) { - i++; - } - - const stackFrameEndIndex = i || 1; + const nextGroupIndex = getNextGroupIndex(stackframes); return [ - { isLibraryFrame, stackframes: stackframes.slice(0, stackFrameEndIndex) }, - ...getGroupedStackframes(stackframes.slice(stackFrameEndIndex)) + { + isLibraryFrame: Boolean(stackframes[0].library_frame), + stackframes: stackframes.slice(0, nextGroupIndex) + }, + ...getGroupedStackframes(stackframes.slice(nextGroupIndex)) ]; }