Skip to content
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

Add metric and stat entity #54

Merged
merged 12 commits into from
May 3, 2022
Merged

Conversation

VijayanB
Copy link
Member

@VijayanB VijayanB commented Apr 27, 2022

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

Description

Following metrics are added for the Upload API

  1. id : (string) Unique metric identifier
  2. upload: (integer) Number of documents that are requested to be uploaded.
  3. duration: (integer) Time, in milliseconds, spent preprocessing during upload.
  4. success: (integer) Number of documents that are successfully uploaded.
  5. failed: (integer) Number of documents that are failed to upload.

Issues Resolved

#53

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.

@VijayanB VijayanB requested a review from a team April 27, 2022 03:35
@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2022

Codecov Report

Merging #54 (ae7596f) into main (910108d) will decrease coverage by 0.09%.
The diff coverage is 89.80%.

@@             Coverage Diff              @@
##               main      #54      +/-   ##
============================================
- Coverage     88.82%   88.72%   -0.10%     
- Complexity      102      130      +28     
============================================
  Files            15       18       +3     
  Lines           331      470     +139     
  Branches         27       35       +8     
============================================
+ Hits            294      417     +123     
- Misses           33       48      +15     
- Partials          4        5       +1     
Impacted Files Coverage Δ
...n/upload/geojson/UploadGeoJSONTransportAction.java 0.00% <0.00%> (ø)
...nsearch/geospatial/action/upload/UploadMetric.java 86.36% <86.36%> (ø)
...l/action/upload/geojson/UploadGeoJSONResponse.java 86.48% <86.48%> (ø)
...rch/geospatial/action/upload/geojson/Uploader.java 94.73% <90.47%> (-5.27%) ⬇️
...ensearch/geospatial/action/upload/UploadStats.java 100.00% <100.00%> (ø)
...ial/action/upload/geojson/UploadGeoJSONAction.java 100.00% <100.00%> (ø)

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 93df8c1...ae7596f. Read the comment docs.

@VijayanB VijayanB force-pushed the add-stats branch 5 times, most recently from 53be034 to c173dae Compare April 27, 2022 16:16
@VijayanB VijayanB self-assigned this Apr 27, 2022
@VijayanB VijayanB force-pushed the add-stats branch 5 times, most recently from 73a5922 to b476167 Compare April 27, 2022 18:29
Copy link
Member

@jmazanec15 jmazanec15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you define the metrics/stats you are adding in the issue?

* @return Metric's Identifier
*/
public String getMetricID() {
return metricID;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the metricID?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unique identifier for every upload request

private long failedCount;
private String metricID;
private long successCount;
private long uploadCount;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these need to be initialized? Seems like metricID at the least should be a required parameter. What happens if this isnt set?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now metric id is required now. Default for all others are 0

*/
public void addMetric(UploadMetric newMetric) {
Objects.requireNonNull(newMetric, "metric cannot be null");
metrics.add(newMetric);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we check for duplicate metrics anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to HashSet and using metric ID to validate duplicate metrics

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Override toString to return lowercase.

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Since metrics are added to hashset, it doesn't guarante the
order anymore. Hence, test metrics individually.

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
This type represent the geospatial object it uploaded.

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
@VijayanB VijayanB merged commit 7fbdb83 into opensearch-project:main May 3, 2022
@VijayanB VijayanB deleted the add-stats branch May 3, 2022 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants