-
Notifications
You must be signed in to change notification settings - Fork 487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added view for showing detailed trace statistics #506
Conversation
Codecov Report
@@ Coverage Diff @@
## master #506 +/- ##
==========================================
- Coverage 93.56% 92.69% -0.87%
==========================================
Files 217 227 +10
Lines 5296 5911 +615
Branches 1362 1491 +129
==========================================
+ Hits 4955 5479 +524
- Misses 300 391 +91
Partials 41 41
Continue to review full report at Codecov.
|
Hi Philip, this view looks pretty good but I have a few questions:
|
Hi Everett, thanks for your feedback and questions!
|
Add a caption "Group by:" in front of the drop-downs. Is the first screenshot an example of Colored table? |
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.
This is a really fantastic feature and it's great to see this PR. Thank you!
I don't have time for a full review, but I left a few comments. Let me know if any of them don't make sense or there is some other issue with them.
hideSummary: Boolean(embedded && embedded.timeline.hideSummary), | ||
linkToStandalone: getUrl(id), | ||
nextResult: this.nextResult, | ||
onArchiveClicked: this.archiveTrace, | ||
onSlimViewClicked: this.toggleSlimView, | ||
onTraceGraphViewClicked: this.toggleTraceGraphView, | ||
onTraceGraphViewClicked: this.toogleTraceView, |
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.
I suggest renaming the prop to onTraceViewChange
since it is no longer specific to the trace graph view.
))} | ||
(() => { | ||
switch (selectedTraceView) { | ||
case 0: |
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.
This code doesn't indicate selectedTraceView
is actually referring to the index
of the current selection from the menuItems
in AltViewOptions.tsx
. The ordering of values in menuItems
must match this switch statement, and this isn't particular evident. This might be hard to follow for someone looking at it without the context of this PR.
I think it would be easier to follow if you create an enum for the trace view types in packages/jaeger-ui/src/components/TracePage/types.tsx
. These can then be the value passed through the callback. Also, the array of values in AltViewOptions.tsx
can refer to the enum and define the label.
enum ETraceViewType {
TraceGraph = 'TraceGraph',
// etc.
}
const menuItems = [
{
viewType: ETraceViewType.TraceGraph,
label: 'Trace Graph',
},
// etc.
}
const { onTraceGraphViewClicked, traceGraphView, traceID } = props; | ||
const { onTraceGraphViewClicked, traceID, selectedTraceView } = props; | ||
|
||
const menuItems = ['Trace Timeline', 'Trace Graph', 'Trace Overview']; |
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.
This can be a top-level value because it doesn't change / is not based on the arguments to the function.
<Button | ||
className="ub-mr2" | ||
htmlType="button" | ||
onClick={() => onTraceGraphViewClicked(selectedTraceView + 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.
This + 1
is kind of odd. At first I thought repeatedly clicking the button would go from the timeline view to the graph and then get stuck on the trace overview. Then I checked toggleTraceView
and saw that it cycles back to the front if index > 2
.
Originally, when there were only the Trace Timeline and Trace Graph views, this button was intended to show the label for the current view and clicking it would toggle to the other view (reference to the PR).
Now with the three trace views and the two JSON views (and potentially more trace views later), I think there is a really a diminishing return on value of the button cycling through the three trace views, especially considering it ignores the JSON views when cycling, which makes it less intuitive (IMO).
I suggest having this button should show the label of the current view and do nothing when the button itself is clicked. Currently, the dropdown options are shown when the user hovers on the button. It seems fine for the user to select an item from the list. IMO, that's not much work and is not likely to be confusing.
@everett980, @yurishkuro what do you think?
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.
I suggest having this button should show the label of the current view and do nothing when the button itself is clicked.
+1
We had discussed doing this even before this PR. So rather than complicating the button I agree it should only be a dropdown and not a button.
@@ -187,14 +188,14 @@ export function TracePageHeaderFn(props: TracePageHeaderEmbedProps & { forwarded | |||
ref={forwardedRef} | |||
resultCount={resultCount} | |||
textFilter={textFilter} | |||
navigable={!traceGraphView} | |||
navigable={!(selectedTraceView !== 0)} |
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.
Why not use selectedTraceView === 0
?
@@ -0,0 +1,67 @@ | |||
/* |
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.
For consistency (e.g. with UiFindInput.*
), the file should be named PopupSql.css
@@ -0,0 +1,40 @@ | |||
// Copyright (c) 2018 The Jaeger Authors. |
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.
For consistency (e.g. with UiFindInput.*
), the file should be named PopupSql.css
limitations under the License. | ||
*/ | ||
|
||
#title { |
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.
Using IDs in CSS selectors is an anti-pattern. What's the advantage of using an ID here instead of a CSS class?
Also, can you add a few screenshots showing the various states of the UI? Also, I meant to add, in your second screenshot (img), the indentation in the first column looks a like it is not working right. Lastly, what do you think of also indenting the values in the second column, the count column? I'm wondering if that will make the roll-up / grouping more obvious. |
The first screenshot shows the view grouped by only one value. The next screenshot shows the possibilities to group them further. Tags are marked with "Tag:". The view is grouped by two values. This should show the interaction with the search. The search is for "api-gateway". The found hits are marked yellow. By clicking on a column, the detailed view below is hidden. In this screen the column with “api-gateway” was clicked and the "Operation Names" for this “Service Name” were hidden. This shows the coloring of the detail view according to the value of the ST in Duration. At sql tags it is possible to display the complete sql in a popup by clicking on the column. |
I finally got a chance to play around with this branch and I have to say it's very clear how this will be useful! Now that I've tried it, some follow up questions/feedback:
A caption would be helpful. If you change the
I'm still not sure how useful % ST in span duration is. A 10s span with 40% ST is a better opportunity for optimization than a 0.2s span with 100% ST but the latter will be much redder.
Indenting sub-group numerical metrics would be nice, especially on large traces where a single group may have many sub-groups.
|
28bebc9
to
7e0e25b
Compare
Signed-off-by: Philip Dengler <[email protected]>
Signed-off-by: Philip Dengler <[email protected]>
@everett980 do you have any idea why the test "fetchTrace: encountered a declaration exception" is failing? |
@fylip97 Looking at the failing test, it seems to use random numbers. It seems to have a 1e-5 chance of failing. Maybe you got unlucky (and maybe I'm misreading the probability, but definitely see |
could we not mock random for the tests? |
I looks like the code deliberately tries to avoid this issue but has a 1/100000 chance of not avoiding it enough. It should be easy to simply add a small number to one of the two random numbers and it wouldn't happen. |
Signed-off-by: Philip Dengler <[email protected]>
@fylip97 I know I wrote a bit of a saga, but can you let me know your thoughts on these questions? Also, if the UI has changed in your commits since 1/13, can you please add updated screenshots? Lastly, many of the comments left by tiffon are now labeled as |
Signed-off-by: Philip Dengler <[email protected]>
Hi @everett980,
|
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.
Sorry it took me so long to get through this all. I really like the changes made with the checkbox and drop downs.
I've left a fair bit of feedback, but most of it should be pretty quickly actionable.
Please double check the copyright years for new files throughout. Any new file should be 2020.
Lastly, I think the CSS for widths needs to be reworked. I noticed in the CSS that the widths specified are very precise. Then, with a selected subgroup, the widths seem slightly broken.
Not selected:
Selected:
I mentioned how to stop using float. With that change, it should be much easier to style the column widths without exact percentages. The first column can simply flex grow and the rest can be default table styling widths that should accommodate their largest item. If necessary, a maximum width can be added to the other columns, with text overflow ellipsis and a tool tip.
Related: I have noticed that text in the first column can be truncated. For SQL tags, it is possible to view the entire tag in a modal. For everything else, the approach I mentioned for long durations should be used (an ellipsis (already implemented) and a tooltip).
|
||
type Props = { | ||
onTraceGraphViewClicked: () => void; | ||
traceGraphView: boolean; | ||
onTraceViewChange: (actualViewType: ETraceViewType) => void; |
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.
what does "actual" mean here?
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.
Done.
}; | ||
|
||
const menuItems = [ |
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.
const MENU_ITEMS
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.
Done.
return ( | ||
<Dropdown overlay={menu}> | ||
<Button className="ub-mr2" htmlType="button" onClick={handleToggleView}> | ||
Alternate Views <Icon type="down" /> | ||
<Button className="ub-mr2" htmlType="button"> |
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.
This shouldn't be an action-less button, instead it should be a span.
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.
Done.
@@ -49,7 +49,7 @@ type TracePageHeaderEmbedProps = { | |||
nextResult: () => void; | |||
onArchiveClicked: () => void; | |||
onSlimViewClicked: () => void; | |||
onTraceGraphViewClicked: () => void; | |||
onTraceViewChange: (actualViewType: ETraceViewType) => void; |
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.
again, why actual?
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.
Done.
@@ -0,0 +1,54 @@ | |||
/* | |||
Copyright (c) 2018 The Jaeger Authors. |
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.
please update to 2020
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.
Done.
<Button className="ub-mr2" htmlType="button" onClick={handleToggleView}> | ||
Alternate Views <Icon type="down" /> | ||
<Button className="ub-mr2" htmlType="button"> | ||
{menuItems.find(test => test.viewType === viewType) !== undefined |
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.
I'd calculate the dropdownText
outside of the return value. and I think this:
const currentItem = menuItems.find(item => item.viewType === viewType);
const dropdownText = currentItem ? currentItem.label : 'Alternate Views';
is clearer and better handles the (presumably impossible) case of an invalid current viewType.
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.
Done.
{traceGraphView ? 'Trace Timeline' : 'Trace Graph'} | ||
</a> | ||
</Menu.Item> | ||
{menuItems.map(item => |
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.
This would be clearer as:
{menuItems.filter(item => item.viewType !== viewType).map(item => (
<Menu.Item key={item.viewType}>
{/* ... */}
</Menu.Item>
))}
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.
Done.
expect(props.onTraceGraphViewClicked).toHaveBeenCalledTimes(1); | ||
|
||
wrapper.setProps({ traceGraphView: false }); | ||
it('track dropdown menu', () => { |
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.
due to the increase in views, this test is getting slightly out of hand. I would rework it as follows:
const viewInteractions = [{
link: 'Trace Graph',
trackFn: trackGraphView,
onTraceViewChangeArg: ETraceViewType.TraceGraph,
}, {
link: 'Trace Statistics',
trackFn: trackStatisticsView,
onTraceViewChangeArg: ETraceViewType.TraceStatisticsView,
propViewType: ETraceViewType.TraceGraph,
}, {
link: 'Trace Timeline',
trackFn: trackGanttView,
onTraceViewChangeArg: ETraceViewType.TraceTimelineViewer,
propViewType: ETraceViewType.TraceStatisticsView
}];
viewInteractions.forEach(({ link, trackFn, onTraceViewChangeArg, propViewType }, i) => {
if (propViewType) {
wrapper.setProps({ viewType: propViewType });
}
expect(props.onTraceViewChange).toHaveBeenCalledTimes(i);
expect(trackFn).not.toHaveBeenCalled();
getLink(link).simulate('click');
expect(props.onTraceViewChange).toHaveBeenCalledTimes(i + 1);
expect(props.onTraceViewChange).toHaveBeenLastCalledWith(onTraceViewChangeArg);
viewInteractions.forEach(({ trackFn: fn, j }) => {
expect(fn).toHaveBeenCalledTimes(j <= i ? 1 : 0);
});
});
['Avg', 'avg'], | ||
['Min', 'min'], | ||
['Max', 'max'], | ||
['Total ST', 'self'], |
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.
please update this to ['ST Total', 'selfTotal'],
, for consistency.
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.
Likewise, please update the corresponding column header of the table.
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.
Done.
['ST Avg', 'selfAvg'], | ||
['ST Min', 'selfMin'], | ||
['ST Max', 'selfMax'], | ||
['ST Duration', 'percent'], |
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.
please update this to 'ST in Duration'
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.
Done.
Hi @fylip97, just wanted to check in on this PR. About a month ago you replied to nearly all of my feedback saying they've been addressed. If you'd like help with the last two items or don't have time to complete that yourself, you can push the changes you have done and I can review the changes and tackle the last two items. |
Signed-off-by: Philip Dengler <[email protected]>
Hi @everett980 |
@fylip97 @everett980 Thank you for this great feature. Is there any way I can assist in pushing this PR? |
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.
Apologies it took me so long to get to this. I got pulled in another direction.
This feature looks great and I'm super excited to get it out in front of people.
🎉 🎉 🎉 |
Great! Happy to see this feature being approved! |
* Added view for showing detailed trace statistics * Reworked trace statistics ui * review included * feedback added Signed-off-by: Philip Dengler <[email protected]> * Handle merge conflict, clean up AltViewOptions style Signed-off-by: Everett Ross <[email protected]> Co-authored-by: Philip <[email protected]> Co-authored-by: Everett Ross <[email protected]> Signed-off-by: vvvprabhakar <[email protected]>
* Added view for showing detailed trace statistics * Reworked trace statistics ui * review included * feedback added Signed-off-by: Philip Dengler <[email protected]> * Handle merge conflict, clean up AltViewOptions style Signed-off-by: Everett Ross <[email protected]> Co-authored-by: Philip <[email protected]> Co-authored-by: Everett Ross <[email protected]> Signed-off-by: vvvprabhakar <[email protected]>
* Added view for showing detailed trace statistics * Reworked trace statistics ui * review included * feedback added Signed-off-by: Philip Dengler <[email protected]> * Handle merge conflict, clean up AltViewOptions style Signed-off-by: Everett Ross <[email protected]> Co-authored-by: Philip <[email protected]> Co-authored-by: Everett Ross <[email protected]> Signed-off-by: vvvprabhakar <[email protected]>
Signed-off-by: Philip Dengler [email protected]
Which problem is this PR solving?
Short description of the changes