[APM] Remove additional "No data" message and re-ordering charts #75399

cauemarcondes · 2020-08-19T08:47:43Z

closes #74834
closes #74850

elasticmachine · 2020-08-19T08:47:47Z

Pinging @elastic/apm-ui (Team:apm)

formgeist · 2020-08-19T08:49:54Z

@cauemarcondes Do you think it's possible that we can include this issue in this PR as well? #74850

cauemarcondes · 2020-08-19T08:57:42Z

@cauemarcondes Do you think it's possible that we can include this issue in this PR as well? #74850

Yes, will do it.

formgeist · 2020-08-19T09:00:33Z

@cauemarcondes I'm not seeing the example that you shared. The "Time spent.." chart has no x-axis values and it's not aligned in the middle as the screenshot would indicate.

I'm just picking a time range that I know has no data to return.

cauemarcondes · 2020-08-19T09:03:32Z

I'm just picking a time range that I know has no data to return.

That's weird, can you share the URL? Have you pulled to get the latest changes?

formgeist · 2020-08-19T09:11:32Z

That's weird, can you share the URL? Have you pulled to get the latest changes?

OK, must've messed in my pull - now it looks as described 👍

formgeist

LGTM

cauemarcondes · 2020-08-19T09:21:36Z

Do you think it's possible that we can include this issue in this PR as well? #74850

@formgeist I have just pushed a new version with the charts re-ordered.

formgeist · 2020-08-19T09:50:27Z

@formgeist I have just pushed a new version with the charts re-ordered.

Thanks - I noticed that the charts have been rearranged on the RUM services, which features an additional set of charts (map + page load distribution). I think we'd want to keep the original position of these at the bottom.

Before

Currently in this PR

Proposed layout

cauemarcondes · 2020-08-19T10:46:27Z

I noticed that the charts have been rearranged on the RUM services, which features an additional set of charts (map + page load distribution). I think we'd want to keep the original position of these at the bottom.

good catch.

cauemarcondes · 2020-08-19T11:13:38Z

@formgeist

formgeist · 2020-08-19T11:14:04Z

@cauemarcondes LGTM 👍

...lugins/apm/public/components/shared/TransactionBreakdown/TransactionBreakdownGraph/index.tsx

…ove-msg

cauemarcondes · 2020-08-24T07:54:48Z

@elasticmachine merge upstream

dgieselaar

LGTM, but we should check what happens when there is a large number of KPIs (e.g. >8)

...ck/plugins/apm/public/components/shared/TransactionBreakdown/TransactionBreakdownKpiList.tsx

sorenlouv

lgtm

formgeist

LGTM - I look forward to hearing any feedback from users who might have found the large KPIs useful. I think the consistent look for the charts works much better with this layout.

formgeist · 2020-08-26T08:42:25Z

Once this goes in, I would like to open a new issue to solve some of the responsive layout enhancements to allow for narrower screen sizes to be able to appropriately read the charts. I think we can solve this after the fact, if time permits.

cauemarcondes · 2020-08-26T11:44:17Z

@elasticmachine merge upstream

sorenlouv · 2020-08-26T12:55:14Z

x-pack/plugins/apm/public/components/shared/TransactionBreakdown/index.tsx

+          <TransactionBreakdownGraph
+            timeseries={timeseries.map((serie) => ({
+              ...serie,
+              legendValue: asPercent(serie.legendValue, 1),


Any reason for overwriting series here, and not where it's defined?

kibana/x-pack/plugins/apm/server/lib/transactions/breakdown/index.ts

Lines 211 to 217 in 63265b6

const timeseries = kpis.map((kpi) => ({

title: kpi.name,

color: kpi.color,

type: 'areaStacked',

data: timeseriesPerSubtype[kpi.name],

hideLegend: true,

}));

asPercent is defined in /public and I think the server doesn't have to know how the UI is going to show the value, it should only provide the value. I have my concerns about color, type and hideLegend, maybe they should all be removed from the server and be defined only in the client.

See #70161 in the backlog.

I think the server doesn't have to know how the UI is going to show the value, it should only provide the value.

It depends. Are there any drawbacks to defining it on the server? I see clear drawbacks in defining the chart config across multiple files.
That being said, if it's too much of a hassle because it'll require you to move asPercent and its dependencies to the server I'm fine with leaving it as-is.

@smith Regarding #70161: I'd be surprised if we don't have a way to access the advanced settings from the server.

Thanks for your input @sqren, the only drawback I can see here is that I'd have to move the asPercent to a common place. But if it makes sense the server to send the value already formatted then I wouldn't think it as a drawback but as an improvement. My question is does it make sense sending the value already formatted from the server? For what I've seen we never send any formatted value from the server, we always leave this responsibility to the client.

FYI: I'm not against moving the asPercent, I only want to keep consistency across all APIs we've got.

@smith: why? (Other than it being the convention)

On a technical level, some explanation is in #70161. If a users are using different themes and the API includes the colors of the lines the API would have to return different responses based on the theme.

If the presentational elements are coupled with the chart data, changing the presentation becomes more difficult.

For theming then it makes sense, although I can imagine that can be supported on the server as well. Personally I'm not swayed by "presentational data belongs on the client". IME, server-side code is more predictable and testable, and it's nice to have it in one place.

well, for now then I'll move the asPercent to the common directory and return the value already formatted from the server. We can continue discussing on Nathan's issue #70161.

Sounds good @cauemarcondes 👍

cauemarcondes · 2020-08-27T06:25:32Z

@elasticmachine merge upstream

cauemarcondes · 2020-08-31T06:59:09Z

@elasticmachine merge upstream

…ove-msg

kibanamachine · 2020-09-01T10:38:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c4f5968

Build metrics

async chunks size

id	value	diff	baseline
apm	4.7MB	-5.4KB	4.7MB

distributable file count

id	value	diff	baseline
total	45456	+1	45455

History

💔 Build #71496 failed 04bfdd0
💔 Build #71493 failed d960841
💚 Build #71200 succeeded 799e1fb
💚 Build #71177 succeeded eae5696
💚 Build #70770 succeeded 2caea0c

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…stic#75399) * removing extra message * adding x-axis values when no data is available * reordering charts * fixing internationalization * fixing transaction RUM agent order * addressing PR comment * removing kpis list and show it in the chart legend instead * fixing API test * moving asPercent to the common directory * fixing api test * fixing unit test * removing unused prop * fixing unit test Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

) (#76364) * removing extra message * adding x-axis values when no data is available * reordering charts * fixing internationalization * fixing transaction RUM agent order * addressing PR comment * removing kpis list and show it in the chart legend instead * fixing API test * moving asPercent to the common directory * fixing api test * fixing unit test * removing unused prop * fixing unit test Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

cauemarcondes added 2 commits August 18, 2020 11:55

removing extra message

624b2a3

adding x-axis values when no data is available

37410ce

cauemarcondes requested a review from a team as a code owner August 19, 2020 08:47

botelastic bot added the Team:APM All issues that need APM UI Team support label Aug 19, 2020

cauemarcondes added release_note:enhancement v7.10.0 labels Aug 19, 2020

cauemarcondes requested a review from formgeist August 19, 2020 08:48

formgeist approved these changes Aug 19, 2020

View reviewed changes

reordering charts

6cef87d

cauemarcondes changed the title ~~[APM] Time spent by span type chart: Remove additional "No data" message since the expand/collapse toggle is not used~~ [APM] Remove additional "No data" message and re-ordering charts Aug 19, 2020

fixing internationalization

37b5c47

fixing transaction RUM agent order

c713db9

dgieselaar reviewed Aug 20, 2020

View reviewed changes

cauemarcondes added 2 commits August 20, 2020 12:56

Merge branch 'master' of github.com:elastic/kibana into apm-chart-rem…

1868699

…ove-msg

addressing PR comment

31ac18d

cauemarcondes requested a review from dgieselaar August 20, 2020 11:54

Merge branch 'master' into apm-chart-remove-msg

911e4bd

dgieselaar reviewed Aug 25, 2020

View reviewed changes

...ck/plugins/apm/public/components/shared/TransactionBreakdown/TransactionBreakdownKpiList.tsx Outdated Show resolved Hide resolved

dgieselaar approved these changes Aug 25, 2020

View reviewed changes

sorenlouv approved these changes Aug 25, 2020

View reviewed changes

removing kpis list and show it in the chart legend instead

263fdce

cauemarcondes requested review from formgeist, dgieselaar and sorenlouv August 25, 2020 12:09

fixing API test

2f907f1

formgeist approved these changes Aug 26, 2020

View reviewed changes

Merge branch 'master' into apm-chart-remove-msg

3eb4f5f

sorenlouv reviewed Aug 26, 2020

View reviewed changes

Merge branch 'master' into apm-chart-remove-msg

2caea0c

elasticmachine and others added 7 commits August 31, 2020 00:59

Merge branch 'master' into apm-chart-remove-msg

eae5696

moving asPercent to the common directory

a807f06

fixing api test

799e1fb

Merge branch 'master' of github.com:elastic/kibana into apm-chart-rem…

5ab6936

…ove-msg

fixing unit test

d960841

removing unused prop

04bfdd0

fixing unit test

c4f5968

cauemarcondes merged commit bf7b478 into elastic:master Sep 1, 2020

cauemarcondes deleted the apm-chart-remove-msg branch September 1, 2020 11:38

cauemarcondes mentioned this pull request Sep 1, 2020

[7.x] [APM] Remove additional "No data" message and re-ordering charts (#75399) #76364

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Remove additional "No data" message and re-ordering charts #75399

[APM] Remove additional "No data" message and re-ordering charts #75399

cauemarcondes commented Aug 19, 2020 •

edited

Loading

elasticmachine commented Aug 19, 2020

formgeist commented Aug 19, 2020

cauemarcondes commented Aug 19, 2020

formgeist commented Aug 19, 2020

cauemarcondes commented Aug 19, 2020

formgeist commented Aug 19, 2020

formgeist left a comment

cauemarcondes commented Aug 19, 2020

formgeist commented Aug 19, 2020

cauemarcondes commented Aug 19, 2020

cauemarcondes commented Aug 19, 2020

formgeist commented Aug 19, 2020

cauemarcondes commented Aug 24, 2020

dgieselaar left a comment

sorenlouv left a comment

formgeist left a comment

formgeist commented Aug 26, 2020

cauemarcondes commented Aug 26, 2020

sorenlouv Aug 26, 2020

cauemarcondes Aug 26, 2020

smith Aug 26, 2020

sorenlouv Aug 26, 2020 •

edited

Loading

cauemarcondes Aug 27, 2020

dgieselaar Aug 27, 2020

smith Aug 27, 2020

dgieselaar Aug 31, 2020

cauemarcondes Aug 31, 2020

sorenlouv Aug 31, 2020

cauemarcondes commented Aug 27, 2020

cauemarcondes commented Aug 31, 2020

kibanamachine commented Sep 1, 2020

	const timeseries = kpis.map((kpi) => ({
	title: kpi.name,
	color: kpi.color,
	type: 'areaStacked',
	data: timeseriesPerSubtype[kpi.name],
	hideLegend: true,
	}));

[APM] Remove additional "No data" message and re-ordering charts #75399

[APM] Remove additional "No data" message and re-ordering charts #75399

Conversation

cauemarcondes commented Aug 19, 2020 • edited Loading

elasticmachine commented Aug 19, 2020

formgeist commented Aug 19, 2020

cauemarcondes commented Aug 19, 2020

formgeist commented Aug 19, 2020

cauemarcondes commented Aug 19, 2020

formgeist commented Aug 19, 2020

formgeist left a comment

Choose a reason for hiding this comment

cauemarcondes commented Aug 19, 2020

formgeist commented Aug 19, 2020

cauemarcondes commented Aug 19, 2020

cauemarcondes commented Aug 19, 2020

formgeist commented Aug 19, 2020

cauemarcondes commented Aug 24, 2020

dgieselaar left a comment

Choose a reason for hiding this comment

sorenlouv left a comment

Choose a reason for hiding this comment

formgeist left a comment

Choose a reason for hiding this comment

formgeist commented Aug 26, 2020

cauemarcondes commented Aug 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cauemarcondes commented Aug 27, 2020

cauemarcondes commented Aug 31, 2020

kibanamachine commented Sep 1, 2020

💚 Build Succeeded

Build metrics

async chunks size

distributable file count

History

cauemarcondes commented Aug 19, 2020 •

edited

Loading

sorenlouv Aug 26, 2020 •

edited

Loading