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

Introduced service time metrics to OpenSearch-Py client. #689

Closed

Conversation

saimedhi
Copy link
Collaborator

@saimedhi saimedhi commented Mar 6, 2024

Description

Introduced service time metrics to OpenSearch-Py client.

Issues Resolved

#678

Guide and Tests need to be added for this change.

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.

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 65.21739% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 72.06%. Comparing base (4b69c09) to head (5ea57d3).
Report is 2 commits behind head on main.

Files Patch % Lines
opensearchpy/metrics.py 60.00% 16 Missing ⚠️
opensearchpy/connection/http_requests.py 66.66% 3 Missing ⚠️
opensearchpy/connection/http_urllib3.py 66.66% 3 Missing ⚠️
opensearchpy/transport.py 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #689      +/-   ##
==========================================
- Coverage   72.14%   72.06%   -0.08%     
==========================================
  Files          89       90       +1     
  Lines        7945     8009      +64     
==========================================
+ Hits         5732     5772      +40     
- Misses       2213     2237      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@saimedhi
Copy link
Collaborator Author

saimedhi commented Mar 6, 2024

I'm unsure about my changes to async connections. I've tested them, but I'm uncertain if I've covered all cases. Let's start by applying these changes for requests and urllib3 connections first. For async connections, I'll need to review whether a similar approach to sync connections is suitable or if we should utilize something like aiohttptraceconfig.

I've created a copy of my tested and functional changes, which include async connections. However, I'm streamlining this PR to concentrate solely on sync connections.

@saimedhi saimedhi force-pushed the add/service_time_metrics branch 2 times, most recently from dd23f24 to aa3291c Compare March 6, 2024 20:17
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Good start. The if calculate_service_time: is the anti-pattern we need to fix here and make it more object-oriented. Define a generic Metrics interface with methods such as request_start, and a specific implementation for it using Events, e.g. EventsMetrics. See below.

opensearchpy/connection/http_requests.py Outdated Show resolved Hide resolved
opensearchpy/connection/http_requests.py Outdated Show resolved Hide resolved
opensearchpy/connection/http_requests.py Outdated Show resolved Hide resolved
opensearchpy/metrics.py Outdated Show resolved Hide resolved
@saimedhi saimedhi marked this pull request as draft March 15, 2024 23:39
@saimedhi saimedhi force-pushed the add/service_time_metrics branch 7 times, most recently from 91dc132 to f6c900e Compare March 18, 2024 21:11
@saimedhi saimedhi force-pushed the add/service_time_metrics branch from ee8ed35 to 0de0c4b Compare March 18, 2024 23:38
Signed-off-by: saimedhi <saimedhi@amazon.com>
@saimedhi saimedhi force-pushed the add/service_time_metrics branch from 0de0c4b to 5ea57d3 Compare March 18, 2024 23:47
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Better. Needs tests and some more changes.

opensearchpy/connection/http_requests.py Show resolved Hide resolved
opensearchpy/connection/http_requests.py Show resolved Hide resolved
opensearchpy/metrics.py Show resolved Hide resolved
opensearchpy/metrics.py Show resolved Hide resolved
opensearchpy/metrics.py Show resolved Hide resolved
opensearchpy/transport.py Show resolved Hide resolved
@saimedhi saimedhi closed this Apr 3, 2024
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.

2 participants