-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[APM] One-line trace summary #44842
[APM] One-line trace summary #44842
Conversation
💔 Build Failed |
Really like how this looks in general 👍 Some things that caught my eye (will review code later):
|
<ErrorCountBadge | ||
errorCount={errorCount} | ||
includeText={true} | ||
title={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.
Why title={undefined}
? Can't that be dropped?
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.
(Same with other occurrences of title={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.
An EuiBadge
by default apparently sets the title attribute of the element to the contents of the element, so if you hover over "1 Error" you get a title tooltip that says "1 Error". This also occurs when an EuiToolTip
is present, so you get a weird double-tooltip.
// TODO: Light/Dark theme (@see https://github.com/elastic/kibana/issues/44840) | ||
const theme = euiLightVars; | ||
|
||
const Heading: React.FunctionComponent = () => ( |
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've never understood the appeal of these annotations (but I'm definitely in the minority here :D). If we are going to use them what do you think about the shorthand:
const Heading: React.FC = () => (
or perhaps even:
import React, { FC } from 'react';
// ...
const Heading: FC = () => (
?
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.
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.
Yeah, I can see that. However, I barely think you can find any usages of those props anywhere. I recall displayName
was needed for HOC (replaced by hooks),defaultProps
is replaced with es6 default parameters and propTypes
is replaced with Typescript.
I can't remember how contextTypes
- we don't use it even though we use context a lot.
So to me it is a solution to a problem I don't have :)
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 me it's less about the autocomplete and more about explicitly stating, "this is what this thing is."
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.
Makes sense. I find great value in Typescript's explicitness . It's like code comments that are guaranteed to be up to date.
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.
FWIW, @sqren previously argued that FC implicitly types children
which I think is a good reason to avoid it.
import { ErrorCountBadge } from './ErrorCountBadge'; | ||
import { truncate } from '../../../../style/variables'; | ||
|
||
// TODO: Light/Dark theme (@see https://github.com/elastic/kibana/issues/44840) |
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.
As mentioned in the issue we also have this one #29725
import { NOT_AVAILABLE_LABEL } from '../../../../../common/i18n'; | ||
import { isRumAgentName } from '../../../../../common/agent_name'; | ||
import { ErrorCountBadge } from './ErrorCountBadge'; | ||
import { truncate } from '../../../../style/variables'; |
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.
Unused?
const Separator: React.FunctionComponent = () => ( | ||
<> | ||
{' '} | ||
<SeparatorText aria-hidden={true}>|</SeparatorText>{' '} |
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.
Instead of using whitespace {' '}
for styling, you should add padding to the styled component.
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.
You could perhaps also opt for border-right: 1px solid ${euiColorMediumShade}
, and have a TraceSummaryBlock
component or something like that. Not sure if that would be better though.
} | ||
)} | ||
> | ||
<>({percentOfTraceText})</> |
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.
Is the fragment needed?
<>({percentOfTraceText})</> | |
({percentOfTraceText}) |
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.
Oh - is it because it's required by EuiToolTip
? Then it makes sense (although EuiToolTip
should just accept strings in the first place)
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.
Opened elastic/eui#2292 to track this.
color = theme.euiColorVis7; | ||
} else if (status >= 500) { | ||
color = theme.euiColorVis2; | ||
} |
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.
Perhaps extract this to a separate method const color = getStatusCodeColor(status)
return null; | ||
} else if (status < 300) { | ||
color = theme.euiColorVis0; | ||
} else if (status >= 300 || status < 400) { |
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.
Shouldn't this be &&
instead of ||
? afaict anything above 300 will always return euiColorVis5
} | ||
)} | ||
> | ||
<MethodText>{method}</MethodText> |
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.
Perhaps simply use <strong>{method}</strong>
?
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.
Or maybe that's abuse to use semantic tags for styling. I'm okay with either
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 an explicit styled component is probably safer and more "semantic", but I'll just use strong
because it's less code.
const isRumAgent = isRumAgentName(transaction.agent.name); | ||
const url = isRumAgent | ||
? idx(transaction, _ => _.transaction.page.url) || NOT_AVAILABLE_LABEL | ||
: idx(transaction, _ => _.url.full) || NOT_AVAILABLE_LABEL; |
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'm not 100% but I don't think a RUM transaction will ever have http
. We have to double check with @vigneshshanmugam or @jahtalab .
If that's the case we should remove the transaction.page.url
from HttpInfo
(since it will never be rendered) and instead render the RUM url as a separate use case aka:
if (isHttp) (
<HttpInfo />
} else if (result) {
<TransactionResult />
} else if (isRumAgent()) {
<RumAgentUrl />
}
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.
As @sqren mentioned, We don't have transaction.http
field at the moment for RUM agent.
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.
Thank @vigneshshanmugam. Do you plan to add it in the near-ish future?
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 changed this to remove the check for http which really wasn't necessary in the first place.
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.
Atleast for spans, we have the http info, But cant for transaction everything is grouped under page, request and response so wont be added in the nearish future.
@sqren do you have an example of a trace where it looked like that for you or was it all of them? Does not look like that for me. |
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.
Great work Nathan!
export const ErrorCountBadge = ({ errorCount = 0, ...rest }: Props) => ( | ||
export const ErrorCountBadge = ({ | ||
errorCount = 0, | ||
includeText = false, |
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.
instead of includeText
, maybe this component should just support children
? Also this label is pretty similar to xpack.apm.transactionDetails.errorsOverviewLink
, which we use in StickyTransactionProperties
.
transaction={transaction} | ||
/> | ||
) | ||
).not.toThrowError(); |
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.
how useful is this test?
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.
It asserts that the thing actually can render given those props. I usually use a test like this as a starting point and then use Jest coverage mapping or just TDD to fill in all the use cases.
So, it's not that useful but more useful than having nothing.
// TODO: Light/Dark theme (@see https://github.com/elastic/kibana/issues/44840) | ||
const theme = euiLightVars; | ||
|
||
const Heading: React.FunctionComponent = () => ( |
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.
FWIW, @sqren previously argued that FC implicitly types children
which I think is a good reason to avoid it.
); | ||
|
||
return ( | ||
<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.
EUIToolTip
supports the className
prop, so you might be able to use that with styled-components
:
const ToolTipWithoutWhitespace = styled(EUIToolTip)`
white-space: nowrap;
`;
(iirc)
const Separator: React.FunctionComponent = () => ( | ||
<> | ||
{' '} | ||
<SeparatorText aria-hidden={true}>|</SeparatorText>{' '} |
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.
You could perhaps also opt for border-right: 1px solid ${euiColorMediumShade}
, and have a TraceSummaryBlock
component or something like that. Not sure if that would be better though.
@@ -0,0 +1,255 @@ | |||
/* |
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.
Might be useful to split this file up? e.g. TraceSummary/{index,Duration,HttpStatusBadge}.tsx
}) => { | ||
let color = 'default'; | ||
|
||
if (!status) { |
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.
maybe this looks better as:
const colors = {
2: theme.euiColorVis0,
3: theme.euiColorVis5,
4: theme.euiColorVis7,
5: theme.euiColorVis2
};
const resultGroup = status ? status.substr(0) : null;
const color = colors[resultGroup] || null;
}; | ||
|
||
const HttpInfoBadge = styled(EuiBadge)` | ||
max-width: 250px; |
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 think we should use units here to make elements align better. Those units are located in x-pack/legacy/plugins/apm/public/style/variables.ts
.
)} | ||
> | ||
<MethodText>{method}</MethodText> | ||
</EuiToolTip>{' '} |
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.
it might make sense to use EUIFlexGrid
and EUIFlexItem
here, instead of using spaces (or even padding
. There's some additional benefits we get, responsiveness for instance (well, sort of).
duration={transaction.transaction.duration.us} | ||
totalDuration={totalDuration} | ||
/> | ||
{(isHttp || result) && <Separator />} |
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 think this can be improved a little. The conditions and different branches make it a little hard to follow for me. What do you think of:
const items = [
<Timestamp transactionTimestamp={transaction['@timestamp']} />,
<Duration
duration={transaction.transaction.duration.us}
totalDuration={totalDuration}
/>
];
if (isHttp) {
items.push(<HttpInfo transaction={transaction} />);
} else if (result) {
items.push(<Result result={result} />);
}
if (errorCount > 0) {
items.push(
<ErrorCountBadge
errorCount={errorCount}
includeText={true}
title={undefined}
/>
);
}
and then:
{items.reduce(
(acc, item, index) => {
const next = acc.concat(item);
if (index !== items.length - 1) {
next.push(<Separator />);
}
return next;
},
[] as React.ReactElement[]
)}
I think this would improve further if you use a EUIFlexItem with padding/border-right too.
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 find that much harder to follow, but I'll try to make what I have there easier to follow.
@dgieselaar @sqren these things seem to be in conflict: The timestamp and duration labels are relatively long... The summary relatively quickly starts wrapping... and URL shouldn't be truncated so aggressively In addition to status code we should also show status text, eg "200 Success" @katrin-freihofner would you mind taking a look to see if there's changes we can make to present more information and prevent it from wrapping so much? |
hehe, true :) I think there is a bit of truth to both though. We talked about using the short month format. That'll help a bit. If we do that I think it'll be possible to show both more of the url and have the status text without it wrapping. |
Regarding the status code colors: I don't think we should stick to the EUI colors. Normally I'm totally for following a color scheme but in this case it just looks really off (imo). I previously mentioned the colors Google Chrome (and many others) use: I thought we agreed on them but maybe not. @katrin-freihofner ? EDIT: Okay, maybe we should stick to the EUI color palette but at least still use green-ish for 2xx, yellow-ish for 3xx and red for 4xx and 5xx. |
Eui provides nice colors for indicating status: https://elastic.github.io/eui/#/utilities/color-palettes |
Yay! Those colors are much better! Thanks for pointing that out. |
@smith good work! I have two questions:
|
Since #42357 the errors badge in the header is no longer clickable. So I think it is fine as-is. |
I'm ok with that. Do we have any kind of team-specific style guide or do we suggest changes to the kibana JS style guide for decisions like this? |
@sqren not sure if we have this in the transaction data. |
No, we only have the status code. There are probably lots of libraries that can do this but keeping out own list is probably simpler. Porting it from this php gists should do the trick: https://gist.github.com/henriquemoody/6580488 |
https://github.com/prettymuchbryce/http-status-codes seems to be exactly what we want and it has no other dependencies. It's MIT licensed. Would it be better just to copy the code in or add as a dependency in x-pack? |
ccfb859
to
99099ea
Compare
Replace the `StickyTransactionProperties` with a trace summary that puts everything on one line. Fixes elastic#43247.
This looks great. Good job @smith ! I'm surprised why the status code/text is black on red (whereas the error is white on red): I think the contrast would be better with white: I think EUI does some contrast calculation and determines the color, so not something we should have to override. Just wondering why it decided for black instead of white. Any idea about this, @cchaos ? (we use |
…-to-np-ready * 'master' of github.com:elastic/kibana: (138 commits) [Canvas] i18n work on workpad header (and a few header CTAs) and convert to typescript (elastic#44943) update close/delete system index modals (elastic#45037) TS return type of createIndexPatternSelect (elastic#45107) [ML] Fix focus chart updating. (elastic#45146) [ML] Data frame transform: Fix progress in wizard create step. (elastic#45116) [Graph] Re-enable functional test (elastic#44683) [SIEM] unique table id for each top talkers table (elastic#45014) [SIEM] ip details heading draggable (elastic#45179) [Maps][File upload] Set complete on index pattern creation (elastic#44423) [Maps] unmount map embeddable component on destroy (elastic#45183) [SIEM] Adds error toasts to MapEmbeddable component (elastic#45088) fix redirect to maintain search query string (elastic#45184) [APM] One-line trace summary (elastic#44842) [Infra UI] Display non-metric details on Node Detail page (elastic#43551) [Maps][File upload] Removing bbox from parsed file pending upstream lib fix (elastic#45194) [Logs UI] Improve live streaming behavior when scrolling (elastic#44923) [APM] Fix indefinite loading state in agent settings for unauthorized user roles (elastic#44970) [Reporting] Rewrite addForceNowQuerystring to getFullUrls (elastic#44851) [Reporting/ESQueue] Improve logging of doc-update events (elastic#45077) [Reporting] Make screenshot capture less noisy by default (elastic#45185) ...
@cchaos we're using the euiPaletteForStatus from https://elastic.github.io/eui/#/utilities/color-palettes. Do you think these are the correct colors to use for HTTP status codes? |
That status palette is more for creating a larger spectrum between green and red. Though they can be used for badges as well, we haven't tested them in that capacity as they were originally created for charts (no text). |
@katrin-freihofner should we replace these with Secondary/Warning/Danger? I can make a ticket. |
@smith I guess this would work better. 👍 |
@katrin-freihofner Maybe it's just me, but I find the green in the status palette ( I've tried to find a bunch of examples of http status code colors, and even though they don't have exactly the same shades they all have green, yellow and red that closer corresponds to success, warning and error imo. https://www.steveschoger.com/status-code-poster/img/status-code.png |
@sqren we can use the Eui badge's |
Oh yes, sorry if that's what you meant in the first place. That's much better. What do you think about: |
2xx is definitely success and 5xx is definitely error. |
In my opinion, a warning is fine for 4xx. What if we use a more neutral color like |
Works for me 👍 We could use the same for 1xx |
@katrin-freihofner @sqren opened #45406 for color updates. |
Tested in Chrome and Firefox. This should also be cross-browser tested in IE. |
Summary
Replace the
StickyTransactionProperties
with a trace summary that puts everything on one line.Fixes #43247.
Some example screenshots
An HTTP request with an error:
An HTTP request with no errors:
A non-HTTP request with no result:
A non-HTTP request with an error an a result:
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.Documentation was added for features that require explanation or tutorialsFor maintainers