[Observability] Remove outdated top_alerts route and related types #107579

weltenwort · 2021-08-03T18:57:03Z

📝 Summary

This removes the AlertsTable, which has since been replaced with a "timeline grid"-based implementation. Along with that it removes the observability-specific "top alerts" HTTP API, which has been replaced with the "timeline" search strategy used internally by the timeline grid.

closes #104927

🕵️ Review notes

APM code owners: This removes parts of tests located in APM that queried the removed API. It also removes the type import of the implicit route handler return value, because the timeline search strategy doesn't provide that amount of type safety. To mitigate that the parseAlert function decodes the alert documents (similar to decorateResponse before).
The story for the alerts table apparently became mostly a no-op story when the t-grid was introduced. This PR doesn't fix it, but removes references to the removed API and types so it renders without errors.

weltenwort · 2021-08-05T15:30:33Z

x-pack/plugins/observability/public/pages/alerts/alerts.stories.tsx

- if (endpoint === '/api/observability/rules/alerts/top') {
- return items;
- } else if (endpoint === '/api/observability/rules/alerts/dynamic_index_pattern') {
+ if (endpoint === '/api/observability/rules/alerts/dynamic_index_pattern') {


I know this doesn't fix the story but it also doesn't break it further. But IMHO it would exceed the scope of the PR to implement a working t-grid mock.

If the story isn't doing doing anything we should just delete it.

weltenwort · 2021-08-05T15:34:39Z

x-pack/plugins/observability/public/pages/alerts/alerts_flyout/index.tsx

 import { SeverityBadge } from '../severity_badge';

 type AlertsFlyoutProps = {
 alert?: TopAlert;
- alerts?: TopAlertResponse[];
+ alerts?: Array<Record<string, unknown>>;


TopAlertResponse was only every indirectly defined via the return type of the server-side route registration. The removal of that route meant the type went away as well. The data fetching now happens via the timeline search strategy, which doesn't provide much type safety around the data, which is reflected by this Record<string, unknown>. The object is decoded using parseAlert anyway (which is the successor to decorateResponse).

Even if we don't have much type safety now, would it make sense to extract Record<string, unknown> to a separate type? That way if/when we get better typings it's a simpler change

weltenwort · 2021-08-05T15:36:44Z

x-pack/plugins/observability/public/pages/alerts/parse_alert.ts

+import { ObservabilityRuleTypeRegistry } from '../../rules/create_observability_rule_type_registry';
+
+export const parseAlert = (observabilityRuleTypeRegistry: ObservabilityRuleTypeRegistry) => (
+ alert: Record<string, unknown>


This is the successor to decorateResponse, whose argument type TopAlertResponse was only every indirectly defined via the return type of the server-side route registration. The removal of that route meant the type went away as well. The data fetching now happens via the timeline search strategy, which doesn't provide much type safety around the data, which is reflected by this Record<string, unknown>.

weltenwort · 2021-08-05T15:39:22Z

x-pack/test/apm_api_integration/tests/alerts/rule_registry.ts

- const { body: topAlerts, status: topAlertStatus } = await supertest
- .get(
- format({
- pathname: '/api/observability/rules/alerts/top',


Not sure why this was part of the APM api integration tests, but with the removal of that route this test won't work anymore. I don't think it's reasonable to query the timeline search strategy in here instead, so we'll have to rely on the ES document checks above.

elasticmachine · 2021-08-05T15:46:30Z

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

afgomez

Code looks alright! I left a small comment but we can track it afterwards

afgomez · 2021-08-10T13:47:31Z

x-pack/plugins/observability/public/pages/alerts/alerts_flyout/index.tsx

 import { SeverityBadge } from '../severity_badge';

 type AlertsFlyoutProps = {
 alert?: TopAlert;
- alerts?: TopAlertResponse[];
+ alerts?: Array<Record<string, unknown>>;


Even if we don't have much type safety now, would it make sense to extract Record<string, unknown> to a separate type? That way if/when we get better typings it's a simpler change

x-pack/plugins/observability/public/pages/alerts/parse_alert.ts

weltenwort · 2021-08-11T12:07:45Z

@afgomez I hope to have addressed your feedback, thank you for the review!

afgomez · 2021-08-11T15:10:26Z

Thank you @weltenwort !

This reverts commit 492378c.

This reverts commit a1267b1.

This reverts commit def6987.

This reverts commit c88d4cd.

kibanamachine · 2021-08-12T19:55:12Z

💚 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
`observability`	479.2KB	479.3KB	+160.0B

History

💚 Build #144829 succeeded 4bbb744
💔 Build #144559 failed 492378c
💔 Build #144478 failed a1267b1
💔 Build #144384 failed def6987
💚 Build #143667 succeeded 78a7b2f

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

cc @weltenwort

…lastic#107579) * Remove outdated top_alerts route and related types * Remove tests for deleted code * Remove test for deleted API * Remove reference to deleted type * Remove unused translations * Remove unused mock from story * Remove no-op alerts page story for now * Remove unsafe type assertions * Factor out alert field type * Compile kbn-io-ts-utils for the browser as well * Avoid deep import which doesn't work cross-platform * Revert "Avoid deep import which doesn't work cross-platform" This reverts commit 492378c. * Revert "Compile kbn-io-ts-utils for the browser as well" This reverts commit a1267b1. * Revert "Factor out alert field type" This reverts commit def6987. * Revert "Remove unsafe type assertions" This reverts commit c88d4cd. * Remove unsafe type assertions (again)

kibanamachine · 2021-08-12T20:25:14Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…107579) (#108452) * Remove outdated top_alerts route and related types * Remove tests for deleted code * Remove test for deleted API * Remove reference to deleted type * Remove unused translations * Remove unused mock from story * Remove no-op alerts page story for now * Remove unsafe type assertions * Factor out alert field type * Compile kbn-io-ts-utils for the browser as well * Avoid deep import which doesn't work cross-platform * Revert "Avoid deep import which doesn't work cross-platform" This reverts commit 492378c. * Revert "Compile kbn-io-ts-utils for the browser as well" This reverts commit a1267b1. * Revert "Factor out alert field type" This reverts commit def6987. * Revert "Remove unsafe type assertions" This reverts commit c88d4cd. * Remove unsafe type assertions (again) Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com>

Remove outdated top_alerts route and related types

7f8d440

weltenwort self-assigned this Aug 3, 2021

weltenwort added 8 commits August 3, 2021 23:42

Remove tests for deleted code

a6f2c40

Remove test for deleted API

6bf826a

Merge branch 'master' into obs-rac-remove-unused-api

de546f2

Remove reference to deleted type

3a22b8f

Merge branch 'master' into obs-rac-remove-unused-api

564b740

Remove unused translations

c0c794e

Remove unused mock from story

8ccbead

Merge branch 'master' into obs-rac-remove-unused-api

2917b5e

weltenwort commented Aug 5, 2021

View reviewed changes

weltenwort marked this pull request as ready for review August 5, 2021 15:46

weltenwort requested a review from a team as a code owner August 5, 2021 15:46

weltenwort requested a review from a team August 5, 2021 15:46

smith approved these changes Aug 5, 2021

View reviewed changes

weltenwort added 2 commits August 9, 2021 14:23

Merge branch 'master' into obs-rac-remove-unused-api

76c6964

Remove no-op alerts page story for now

78a7b2f

afgomez self-requested a review August 10, 2021 13:44

afgomez approved these changes Aug 10, 2021

View reviewed changes

weltenwort added 4 commits August 10, 2021 19:22

Merge branch 'master' into obs-rac-remove-unused-api

29d9a16

Remove unsafe type assertions

c88d4cd

Merge branch 'master' into obs-rac-remove-unused-api

f9965c5

Factor out alert field type

def6987

afgomez approved these changes Aug 11, 2021

View reviewed changes

weltenwort and others added 9 commits August 11, 2021 17:33

Compile kbn-io-ts-utils for the browser as well

a1267b1

Avoid deep import which doesn't work cross-platform

492378c

Revert "Avoid deep import which doesn't work cross-platform"

714020a

This reverts commit 492378c.

Revert "Compile kbn-io-ts-utils for the browser as well"

0d60ce7

This reverts commit a1267b1.

Revert "Factor out alert field type"

9475988

This reverts commit def6987.

Revert "Remove unsafe type assertions"

d66a474

This reverts commit c88d4cd.

Remove unsafe type assertions (again)

77631aa

Merge branch 'master' into obs-rac-remove-unused-api

4bbb744

Merge branch 'master' into obs-rac-remove-unused-api

ad01a55

weltenwort merged commit 45b33ba into elastic:master Aug 12, 2021

kibanamachine mentioned this pull request Aug 12, 2021

[7.x] [Observability] Remove outdated top_alerts route and related types (#107579) #108452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Observability] Remove outdated top_alerts route and related types #107579

[Observability] Remove outdated top_alerts route and related types #107579

weltenwort commented Aug 3, 2021 •

edited

Loading

weltenwort Aug 5, 2021

smith Aug 5, 2021

weltenwort Aug 5, 2021

afgomez Aug 10, 2021

weltenwort Aug 5, 2021

weltenwort Aug 5, 2021 •

edited

Loading

elasticmachine commented Aug 5, 2021

afgomez left a comment

afgomez Aug 10, 2021

weltenwort commented Aug 11, 2021 •

edited

Loading

afgomez commented Aug 11, 2021

kibanamachine commented Aug 12, 2021

kibanamachine commented Aug 12, 2021

[Observability] Remove outdated top_alerts route and related types #107579

[Observability] Remove outdated top_alerts route and related types #107579

Conversation

weltenwort commented Aug 3, 2021 • edited Loading

📝 Summary

🕵️ Review notes

weltenwort Aug 5, 2021

Choose a reason for hiding this comment

smith Aug 5, 2021

Choose a reason for hiding this comment

weltenwort Aug 5, 2021

Choose a reason for hiding this comment

afgomez Aug 10, 2021

Choose a reason for hiding this comment

weltenwort Aug 5, 2021

Choose a reason for hiding this comment

weltenwort Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Aug 5, 2021

afgomez left a comment

Choose a reason for hiding this comment

afgomez Aug 10, 2021

Choose a reason for hiding this comment

weltenwort commented Aug 11, 2021 • edited Loading

afgomez commented Aug 11, 2021

kibanamachine commented Aug 12, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented Aug 12, 2021

💚 Backport successful

weltenwort commented Aug 3, 2021 •

edited

Loading

weltenwort Aug 5, 2021 •

edited

Loading

weltenwort commented Aug 11, 2021 •

edited

Loading