Redesign Trace Timeline #2736
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| /* | ||
| * Copyright 2015-2019 The OpenZipkin Authors | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except | ||
| * in compliance with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software distributed under the License | ||
| * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express | ||
| * or implied. See the License for the specific language governing permissions and limitations under | ||
| * the License. | ||
| */ | ||
| import PropTypes from 'prop-types'; | ||
| import React from 'react'; | ||
| import moment from 'moment'; | ||
| import { withStyles } from '@material-ui/styles'; | ||
| import Box from '@material-ui/core/Box'; | ||
| import Table from '@material-ui/core/Table'; | ||
| import TableBody from '@material-ui/core/TableBody'; | ||
| import TableCell from '@material-ui/core/TableCell'; | ||
| import TableRow from '@material-ui/core/TableRow'; | ||
| import Paper from '@material-ui/core/Paper'; | ||
|
|
||
| import { spanAnnotationPropTypes } from '../../../prop-types'; | ||
|
|
||
| const propTypes = { | ||
| annotation: spanAnnotationPropTypes.isRequired, | ||
| classes: PropTypes.shape({}).isRequired, | ||
| }; | ||
|
|
||
| const style = { | ||
| cell: { | ||
| paddingTop: '8px', | ||
|
There was a problem hiding this comment. Hi! Curious, isn't it better to use Makes it easier for global customization as well if there needs to be a global padding change in the future. |
||
| paddingBottom: '8px', | ||
| }, | ||
| }; | ||
|
|
||
| const SpanAnnotation = ({ annotation, classes }) => ( | ||
| <Box> | ||
| <Box fontSize="1.1rem" mb={0.5}> | ||
| {annotation.value} | ||
| </Box> | ||
| <Paper> | ||
| <Table> | ||
| <TableBody data-testid="span-annotation--table-body"> | ||
|
There was a problem hiding this comment. Probably not this PR, but it'd be good to strip these with https://www.npmjs.com/package/babel-plugin-react-remove-properties zipkin UI has tons of DOM, so these extra attributes look like they can have significant enough impact on performance compared to other apps. |
||
| { | ||
| [ | ||
| { | ||
| label: 'Start Time', | ||
| // moment.js only supports millisecond precision, however our timestamps have | ||
| // microsecond precision. So we use moment.js to generate the human readable time | ||
| // with just milliseconds and then append the last 3 digits of the timestamp | ||
| // which are the microseconds. | ||
| // NOTE: a.timestamp % 1000 would save a string conversion but drops | ||
| // leading zeros. | ||
| value: moment(annotation.timestamp / 1000).format('MM/DD HH:mm:ss.SSS') | ||
| + annotation.timestamp.toString().slice(-3), | ||
|
There was a problem hiding this comment. I guess the result of this would be something like I believe it's common in several programming languages and is coming to JS too: https://github.com/tc39/proposal-numeric-separator#references There was a problem hiding this comment. Also I think it would be useful to extract this functionalitity to something like a global util of this package? (especially for its comment) There was a problem hiding this comment. Thank you for your advice! |
||
| }, | ||
| { label: 'Relative Time', value: annotation.relativeTime }, | ||
| { label: 'Address', value: annotation.endpoint }, | ||
| ].map(e => ( | ||
| <TableRow key={e.label}> | ||
| <TableCell className={classes.cell} data-testid="span-annotation--label"> | ||
| {e.label} | ||
| </TableCell> | ||
| <TableCell className={classes.cell} data-testid="span-annotation--value"> | ||
| {e.value} | ||
| </TableCell> | ||
| </TableRow> | ||
| )) | ||
| } | ||
| </TableBody> | ||
| </Table> | ||
| </Paper> | ||
| </Box> | ||
| ); | ||
|
|
||
| SpanAnnotation.propTypes = propTypes; | ||
|
|
||
| export default withStyles(style)(SpanAnnotation); | ||
|
There was a problem hiding this comment. Also how about adding There was a problem hiding this comment. I want to think a little more about whether to use Before
After
Thank you for your advice! There was a problem hiding this comment. That's a nice improvement! I'm guessing there might be other good places like each individual trace (make sure not to rerender when expanding a trace) - but often it's just simplest to reason about to use
But will leave it to you how much you add it :) |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| /* | ||
| * Copyright 2015-2019 The OpenZipkin Authors | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except | ||
| * in compliance with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software distributed under the License | ||
| * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express | ||
| * or implied. See the License for the specific language governing permissions and limitations under | ||
| * the License. | ||
| */ | ||
| import React from 'react'; | ||
| import { createShallow } from '@material-ui/core/test-utils'; | ||
|
|
||
| import SpanAnnotation from './SpanAnnotation'; | ||
|
|
||
| describe('<SpanAnnotation />', () => { | ||
| let shallow; | ||
|
|
||
| beforeEach(() => { | ||
| shallow = createShallow(); | ||
| }); | ||
|
|
||
| it('should render annotation data', () => { | ||
| const wrapper = shallow( | ||
| <SpanAnnotation.Naked | ||
| annotation={{ | ||
| value: 'Server Start', | ||
| timestamp: 1543334627716006, // 11/28 01:03:47.716006 | ||
| relativeTime: '700ms', | ||
| endpoint: '127.0.0.1', | ||
| }} | ||
| classes={{}} | ||
| />, | ||
| ); | ||
| const rows = wrapper.find('[data-testid="span-annotation--table-body"]'); | ||
| expect(rows.childAt(0).find('[data-testid="span-annotation--label"]').text()).toBe('Start Time'); | ||
| expect(rows.childAt(0).find('[data-testid="span-annotation--value"]').text()).toBe('11/28 01:03:47.716006'); | ||
| expect(rows.childAt(1).find('[data-testid="span-annotation--label"]').text()).toBe('Relative Time'); | ||
| expect(rows.childAt(1).find('[data-testid="span-annotation--value"]').text()).toBe('700ms'); | ||
| expect(rows.childAt(2).find('[data-testid="span-annotation--label"]').text()).toBe('Address'); | ||
| expect(rows.childAt(2).find('[data-testid="span-annotation--value"]').text()).toBe('127.0.0.1'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First time looking at lens code :)
Out of curiosity, have you thought about using
Grid? Maybe it makes this sort of layout simpler.