[APM] Consistent flyout headers by dgieselaar · Pull Request #46312 · elastic/kibana

dgieselaar · 2019-09-22T10:43:20Z

Closes #46078 and #46074.

Trace summary:

=>

Transaction details flyout:

=>

Span details flyout:

=>

Error occurrence summary:

=>

elasticmachine · 2019-09-22T12:20:18Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

dgieselaar · 2019-09-23T09:13:25Z

Really like how this looks, great work from the design team 👍. Do the badges perhaps need tooltips to explain type/subtype/action?

formgeist · 2019-09-23T10:21:42Z

Do the badges perhaps need tooltips to explain type/subtype/action?

Yes, I think those are still necessary on hover.

elasticmachine · 2019-09-23T10:52:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sorenlouv · 2019-09-23T11:17:47Z

I like this. Great work 👍
I feel the separator below the "[Name] / Service / Transaction" section is still needed. What do you think @formgeist ?

formgeist · 2019-09-23T11:29:55Z

I feel the separator below the "[Name] / Service / Transaction" section is still needed. What do you think @formgeist ?

Tbh, I think there are already too many grey dividers in the flyouts, so I'd rather go ahead without since now there's also a big enough difference in their styles. Before we had the same label/value look and feel.

formgeist

Just a minor addition; looks very good @dgieselaar

Btw. did we settle on including the user.agent data in this PR?

sorenlouv · 2019-09-23T11:43:41Z

Instead of having a labels object you can use the select operator:

'The % of {parentType, select, span {span} trace {trace} other {transaction}} exceeds 100% because this {childType, select, span {span} trace {trace} other {transaction}} takes longer than the root transaction.'

To re-use the select you can probably do something like:

const getTranslatedType = type => i18n.translate('xpack.apm.transactionDetails.type', '{type, select, span {span} trace {trace} other {transaction}}', { type });

Actually... I'm not sure this is better... 🤔 You decide.

select is much better indeed. I've used your first suggestion (with some minor changes).

sorenlouv · 2019-09-23T12:18:19Z

Perhaps move this into Summary/index.tsx and change the type to:

items: Array<React.ReactElement | undefined>;

This way we can avoid the as React.ReactElement[] and falsy values are automatically removed

sorenlouv · 2019-09-23T12:23:40Z

I'm trying to recall if this is still necessary. In 6.x we used to only have the type field with dots as delimiter. But to move to 7.x the user had to migrate their data afair. So maybe this is no longer necessary? If you remove it you might want to double check with apm server team.

Checked with the server team, it can be removed.

sorenlouv · 2019-09-23T12:28:38Z

This is a good use-case for the null coalesce operator you ranted about :p

I really like that operator! I'm just empathetic towards the people that have to write the parsers 😬

sorenlouv · 2019-09-23T12:29:52Z

Using arbitrary units feels wrong.

This is the only way I can get the error badge to align with the other badges. I think that's because the others are wrapped in a tooltip which adds a padding/margin one way or the other. Do you have any suggestions?

If I'm getting nitpicky; Even with that added margin, the error badge is higher up than the other badges.

I think aligning it vertically (with flexbox or similar) is usually better than doing it manually based on pixels.

I've now opted for px(units.eighth). I tried centering it vertically but I couldn't get it to work in a reasonable amount of time.

sorenlouv · 2019-09-23T12:30:22Z

sorenlouv · 2019-09-23T12:38:21Z

result is a bit generic. What do you think about transactionResult to be more clear?

Fixed. Also renamed the component to TransactionResultSummaryItem.

sorenlouv · 2019-09-23T12:39:22Z

Since this is not re-used anywhere, perhaps better to keep it in TransactionSummary?

Moved it to TransactionSummary.tsx.

sorenlouv

Just a few nits. LGTM

elasticmachine · 2019-09-23T12:47:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

Closes elastic#46078.

sorenlouv · 2019-09-25T13:42:08Z

+import { px, units } from '../../../../public/style/variables';
+
+interface Props {
+  items: Array<React.ReactElement | null | undefined>;


Why both null and undefined? Can't we pick one?

We can, but that puts the onus on the consumer of this component to make sure that any falsey value is either null or undefined, which is not the case right now because we use {condition} && {component} in some places, rather than {condition} ? {component} : null.

(another great use case for the null-coalescing operator)

Okay, good point. Leave it be.

sorenlouv · 2019-09-25T13:45:43Z

+    : idx(transaction, _ => _.url.full);
+
+  if (url) {
+    const method = idx(transaction, _ => _.http.request.method) || '';


Perhaps make method optional, and then handle the undefined state in HttpInfoSummaryItem.

My assumption was that method is always set when url is set, and I just need to trick TS. Is that assumption incorrect?

I don't think transactions from the RUM agent has the method property. It creates a separate span for outgoing http requests.

Good one, I'll check.

elasticmachine · 2019-09-25T15:14:03Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

formgeist · 2019-09-25T16:36:16Z

@dgieselaar Just checking, but since you added the timezone to the absolute timestamp tooltip, do you reckon we close this issue too? #46074

smith

Looks great!

You might want to check with @katrin-freihofner if you haven't already about having the request method be bold.

smith · 2019-09-25T16:31:28Z

      <HttpInfoBadge title={undefined}>
        <EuiToolTip content={methodLabel}>
-          <strong>{method}</strong>
+          <>{method.toUpperCase()}</>


This was bold in the designs previously. Did we decide to change it?

@smith @katrin-freihofner I decided to remove the strong because it was rendering the label a little too strong IMO. I suppose I get that we wanted to differentiate between request method and URL, but is it really that important?

I think I originally suggested it, to differentiate it from the URL. I'm okay with removing it though

dgieselaar · 2019-09-25T18:11:19Z

@formgeist:

@dgieselaar Just checking, but since you added the timezone to the absolute timestamp tooltip, do you reckon we close this issue too? #46074

yeah, let's close it. I've added it to the description.

* [APM] Consistent flyout headers Closes elastic#46078 and elastic#46074. * Tooltips for span flyout badges * Review feedback

In elastic#46312, we moved the service name property to the StickySpanProperties component. However, the import of SERVICE_NAME was incorrect, resulting in the tooltip showing the string 'licensing' instead of 'service.name'. The import now correctly points at our elasticsearch field names file.

* [APM] Consistent flyout headers (#46312) * [APM] Consistent flyout headers Closes #46078 and #46074. * Tooltips for span flyout badges * Review feedback * Fix import for SERVICE_NAME in StickySpanProperties

In #46312, we moved the service name property to the StickySpanProperties component. However, the import of SERVICE_NAME was incorrect, resulting in the tooltip showing the string 'licensing' instead of 'service.name'. The import now correctly points at our elasticsearch field names file.

sorenlouv · 2019-10-21T10:56:18Z

Test plan: verified ✅

* [APM] Consistent flyout headers Closes elastic#46078 and elastic#46074. * Tooltips for span flyout badges * Review feedback

In elastic#46312, we moved the service name property to the StickySpanProperties component. However, the import of SERVICE_NAME was incorrect, resulting in the tooltip showing the string 'licensing' instead of 'service.name'. The import now correctly points at our elasticsearch field names file.

dgieselaar force-pushed the consistent-flyout-headers branch 2 times, most recently from 3a45c60 to 66c91d1 Compare September 23, 2019 09:09

dgieselaar marked this pull request as ready for review September 23, 2019 09:13

dgieselaar requested a review from a team September 23, 2019 09:13

formgeist reviewed Sep 23, 2019

View reviewed changes

Comment thread x-pack/legacy/plugins/apm/public/components/app/ErrorGroupDetails/DetailView/index.tsx Outdated

dgieselaar added release_note:enhancement v7.5.0 labels Sep 23, 2019

sorenlouv reviewed Sep 23, 2019

View reviewed changes

sorenlouv approved these changes Sep 23, 2019

View reviewed changes

dgieselaar added 3 commits September 25, 2019 13:50

[APM] Consistent flyout headers

6bb852d

Closes elastic#46078.

Tooltips for span flyout badges

dc909b2

Review feedback

8723da1

dgieselaar force-pushed the consistent-flyout-headers branch from f7eea3c to 8723da1 Compare September 25, 2019 13:22

sorenlouv reviewed Sep 25, 2019

View reviewed changes

smith approved these changes Sep 25, 2019

View reviewed changes

dgieselaar merged commit bca9750 into elastic:master Sep 25, 2019

dgieselaar deleted the consistent-flyout-headers branch September 25, 2019 18:11

This was referenced Sep 25, 2019

[APM] Display Trace sample timestamp in preferred timezone #46074

Closed

[7.x] [APM] Consistent flyout headers (#46312) #46620

Merged

dgieselaar added a commit to dgieselaar/kibana that referenced this pull request Sep 25, 2019

[APM] Consistent flyout headers (elastic#46312)

344c372

* [APM] Consistent flyout headers Closes elastic#46078 and elastic#46074. * Tooltips for span flyout badges * Review feedback

dgieselaar mentioned this pull request Sep 26, 2019

[APM] Fix tooltip for service name in span flyout #46680

Merged

sorenlouv added the apm-test-plan-7.5.0 label Oct 10, 2019

sorenlouv added the apm:test-plan-done Pull request that was successfully tested during the test plan label Oct 21, 2019

patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026

[APM] Consistent flyout headers (elastic#46312)

6c188f2

* [APM] Consistent flyout headers Closes elastic#46078 and elastic#46074. * Tooltips for span flyout badges * Review feedback

Conversation

dgieselaar commented Sep 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 22, 2019

💔 Build Failed

Uh oh!

dgieselaar commented Sep 23, 2019

Uh oh!

formgeist commented Sep 23, 2019

Uh oh!

elasticmachine commented Sep 23, 2019

💚 Build Succeeded

Uh oh!

sorenlouv commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

formgeist commented Sep 23, 2019

Uh oh!

formgeist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sorenlouv Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorenlouv Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorenlouv Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorenlouv Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorenlouv left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 23, 2019

💚 Build Succeeded

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dgieselaar commented Sep 22, 2019 •

edited

Loading

sorenlouv commented Sep 23, 2019 •

edited

Loading

sorenlouv Sep 23, 2019 •

edited

Loading

sorenlouv Sep 23, 2019 •

edited

Loading

sorenlouv Sep 23, 2019 •

edited

Loading

sorenlouv Sep 23, 2019 •

edited

Loading