Skip to content

[APM] Performance comparison charts by user agent (browser) - #49582

Merged
smith merged 11 commits into
elastic:masterfrom
smith:nls/43342/browser-breakdown
Nov 20, 2019
Merged

smith merged 11 commits into
elastic:masterfrom
smith:nls/43342/browser-breakdown

Conversation

@smith

@smith smith commented Oct 28, 2019 •

Copy link
Copy Markdown
Contributor

Show a chart with average page load broken down by user agent name on the RUM overview.

I tested this by setting up a RUM agent on a sample app on my test cloud cluster.

On the internal APM dev cluster you'll need to set the time range to about 90 days then look at the "client" app and find the time range where there are requests. Unfortunately with this sample data the only series you see is "Other" because there aren't a lot of requests.

Also factor out the color selection into a common helper.

Fixes #43342

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@smith
smith force-pushed the nls/43342/browser-breakdown branch from 1f07d81 to 71be5ce Compare October 29, 2019 17:40
@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@smith
smith force-pushed the nls/43342/browser-breakdown branch from 71be5ce to 44d676a Compare November 1, 2019 02:57
@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@smith
smith force-pushed the nls/43342/browser-breakdown branch 2 times, most recently from 2f1410f to 2a1ae9e Compare November 1, 2019 21:47
@smith
smith marked this pull request as ready for review November 1, 2019 21:50
@smith
smith requested a review from a team November 1, 2019 21:50
@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@smith

smith commented Nov 11, 2019

Copy link
Copy Markdown
Contributor Author

@elasticmachine retest

@smith

smith commented Nov 11, 2019

Copy link
Copy Markdown
Contributor Author

@elasticmachine test this please

@elasticmachine

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@formgeist

Copy link
Copy Markdown
Contributor

In order to test multiple values or lines, we would need to fake a dataset that has some other values than Other. How would we best go about this?

@smith

smith commented Nov 12, 2019

Copy link
Copy Markdown
Contributor Author

I created a Codesandbox app with RUM agent installed: https://codesandbox.io/s/admiring-mclaren-6o624 then pointed it at my cloud cluster, deployed it with netlify, then pointed my local kibana at that cluster, then loaded the page in a bunch of different browsers.

It would be nicer to have a more realistic data set, but I think we should spend the time on that once we work on adding sample data.

@smith

smith commented Nov 13, 2019

Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

1 similar comment
@smith

smith commented Nov 13, 2019

Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@smith
smith force-pushed the nls/43342/browser-breakdown branch from 6020cfd to d6d7539 Compare November 14, 2019 04:35
@elasticmachine

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is... not great. And it's totally on me (I introduced it!).
LMK if you have any ideas to improve it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it would be useful if you added a user agent in the sample doc:

@smith smith Nov 15, 2019 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added user agent in c39defa8e1.

@sorenlouv sorenlouv Nov 14, 2019 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing rebase, or should this go into ?

export const TRANSACTION_PAGE_LOAD = 'page-load';
export const TRANSACTION_ROUTE_CHANGE = 'route-change';
export const TRANSACTION_REQUEST = 'request';

@smith smith Nov 15, 2019 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in rebase.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi @cauemarcondes: this will probably conflict with your changes in #49638

@smith smith Nov 15, 2019 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in rebase.

Comment thread x-pack/legacy/plugins/apm/public/hooks/useAvgDurationByBrowser.test.ts Outdated

@sorenlouv sorenlouv Nov 14, 2019 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These colors are defined several times already. Perhaps it would make sense to add them as a constant in https://github.com/elastic/kibana/tree/c40b32010462aa6731d5c8a9312444228b3a819e/x-pack/legacy/plugins/apm/common

Examples:
https://github.com/elastic/kibana/blob/c40b32010462aa6731d5c8a9312444228b3a819e/x-pack/legacy/plugins/apm/server/lib/transactions/breakdown/constants.ts#L10-L22

const colors = [
theme.euiColorVis0,
theme.euiColorVis1,
theme.euiColorVis2,
theme.euiColorVis3,
theme.euiColorVis4,
theme.euiColorVis5,
theme.euiColorVis6
];

const colors = [
theme.euiColorVis0,
theme.euiColorVis1,
theme.euiColorVis2,
theme.euiColorVis3,
theme.euiColorVis4,
theme.euiColorVis5,
theme.euiColorVis6
];

Btw. I like the modulus approach (colors[index % colors.length]) you took that ensures we'll loop through them regardless how many lines there are. In contrast to

So maybe a shared helper together with the colors would make sense?

const getVizColorForIndex = index => vizColors[index % vizColors.length]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this in e865ebe1b5.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smith if you rebase master, you'll have to replace getTimeFormatter to getDurationFormatter

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in rebase.

@smith
smith force-pushed the nls/43342/browser-breakdown branch from d6d7539 to e865ebe Compare November 15, 2019 17:39
@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@smith
smith force-pushed the nls/43342/browser-breakdown branch from 14f721b to a137078 Compare November 15, 2019 22:14
@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@sorenlouv sorenlouv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! 👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A file with request type constants was recently added:

export const TRANSACTION_PAGE_LOAD = 'page-load';
export const TRANSACTION_ROUTE_CHANGE = 'route-change';
export const TRANSACTION_REQUEST = 'request';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idx is no more in APM UI #50849 🍾

@smith
smith force-pushed the nls/43342/browser-breakdown branch from a137078 to 0b3ca10 Compare November 19, 2019 05:29
@elasticmachine

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

smith added 11 commits November 20, 2019 00:35
Show a chart with average page load broken down by user agent name on the RUM overview.

I tested this by setting up a RUM agent on a sample app on my test cloud cluster.

On the internal APM dev cluster you'll need to set the time range to about 90 days then look at the "client" app and find the time range where there are requests. Unfortunately with this sample data the only series you see is "Other" because there aren't a lot of requests.

Fixes elastic#43342
@smith
smith force-pushed the nls/43342/browser-breakdown branch from 0b3ca10 to 508454a Compare November 20, 2019 06:38
@elasticmachine

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@smith
smith merged commit 35b0362 into elastic:master Nov 20, 2019
@smith
smith deleted the nls/43342/browser-breakdown branch November 20, 2019 16:30
smith added a commit to smith/kibana that referenced this pull request Nov 20, 2019
…49582)

Show a chart with average page load broken down by user agent name on the RUM overview.

I tested this by setting up a RUM agent on a sample app on my test cloud cluster.

On the internal APM dev cluster you'll need to set the time range to about 90 days then look at the "client" app and find the time range where there are requests. Unfortunately with this sample data the only series you see is "Other" because there aren't a lot of requests.

Also factor out the color selection into a common helper.

Fixes elastic#43342
@cauemarcondes cauemarcondes self-assigned this Jan 17, 2020
@cauemarcondes

Copy link
Copy Markdown
Contributor

Tests:
IE: ✅
Chrome: ✅
Firefox: ✅
Safari: ✅
Screenshot 2020-01-17 at 10 11 54

@cauemarcondes cauemarcondes added the apm:test-plan-done Pull request that was successfully tested during the test plan label Jan 17, 2020
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
…49582)

Show a chart with average page load broken down by user agent name on the RUM overview.

I tested this by setting up a RUM agent on a sample app on my test cloud cluster.

On the internal APM dev cluster you'll need to set the time range to about 90 days then look at the "client" app and find the time range where there are requests. Unfortunately with this sample data the only series you see is "Other" because there aren't a lot of requests.

Also factor out the color selection into a common helper.

Fixes elastic#43342
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

apm:test-plan-done Pull request that was successfully tested during the test plan release_note:enhancement v7.6.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[APM] Performance comparison charts by user agent (browser)

6 participants