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] Transition to Elastic charts for all relevant APM charts #80298

Merged
merged 19 commits into from
Nov 9, 2020
Merged

[APM] Transition to Elastic charts for all relevant APM charts #80298

merged 19 commits into from
Nov 9, 2020

Conversation

cauemarcondes
Copy link
Contributor

@cauemarcondes cauemarcondes commented Oct 13, 2020 •
edited
Loading

Solves part of #70290

Done:

  • Charts on Transaction Overview page
  • Charts on Transaction Detail page
  • Error distribution chart

To do (Another PR):

  • Charts on Metrics page
  • Improve axis format

Issues:

  • We're not able to define a fixed value for the legends. #561
  • When hovering over the chart, the legend values change. #561
  • It's not possible to add an annotation legend. #555
  • There is no highlight style out-of-the-box. I did it by adding an Annotation around the bucket. #845
  • On a bar chart, it's not possible to click on the whole container. #846

Transactions page:
Screenshot 2020-11-03 at 11 52 29

Transaction detail page:
Screenshot 2020-11-03 at 11 52 40

Error page:
Screenshot 2020-11-03 at 11 52 55

Error detail page:
Screenshot 2020-11-03 at 11 53 06

@cauemarcondes cauemarcondes mentioned this pull request Oct 19, 2020
Closed
@smith
Copy link
Contributor

smith commented Oct 28, 2020

@elasticmachine merge upstream

smith added a commit to smith/kibana that referenced this pull request Oct 29, 2020
@smith
Add error rate chart to overview
4b36f3e 
Take most of the work directly from elastic#80298 to add the error rate chart to the overview.

Rename the existing chart that's on the transactions overview so it still keeps using the old chart for the time being. We don't want to mix chart types (react-vis + elastic-charts) on the same page becuase the interactions are different. We'll switch the transactions page to use elastic charts in a future PR.

Hide the error rate chart on RUM services.
@smith smith mentioned this pull request Oct 29, 2020
Merged
@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

smith added a commit that referenced this pull request Nov 2, 2020
@smith
Add error rate chart to overview (#82082)
2f504a0 
* Add error rate chart to overview

Take most of the work directly from #80298 to add the error rate chart to the overview.

Rename the existing chart that's on the transactions overview so it still keeps using the old chart for the time being. We don't want to mix chart types (react-vis + elastic-charts) on the same page becuase the interactions are different. We'll switch the transactions page to use elastic charts in a future PR.

Hide the error rate chart on RUM services.
smith added a commit to smith/kibana that referenced this pull request Nov 2, 2020
@smith
Add error rate chart to overview (elastic#82082)
2f8edb5 
* Add error rate chart to overview

Take most of the work directly from elastic#80298 to add the error rate chart to the overview.

Rename the existing chart that's on the transactions overview so it still keeps using the old chart for the time being. We don't want to mix chart types (react-vis + elastic-charts) on the same page becuase the interactions are different. We'll switch the transactions page to use elastic charts in a future PR.

Hide the error rate chart on RUM services.
smith added a commit that referenced this pull request Nov 2, 2020
@smith
Add error rate chart to overview (#82082) (#82322)
51dc791 
Take most of the work directly from #80298 to add the error rate chart to the overview.

Rename the existing chart that's on the transactions overview so it still keeps using the old chart for the time being. We don't want to mix chart types (react-vis + elastic-charts) on the same page becuase the interactions are different. We'll switch the transactions page to use elastic charts in a future PR.

Hide the error rate chart on RUM services.
@cauemarcondes cauemarcondes added enhancement New value added to drive a business result v7.11.0 release_note:enhancement and removed enhancement New value added to drive a business result labels Nov 3, 2020
@cauemarcondes cauemarcondes marked this pull request as ready for review November 3, 2020 14:55
@cauemarcondes cauemarcondes requested a review from a team as a code owner November 3, 2020 14:55
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Nov 3, 2020
@elasticmachine
Copy link
Contributor

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

@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

@cauemarcondes
Copy link
Contributor Author

retest

smith
smith approved these changes Nov 5, 2020
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.

Had a couple nits and couldn't run it locally due to unrelated issues but code looks good.

x-pack/plugins/apm/public/components/shared/charts/transaction_error_rate_chart/index.tsx Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/selectors/chartSelectors.ts
apmTimeseries,
anomalyTimeseries,
});
const { apmTimeseries, anomalyTimeseries } = charts;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to be apm or tpm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, maybe timeseries would be enough. WDYT?

sorenlouv
sorenlouv reviewed Nov 5, 2020
View reviewed changes
x-pack/plugins/apm/public/selectors/chartSelectors.ts
@@ -31,40 +31,37 @@ export interface ITpmBucket {
}

export interface ITransactionChartData {
tpmSeries: ITpmBucket[];
Copy link
Member

Choose a reason for hiding this comment

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

I'd love to see if we can get rid of this file. Not sure if it's possible but it feels like there's a lot of extra data massaging going on that we don't need.

sorenlouv
sorenlouv reviewed Nov 5, 2020
View reviewed changes
x-pack/plugins/apm/public/hooks/useTransactionBreakdown.ts
@@ -15,7 +15,7 @@ export function useTransactionBreakdown() {
uiFilters,
} = useUrlParams();

const { data = { timeseries: [] }, error, status } = useFetcher(
const { data = { timeseries: undefined }, error, status } = useFetcher(
Copy link
Member

Choose a reason for hiding this comment

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

This looks redundant - or am I missing something?

Suggested change
const { data = { timeseries: undefined }, error, status } = useFetcher(
const { data = { timeseries }, error, status } = useFetcher(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

timeseries is not defined above for me to initialize the data with it, that's why I added the undefined. And it is initialized as undefined because I just show the Loading chart component, if the status is equal to Loading or Pending and the timeseries is undefined. Otherwise, I'd show the Loading every time the user pushes the Refresh button in the DatePicker or when the auto-refresh is on.

Copy link
Member

@sorenlouv sorenlouv Nov 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

timeseries is not defined above for me to initialize the data with it, that's why I added the undefined.

I'm probably still missing your point but do we agree that timeseries will have undefined as value by default?

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 believe the example you gave is a bit different than what we have if I apply the change you suggested:
Screenshot 2020-11-06 at 11 53 38

That's because I'm trying to initialize a value that has been restructured with timeseries which does not exists.

sorenlouv
sorenlouv approved these changes Nov 6, 2020
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

lgtm

@cauemarcondes
Copy link
Contributor Author

@sqren I'm going to merge this PR and handle some comments on another PR with the remaining charts.

@smith
Copy link
Contributor

smith commented Nov 9, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
apm 1277 1275 -2

async chunks size

id before after diff
apm 3.2MB 3.2MB -7.1KB

History

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

sorenlouv
sorenlouv reviewed Nov 9, 2020
View reviewed changes
x-pack/plugins/apm/public/components/app/TransactionDetails/Distribution/index.tsx
}

if (
(!distribution || distribution.noHits) &&
Copy link
Member

Choose a reason for hiding this comment

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

Is noHits still used?

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 will fix that while doing this issue. #82953

@cauemarcondes cauemarcondes merged commit 0217073 into elastic:master Nov 9, 2020
@cauemarcondes cauemarcondes deleted the apm-elastic-charts branch November 9, 2020 14:03
@cauemarcondes cauemarcondes mentioned this pull request Nov 9, 2020
Merged
cauemarcondes added a commit to cauemarcondes/kibana that referenced this pull request Nov 9, 2020
@cauemarcondes @kibanamachine
[APM] Transition to Elastic charts for all relevant APM charts (elast…
6710731 
…ic#80298)

* adding elastic charts

* fixing some stuff

* refactoring

* fixing ts issues

* fixing unit test

* fix i18n

* adding isLoading prop

* adding annotations toggle, replacing transaction error rate to elastic chart

* adding loading state

* adding empty message

* fixing i18n

* removing unused files

* fixing i18n

* removing e2e test since elastic charts uses canvas

* addressing pr comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
cauemarcondes added a commit that referenced this pull request Nov 9, 2020
@cauemarcondes @kibanamachine
[APM] Transition to Elastic charts for all relevant APM charts (#80298)…
afd2385 
… (#82955)

* adding elastic charts

* fixing some stuff

* refactoring

* fixing ts issues

* fixing unit test

* fix i18n

* adding isLoading prop

* adding annotations toggle, replacing transaction error rate to elastic chart

* adding loading state

* adding empty message

* fixing i18n

* removing unused files

* fixing i18n

* removing e2e test since elastic charts uses canvas

* addressing pr comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@formgeist formgeist mentioned this pull request Nov 24, 2020
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorenlouv sorenlouv sorenlouv approved these changes

@smith smith smith approved these changes

Assignees
No one assigned
Labels
release_note:enhancement Team:APM All issues that need APM UI Team support v7.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cauemarcondes @smith @elasticmachine @kibanamachine @sorenlouv