Include stats api to provide upload metrics #64

VijayanB · 2022-05-12T16:53:23Z

Description

Added stats api "_plugins/geospatial/_upload/stats", to retrieve statistics for uploads
in cluster. By default all stats are returned, user can filter the response using
filter query parameter similar to other cluster response.

Scenario 1: No upload is processed

curl -X GET "localhost:9200/_plugins/geospatial/_upload/stats?pretty"
{
  "total" : {
    "request_count" : 0,
    "upload" : 0,
    "success" : 0,
    "failed" : 0,
    "duration" : 0
  },
  "metrics" : [ ]
}

Scenario 2: After 1 successful upload is processed

 curl -X GET "localhost:9200/_plugins/geospatial/_upload/stats?pretty"
{
  "total" : {
    "request_count" : 1,
    "upload" : 5,
    "success" : 5,
    "failed" : 0,
    "duration" : 881
  },
  "metrics" : [
    {
      "node_id" : "VCkDk5YdQKWuwnfrTkNTNQ",
      "id" : "tIOSz4t4Rz2pt6rO4UybXg",
      "type" : "geojson",
      "upload" : 5,
      "success" : 5,
      "failed" : 0,
      "duration" : 881
    }
  ]
}

Scenario 3: After 1 partial upload is processed

 curl -X GET "localhost:9200/_plugins/geospatial/_upload/stats?pretty"
{
  "total" : {
    "request_count" : 2,
    "upload" : 62,
    "success" : 60,
    "failed" : 2,
    "duration" : 1814
  },
  "metrics" : [
    {
      "node_id" : "VCkDk5YdQKWuwnfrTkNTNQ",
      "id" : "tIOSz4t4Rz2pt6rO4UybXg",
      "type" : "geojson",
      "upload" : 5,
      "success" : 5,
      "failed" : 0,
      "duration" : 881
    },
    {
      "node_id" : "VCkDk5YdQKWuwnfrTkNTNQ",
      "id" : "fHHlz4iWQ4uJgoN5dZeEwA",
      "type" : "geojson",
      "upload" : 57,
      "success" : 55,
      "failed" : 2,
      "duration" : 933
    }
  ]
}

Issues Resolved

Check List

Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Use short copyright header. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

Shivamdhar · 2022-05-13T21:38:47Z

src/test/java/org/opensearch/geospatial/stats/upload/UploadStatsServiceTests.java

@@ -7,6 +7,7 @@

 import static org.opensearch.common.xcontent.XContentFactory.jsonBuilder;
 import static org.opensearch.geospatial.GeospatialTestHelper.buildFieldNameValuePair;
+import static org.opensearch.geospatial.GeospatialTestHelper.removeStartAndEndObject;


I think this is an unused import. Did we add checkstyle to the repo?

This method used to be part of this test, i moved it to Helper class, hence, you see change in import

jmazanec15

@VijayanB I think using filter_path to filter breaks pattern for a lot of other stats APIs, including node stats. Typically, its .../stats/{metric}

jmazanec15 · 2022-05-18T22:57:34Z

src/main/java/org/opensearch/geospatial/stats/RestStatsAction.java

+
+    @Override
+    protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient nodeClient) {
+        return channel -> nodeClient.execute(StatsAction.INSTANCE, new StatsRequest(), new RestToXContentListener<>(channel));


Can we add option to limit nodes request gets routed to?

Sorry i couldn't understand. Do you mean to add support to include node_id parameter?

Yes, assuming that the response contains node specific stats? Say I have a 100 node cluster and only want info for 1 node.

src/main/java/org/opensearch/geospatial/stats/StatsNodeResponse.java

src/main/java/org/opensearch/geospatial/stats/StatsResponse.java

src/main/java/org/opensearch/geospatial/stats/StatsTransportAction.java

VijayanB · 2022-05-19T00:32:24Z

@VijayanB I think using filter_path to filter breaks pattern for a lot of other stats APIs, including node stats. Typically, its .../stats/{metric}

I see. It looks like anti-pattern based on REST spec. Nevertheless, let me create an issue to see whether we need to support that format since node stats api follow this format.

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

Added Request,NodeRequest,ResponseNodeResponse,Action classes with boilerplate code and method. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

Added stats api "_plugins/geospatial/stats", to retrieve statistics for uploads in cluster. By default all stats are returned, user can filter the response using filter query parameter similar to other cluster response. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

jmazanec15 · 2022-05-20T16:10:22Z

src/main/java/org/opensearch/geospatial/stats/StatsNodeRequest.java

+    private StatsRequest request;
+
+    public StatsNodeRequest() {
+        super();


This would make request null. Is there a reason for this constructor? I remember somewhere there may have been a required no-arg constructor but I cant remember if it was here.

Good catch. I removed the default constructor. The one that needs default constructor is TransportAction

src/test/java/org/opensearch/geospatial/stats/RestStatsActionIT.java

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

codecov-commenter · 2022-05-25T00:11:46Z

Codecov Report

Merging #64 (89a554c) into main (f2360e5) will decrease coverage by 4.48%.
The diff coverage is 45.31%.

❗ Current head 89a554c differs from pull request most recent head 722ab09. Consider uploading reports for the commit 722ab09 to get more accurate results

@@             Coverage Diff              @@
##               main      #64      +/-   ##
============================================
- Coverage     90.69%   86.20%   -4.49%     
- Complexity      146      159      +13     
============================================
  Files            21       28       +7     
  Lines           548      609      +61     
  Branches         39       41       +2     
============================================
+ Hits            497      525      +28     
- Misses           47       78      +31     
- Partials          4        6       +2

Impacted Files	Coverage Δ
...eospatial/stats/upload/UploadStatsNodeRequest.java	`0.00% <0.00%> (ø)`
...ch/geospatial/stats/upload/UploadStatsRequest.java	`0.00% <0.00%> (ø)`
...atial/stats/upload/UploadStatsTransportAction.java	`0.00% <0.00%> (ø)`
...geospatial/stats/upload/RestUploadStatsAction.java	`20.00% <20.00%> (ø)`
...h/geospatial/stats/upload/UploadStatsResponse.java	`62.50% <62.50%> (ø)`
...opensearch/geospatial/plugin/GeospatialPlugin.java	`88.88% <75.00%> (-11.12%)`	⬇️
...ospatial/stats/upload/UploadStatsNodeResponse.java	`76.92% <76.92%> (ø)`
...pensearch/geospatial/stats/upload/UploadStats.java	`100.00% <100.00%> (ø)`
...rch/geospatial/stats/upload/UploadStatsAction.java	`100.00% <100.00%> (ø)`
...ch/geospatial/stats/upload/UploadStatsService.java	`100.00% <100.00%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2360e5...722ab09. Read the comment docs.

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

VijayanB requested a review from a team May 12, 2022 16:53

VijayanB force-pushed the add-stats-rest branch 4 times, most recently from 1131224 to 078d59a Compare May 12, 2022 22:09

Update copyright value

d577a88

Use short copyright header. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

VijayanB force-pushed the add-stats-rest branch from 078d59a to 642aa14 Compare May 13, 2022 18:14

VijayanB self-assigned this May 13, 2022

Shivamdhar approved these changes May 13, 2022

View reviewed changes

jmazanec15 reviewed May 18, 2022

View reviewed changes

VijayanB added 4 commits May 18, 2022 18:20

Add null argument check for new UploadStats

7349d3e

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

Add skeleton class and methods for stats API

310b693

Added Request,NodeRequest,ResponseNodeResponse,Action classes with boilerplate code and method. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

Add integration test for stats API

406f939

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

VijayanB force-pushed the add-stats-rest branch from 642aa14 to 406f939 Compare May 19, 2022 01:21

jmazanec15 reviewed May 20, 2022

View reviewed changes

VijayanB added 2 commits May 24, 2022 15:42

Rename class to UploadStats

34113f5

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

Remove upload from response

9d64ced

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

VijayanB force-pushed the add-stats-rest branch from e1f240f to 9d64ced Compare May 25, 2022 00:08

Move upload api to super class

86a6a1b

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

VijayanB force-pushed the add-stats-rest branch from 323652a to 86a6a1b Compare May 25, 2022 00:27

Refactor upload integration test

28a7dbd

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

VijayanB requested a review from jmazanec15 May 25, 2022 02:04

VijayanB force-pushed the add-stats-rest branch 3 times, most recently from 216f301 to e9f466f Compare May 25, 2022 18:35

Add test case for stream

736cea2

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

VijayanB force-pushed the add-stats-rest branch from e9f466f to 736cea2 Compare May 25, 2022 18:38

jmazanec15 approved these changes May 25, 2022

View reviewed changes

VijayanB merged commit 0b21de6 into opensearch-project:main May 25, 2022

VijayanB deleted the add-stats-rest branch May 25, 2022 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include stats api to provide upload metrics #64

Include stats api to provide upload metrics #64

VijayanB commented May 12, 2022 •

edited

Loading

Shivamdhar May 13, 2022

VijayanB May 18, 2022

jmazanec15 left a comment

jmazanec15 May 18, 2022

VijayanB May 19, 2022

jmazanec15 May 20, 2022

VijayanB commented May 19, 2022

jmazanec15 May 20, 2022

VijayanB May 25, 2022

codecov-commenter commented May 25, 2022 •

edited

Loading

Include stats api to provide upload metrics #64

Include stats api to provide upload metrics #64

Conversation

VijayanB commented May 12, 2022 • edited Loading

Description

Issues Resolved

Check List

Shivamdhar May 13, 2022

Choose a reason for hiding this comment

VijayanB May 18, 2022

Choose a reason for hiding this comment

jmazanec15 left a comment

Choose a reason for hiding this comment

jmazanec15 May 18, 2022

Choose a reason for hiding this comment

VijayanB May 19, 2022

Choose a reason for hiding this comment

jmazanec15 May 20, 2022

Choose a reason for hiding this comment

VijayanB commented May 19, 2022

jmazanec15 May 20, 2022

Choose a reason for hiding this comment

VijayanB May 25, 2022

Choose a reason for hiding this comment

codecov-commenter commented May 25, 2022 • edited Loading

Codecov Report

VijayanB commented May 12, 2022 •

edited

Loading

codecov-commenter commented May 25, 2022 •

edited

Loading