-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
[APM] Adding comparison latency chart #91339
[APM] Adding comparison latency chart #91339
Conversation
Pinging @elastic/apm-ui (Team:apm) |
x-pack/plugins/apm/public/components/shared/charts/latency_chart/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/public/components/shared/charts/latency_chart/index.tsx
Outdated
Show resolved
Hide resolved
customTheme={comparisonChartTheme} | ||
timeseries={[ | ||
...currentPeriod, | ||
...(comparisonEnabled && previousPeriod ? [previousPeriod] : []), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we using this flag in other places when deciding to pass the data to the component? not saying we shouldn't, but it should be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dgieselaar yes, the comparisonEnabled
will be used on all charts to pass or not the previousPeriod data to the chart component.
@elasticmachine merge upstream |
…arison-latency-chart
…rcondes/kibana into apm-time-comparison-latency-chart
@elasticmachine merge upstream |
x-pack/plugins/apm/public/hooks/use_transaction_latency_chart_fetcher.ts
Outdated
Show resolved
Hide resolved
@ogupte I asked the charts team. New bug opened: elastic/elastic-charts#1063 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments, but it looks good!
jenkins, retest this please |
x-pack/plugins/apm/public/components/shared/charts/latency_chart/index.tsx
Outdated
Show resolved
Hide resolved
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
* adding time comparison to latency chart * adding time comparison to latency chart * fixing TS * fixing api test * addressing PR comments * adding api test * addressing PR comments * fixing api test * rounding date diff * addressing PR comments * fixing api test * refactoring * fixing ts issue * fixing offset function * fixing offset function * addressing PR comments Co-authored-by: Kibana Machine <[email protected]>
* adding time comparison to latency chart * adding time comparison to latency chart * fixing TS * fixing api test * addressing PR comments * adding api test * addressing PR comments * fixing api test * rounding date diff * addressing PR comments * fixing api test * refactoring * fixing ts issue * fixing offset function * fixing offset function * addressing PR comments Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
closes #90571
comparison.latency.chart.mov