[APM] Service overview: Transactions table #83429
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Pinging @elastic/apm-ui (Team:apm) |
sorenlouv
reviewed
Nov 16, 2020
...ins/apm/public/components/app/service_overview/service_overview_transactions_table/index.tsx
Outdated
Show resolved
Hide resolved
sorenlouv
reviewed
Nov 16, 2020
...ins/apm/public/components/app/service_overview/service_overview_transactions_table/index.tsx
Show resolved
Hide resolved
sorenlouv
reviewed
Nov 16, 2020
Comment on lines
180
to
184
| valueLabel={ | ||
| latency.value !== null | ||
| ? asDuration(latency.value) | ||
| : NOT_AVAILABLE_LABEL | ||
| } |
There was a problem hiding this comment.
asDuration already defaults to NOT_AVAILABLE_LABEL when there's no value
Suggested change
| valueLabel={ | |
| latency.value !== null | |
| ? asDuration(latency.value) | |
| : NOT_AVAILABLE_LABEL | |
| } | |
| valueLabel={asDuration(latency.value)} |
sorenlouv
reviewed
Nov 16, 2020
...ins/apm/public/components/app/service_overview/service_overview_transactions_table/index.tsx
Outdated
Show resolved
Hide resolved
sorenlouv
reviewed
Nov 16, 2020
| ? asTransactionRate(traffic.value) | ||
| : NOT_AVAILABLE_LABEL | ||
| } | ||
| start={parseFloat(start!)} |
There was a problem hiding this comment.
I don't understand this. Isn't start a string like 2020-11-09T19:48:43.778Z? If that's the case parseFloat returns 2020 - is that the intention?
There was a problem hiding this comment.
Btw looking closer at this:
- We don't need to pass
startorendtoSparkPlotWithValueLabelsince we don't show any ticks the date range for the empty state doesn't matter - Furthermore: the empty state created in
SparkPlotWithValueLabelseems to be made doubly redundant inSparkPlotby theisEmptycheck
There was a problem hiding this comment.
No idea how I got here 🤦♂️ Will fix.
sorenlouv
reviewed
Nov 16, 2020
x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/index.ts
Outdated
Show resolved
Hide resolved
sorenlouv
reviewed
Nov 16, 2020
x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/index.ts
Outdated
Show resolved
Hide resolved
sorenlouv
reviewed
Nov 16, 2020
x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/index.ts
Outdated
Show resolved
Hide resolved
sorenlouv
reviewed
Nov 16, 2020
x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/index.ts
Outdated
Show resolved
Hide resolved
sorenlouv
reviewed
Nov 16, 2020
x-pack/test/apm_api_integration/basic/tests/service_overview/transaction_groups.ts
Outdated
Show resolved
Hide resolved
smith
requested changes
Nov 17, 2020
There was a problem hiding this comment.
Looks good. Søren's comments make sense too.
I just merged #83360, so could you please set this up to use ServiceOverviewTable instead of of EuiBasicTable? It handles some CSS for the loading state and pinning the pagination to the bottom.
Could we make the impact column a touch wider? It's truncated when we sort by it, which is the default:
…iew-transactions-table
…iew-transactions-table
smith
approved these changes
Nov 18, 2020
…iew-transactions-table
sorenlouv
reviewed
Nov 20, 2020
Comment on lines
189
to
190
| start={new Date(start!).getTime()} | ||
| end={new Date(end!).getTime()} |
There was a problem hiding this comment.
Still not following this. Why are we passing start and end? We are not using it for the empty state anyway (it's a plot without ticks)
There was a problem hiding this comment.
Ah, I get what you mean now, will fix.
sorenlouv
reviewed
Nov 20, 2020
Comment on lines
211
to
212
| start={new Date(start!).getTime()} | ||
| end={new Date(end!).getTime()} |
sorenlouv
reviewed
Nov 20, 2020
Comment on lines
233
to
234
| start={new Date(start!).getTime()} | ||
| end={new Date(end!).getTime()} |
sorenlouv
reviewed
Nov 20, 2020
| describe('Service overview transaction groups', () => { | ||
| describe('when data is not loaded', () => { | ||
| it('handles the empty state', async () => { | ||
| const response = await supertest.get( |
There was a problem hiding this comment.
nit: This is still typed as any but not terribly important.
There was a problem hiding this comment.
Not for now but a thought I just got:
WDYT about a client that wraps supertest and automatically gets the type based on the url and method - similar to what we use in APM?
const res = apmSupertest({
endpoint: 'GET /api/apm/services/opbeans-java/overview_transaction_groups',
query: {
start,
end,
uiFilters: '{}',
size: 5,
numBuckets: 20,
pageIndex: 0,
sortDirection: 'desc',
sortField: 'impact',
},
// ...
})I think that would provide an awesome DX - especially when editing a test to accommodate an API change
There was a problem hiding this comment.
I like it 👍 I think there's a small chance of us being less inclined to test things that don't align with the types, but generally I would say the benefits outweigh that risk.
sorenlouv
approved these changes
Nov 20, 2020
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
Nov 20, 2020
* master: (38 commits) [ML] Data frame analytics: Adds functionality to map view (elastic#83710) Add usage collection for savedObject tagging (elastic#83160) [SECURITY_SOLUTION] 145: Advanced Policy Tests (elastic#82898) [APM] Service overview transactions table (elastic#83429) [ML] Fix Single Metric Viewer not loading if job is metric with no partition (elastic#83880) do not export types from 3rd party modules as 'type' (elastic#83803) [Fleet] Allow to send SETTINGS action (elastic#83707) Fixes Failing test: Chrome X-Pack UI Functional Tests.x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/details·ts - Actions and Triggers app Alert Details Alert Instances renders the active alert instances (elastic#83478) [Uptime]Reduce chart height on monitor detail page (elastic#83777) [APM] Prefer `APIReturnType` over `PromiseReturnType` (elastic#83843) [Observability] Fix telemetry for Observability Overview (elastic#83847) [Alerting] Adds generic UI for the definition of conditions for Action Groups (elastic#83278) ensure workload agg doesnt run until next interval when it fails (elastic#83632) [ILM] Policy form should not throw away data (elastic#83077) [Monitoring] Stop collecting Kibana Usage in bulkUploader (elastic#83546) [TSVB] fix wrong imports (elastic#83798) [APM] Correlations UI POC (elastic#82256) list all the refs in tsconfig.json (elastic#83678) Bump jest (and related packages) to v26.6.3 (elastic#83724) Functional tests - stabilize reporting tests for cloud execution (elastic#83787) ...
Closed
mshustov
pushed a commit
that referenced
this pull request
Nov 23, 2020
Co-authored-by: Dario Gieselaar <[email protected]>
sorenlouv
changed the title
Closed
💔 Build Failed
Failed CI StepsTest FailuresX-Pack API Integration Tests.x-pack/test/api_integration/apis/ml/results/get_anomalies_table_data·ts.apis Machine Learning ResultsService GetAnomaliesTableData should fetch anomalies table dataStandard OutStack TraceX-Pack API Integration Tests.x-pack/test/api_integration/apis/ml/results/get_anomalies_table_data·ts.apis Machine Learning ResultsService GetAnomaliesTableData should fetch anomalies table dataStandard OutStack TraceMetrics [docs]Module Count
Async chunks
Distributable file count
History
To update your PR or re-run it, just comment with: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #81726.
Mostly the same work as error groups, so lots of duplication. The instance table in #81721 will need similar functionality. I think that would be the right time to figure out what the right abstraction would be (if we decide we need one).