Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[APM] Adding comparison to transactions table #91435

Merged
merged 19 commits into from
Mar 9, 2021
Merged

[APM] Adding comparison to transactions table #91435

merged 19 commits into from
Mar 9, 2021

Conversation

cauemarcondes
Copy link
Contributor

closes #90572

Screenshot 2021-02-15 at 13 31 04

comparion-transactions-table.mov

@cauemarcondes cauemarcondes added release_note:skip Skip the PR/issue when compiling release notes v7.12.0 apm:comparison labels Feb 15, 2021
@cauemarcondes cauemarcondes requested a review from a team as a code owner February 15, 2021 18:37
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Feb 15, 2021
@elasticmachine
Copy link
Contributor

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

@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

dgieselaar
dgieselaar reviewed Feb 23, 2021
View reviewed changes
x-pack/plugins/apm/server/lib/services/get_service_transaction_group_comparison_statistics.ts Outdated
Comment on lines 218 to 222
const currentPeriodPromise = getServiceTransactionGroupComparisonStatistics({
...commomProps,
start,
end,
});
Copy link
Member

Choose a reason for hiding this comment

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

any reason why this needs an intermediate variable, rather than inlining it in await Promise.all()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just think it makes it easier to read with two separated variables.

@cauemarcondes cauemarcondes force-pushed the apm-time-comparison-transactions-table branch from 2f23bc8 to 9aeb9ef Compare February 24, 2021 13:35
smith
smith requested changes Feb 25, 2021
View reviewed changes
Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

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

The comments here are only for a few misspellings in variable names, not blocking.

But what is blocking is that if I check/uncheck comparisons it doesn't update the impact bars. Also it always shows two bars, even if comparisons are off. Do we want it to switch back to the single bar? I think so.

...m/public/components/app/service_overview/service_overview_transactions_table/get_columns.tsx Outdated Show resolved Hide resolved
...m/public/components/app/service_overview/service_overview_transactions_table/get_columns.tsx Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/components/shared/charts/spark_plot/index.tsx Outdated Show resolved Hide resolved
x-pack/plugins/apm/server/lib/services/get_service_transaction_group_comparison_statistics.ts Outdated Show resolved Hide resolved
x-pack/test/apm_api_integration/tests/transactions/transactions_groups_comparison_statistics.ts Outdated Show resolved Hide resolved
@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

dgieselaar
dgieselaar reviewed Mar 1, 2021
View reviewed changes
...ins/apm/public/components/app/service_overview/service_overview_transactions_table/index.tsx Outdated Show resolved Hide resolved
...ins/apm/public/components/app/service_overview/service_overview_transactions_table/index.tsx Outdated
Comment on lines 115 to 118
comparisonType,
pageIndex,
direction,
field,
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be worth trying to figure out if we can move calculating the visible transaction groups into the useFetcher call as well, to see if we no longer need to disable react-hooks/exhaustive-deps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dgieselaar I can calculate the visible transaction groups inside useFetcher, but I'll still need the react-hooks/exhaustive-deps, because comparisonType needs to be listed on its dependencies, even though it is not used, to trigger the comparison API call when it's changed.

x-pack/plugins/apm/public/components/shared/charts/spark_plot/index.tsx Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/components/shared/charts/spark_plot/index.tsx Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/components/shared/charts/spark_plot/index.tsx Outdated
@@ -85,8 +98,8 @@ export function SparkPlot({
showLegend={false}
tooltip="none"
/>
<AreaSeries
id="area"
<SparkelineSeries
Copy link
Member

Choose a reason for hiding this comment

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

This can't be a LineSeries all the time (or an area series)? What changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is necessary because when comparison is enabled/visible we should change the current period sparkline to LineSeries and show the previous period with AreaSeries.

There's no technical problem with changing all sparklines to LineSeries, but all tables that yet don't have comparison implemented would change to a LineSeries, if @formgeist agrees I'm fine too.

Copy link
Contributor

Choose a reason for hiding this comment

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

The thoughts behind the mixed chart types was primarily to create difference in the sparklines between the current and previous time periods. I think if we keep to either line or area, the comparison vanishes a bit because of the size of the chart. @cauemarcondes is it easy to exemplify the two charts (line or area)?

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, I am fine with mixing line and area. I was just wondering why it's necessary for the current period to switch, and why it can't always be either a line or an area series.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why it's necessary for the current period to switch, and why it can't always be either a line or an area series.

Because there are some places where the spakline is used and it doesn't have comparison data, like on the Service Inventory page table. If I always show it as a LineSeries it would change this table too, that's why I asked @formgeist .

x-pack/plugins/apm/server/utils/offset_previous_period_coordinate.ts Outdated Show resolved Hide resolved
x-pack/test/apm_api_integration/tests/transactions/transactions_groups_comparison_statistics.ts Outdated Show resolved Hide resolved
x-pack/test/apm_api_integration/tests/transactions/transactions_groups_comparison_statistics.ts Outdated Show resolved Hide resolved
@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

@dgieselaar
Copy link
Member

dgieselaar commented Mar 8, 2021 via email

@cauemarcondes cauemarcondes requested a review from smith March 8, 2021 18:02
ogupte
ogupte approved these changes Mar 8, 2021
View reviewed changes
Copy link
Contributor

@ogupte ogupte left a comment

Choose a reason for hiding this comment

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

Great work! excited to see this merging 🚢

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 5.3MB 5.3MB +4.5KB

History

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

@cauemarcondes cauemarcondes merged commit c2fb2ec into elastic:master Mar 9, 2021
@cauemarcondes cauemarcondes deleted the apm-time-comparison-transactions-table branch March 9, 2021 03:00
@cauemarcondes cauemarcondes mentioned this pull request Mar 9, 2021
Merged
cauemarcondes added a commit to cauemarcondes/kibana that referenced this pull request Mar 9, 2021
@cauemarcondes @kibanamachine
[APM] Adding comparison to transactions table (elastic#91435)
7132d0b 
* adding comparison to transactions table

* fixing api test

* merge

* addressing PR comments

* addressing PR comments

* adding kuery filter

* fixing tests

* addressing PR comments

* fixing concurrency issues

* addressing PR comments

* addressing PR comments

* addressing PR comments

* hiding impact bar when comparison disable

Co-authored-by: Kibana Machine <[email protected]>
cauemarcondes added a commit that referenced this pull request Mar 9, 2021
@cauemarcondes @kibanamachine
[APM] Adding comparison to transactions table (#91435) (#94133)
d4aad1d 
* adding comparison to transactions table

* fixing api test

* merge

* addressing PR comments

* addressing PR comments

* adding kuery filter

* fixing tests

* addressing PR comments

* fixing concurrency issues

* addressing PR comments

* addressing PR comments

* addressing PR comments

* hiding impact bar when comparison disable

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
@cauemarcondes cauemarcondes mentioned this pull request Apr 21, 2021
Closed
@ogupte ogupte self-assigned this May 10, 2021
@ogupte ogupte added the apm:test-plan-done Pull request that was successfully tested during the test plan label May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@formgeist formgeist formgeist left review comments

@dgieselaar dgieselaar dgieselaar left review comments

@ogupte ogupte ogupte approved these changes

@smith smith smith approved these changes

Assignees

@ogupte ogupte

Labels
apm:comparison apm:test-plan-7.13.0 apm:test-plan-done Pull request that was successfully tested during the test plan release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v7.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] Adding comparison to transactions table
7 participants
@cauemarcondes @elasticmachine @dgieselaar @kibanamachine @smith @ogupte @formgeist