[Stack Monitoring] Converts logstash api routes to typescript #131946

crespocarlos · 2022-05-10T15:43:22Z

Summary

This converts the logstash api routes to TypeScript.

Additional nodes

Logstash api folder has a sub directory called pipelines. I'm not sure if we should keep it like thas, but I didn't change the folder structure. I've decided not to create a subdirectory in the new http_api directory structure to add the new type definitions.

crespocarlos · 2022-05-10T15:44:57Z

x-pack/plugins/monitoring/common/types/es.ts

@@ -142,6 +142,14 @@ export interface ElasticsearchIndexStats {
  };
 }

+export interface ElasticsearchLogstashStatePipeline {


I've created this type to improve the return type readability on getPipeline function

crespocarlos · 2022-05-10T15:45:37Z

x-pack/plugins/monitoring/server/lib/logstash/get_cluster_status.ts

-  return getLogstashForClusters(req, clusters).then((clusterStatus) =>
-    get(clusterStatus, '[0].stats')
+  return getLogstashForClusters(req, clusters).then(
+    (clusterStatus) => clusterStatus && clusterStatus[0]?.stats


I've seen lodash being removed somewhere else. I thinks this way is safe

There was no defaultValue passed to get so this LGTM.

crespocarlos · 2022-05-10T15:46:40Z

x-pack/plugins/monitoring/server/routes/api/v1/logstash/pipeline.ts


      try {
-        const [pipeline, vertex] = await Promise.all(promises);
+        const [pipeline, vertex] = await Promise.all([


This is like this so that ts can be able to infer the correct type of pipeline and vertex

crespocarlos · 2022-05-10T15:50:51Z

x-pack/plugins/monitoring/server/routes/api/v1/logstash/pipelines/cluster_pipelines.ts

+} from '../../../../../../common/http_api/logstash';
+
+const throughputMetric = 'logstash_cluster_pipeline_throughput';
+const nodesCountMetric = 'logstash_cluster_pipeline_node_count';


Previously, this was logstash_cluster_pipeline_nodes_count. However this is not a valid type (see: here). So we need to either include logstash_cluster_pipeline_nodes_count to the union or use a valid option here.

I actually had to add logstash_cluster_pipeline_nodes_count to PipelineNodeCountMetricKey. The endpoint was returning 500 when logstash_cluster_pipeline_node_count was passed

crespocarlos · 2022-05-10T15:52:34Z

x-pack/plugins/monitoring/server/routes/api/v1/logstash/pipelines/node_pipelines.ts

+} from '../../../../../../common/http_api/logstash';
+
+const throughputMetric = 'logstash_node_pipeline_throughput';
+const nodesCountMetric = 'logstash_node_pipeline_node_count';


Previously, this was logstash_node_pipeline_nodes_count. However this is not a valid type (see: here). So we need to either include logstash_node_pipeline_nodes_count to the union or use a valid option here.

I actually had to add logstash_node_pipeline_nodes_count to PipelineNodeCountMetricKey. The endpoint was returning 500 when logstash_node_pipeline_nodes_count was passed

elasticmachine · 2022-05-10T15:58:44Z

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

crespocarlos · 2022-05-11T11:56:34Z

x-pack/plugins/monitoring/common/http_api/logstash/post_logstash_cluster_pipelines.ts

+  rt.partial({
+    ccs: ccsRT,
+    sort: sortingRT,
+    queryText: rt.string,


The original type had a default value of '' when queryText was not sent. Should we have this validation here?

As queryText can sometimes not be sent, having it as optional here on the io-ts request payload type seems accurate to me. Imo, it should fall to other functions to then check for that and hand what is needed to other call sites.

crespocarlos · 2022-05-11T18:36:44Z

@elasticmachine merge upstream

kibana-ci · 2022-05-18T17:38:23Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #44188 succeeded a4b36212f8d29a76f05d5d50b7c73b610d13c2b2
💚 Build #44113 succeeded 6f16993416a9c2c978d25255dde76d50f2c2db0f
💔 Build #44015 failed 8860b61a99db3d3c55c83e50de0ceb39b864e64b
💔 Build #43944 failed b9552f66c15794ccf5ea1ad8ca8a0fb5fe25602f
💔 Build #43760 failed 87f5c58b66d9a7cdc97809b9e27a014e64162bfd

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

Kerry350

Great work, LGTM 🙌

Thanks for the additional improvements (removing Lodash and so on).

Kerry350 · 2022-05-19T11:51:28Z

x-pack/plugins/monitoring/common/http_api/logstash/post_logstash_cluster_pipelines.ts

+  rt.partial({
+    ccs: ccsRT,
+    sort: sortingRT,
+    queryText: rt.string,


As queryText can sometimes not be sent, having it as optional here on the io-ts request payload type seems accurate to me. Imo, it should fall to other functions to then check for that and hand what is needed to other call sites.

Kerry350 · 2022-05-19T11:56:58Z

x-pack/plugins/monitoring/server/lib/logstash/get_cluster_status.ts

-  return getLogstashForClusters(req, clusters).then((clusterStatus) =>
-    get(clusterStatus, '[0].stats')
+  return getLogstashForClusters(req, clusters).then(
+    (clusterStatus) => clusterStatus && clusterStatus[0]?.stats


There was no defaultValue passed to get so this LGTM.

crespocarlos requested a review from a team as a code owner May 10, 2022 15:43

crespocarlos commented May 10, 2022

View reviewed changes

crespocarlos added the backport:skip This commit does not require backporting label May 10, 2022

crespocarlos changed the title ~~converts logstash api routes to typescript~~ [Stack Monitoring] Converts logstash api routes to typescript May 10, 2022

crespocarlos added Feature:Stack Monitoring release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services labels May 10, 2022

crespocarlos added the v8.3.0 label May 10, 2022

crespocarlos commented May 11, 2022

View reviewed changes

Kerry350 self-requested a review May 18, 2022 16:07

crespocarlos added 4 commits May 18, 2022 18:33

converts logstash api routes to typescript

ee140b2

fixes register routes function names

15177fb

fixes cluster and node pipelines routes

8c02af5

fixes types

411fc59

crespocarlos force-pushed the 117763-covert-logstash-routes-ts branch from a4b3621 to 411fc59 Compare May 18, 2022 16:33

Kerry350 approved these changes May 19, 2022

View reviewed changes

crespocarlos merged commit fa607d1 into elastic:main May 19, 2022

crespocarlos deleted the 117763-covert-logstash-routes-ts branch May 19, 2022 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Stack Monitoring] Converts logstash api routes to typescript #131946

[Stack Monitoring] Converts logstash api routes to typescript #131946

crespocarlos commented May 10, 2022 •

edited

Loading

crespocarlos May 10, 2022

crespocarlos May 10, 2022

Kerry350 May 19, 2022

crespocarlos May 10, 2022 •

edited

Loading

crespocarlos May 10, 2022 •

edited

Loading

crespocarlos May 11, 2022

crespocarlos May 10, 2022

crespocarlos May 11, 2022

elasticmachine commented May 10, 2022

crespocarlos May 11, 2022

Kerry350 May 19, 2022

crespocarlos commented May 11, 2022

kibana-ci commented May 18, 2022

Kerry350 left a comment

Kerry350 May 19, 2022

Kerry350 May 19, 2022

[Stack Monitoring] Converts logstash api routes to typescript #131946

[Stack Monitoring] Converts logstash api routes to typescript #131946

Conversation

crespocarlos commented May 10, 2022 • edited Loading

Summary

Additional nodes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crespocarlos May 10, 2022 • edited Loading

Choose a reason for hiding this comment

crespocarlos May 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crespocarlos commented May 11, 2022

kibana-ci commented May 18, 2022

💚 Build Succeeded

Metrics [docs]

History

Kerry350 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crespocarlos commented May 10, 2022 •

edited

Loading

crespocarlos May 10, 2022 •

edited

Loading

crespocarlos May 10, 2022 •

edited

Loading