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.

Sign up for GitHub

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

Service Time measurement Trail_Approach_1 #679

Closed
wants to merge 14 commits into from
Closed

Service Time measurement Trail_Approach_1 #679

Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
870bc4a
Update http_requests.py
saimedhi Feb 21, 2024
bb18726
Update http_requests.py
saimedhi Feb 21, 2024
bff6a93
Update http_requests.py
saimedhi Feb 21, 2024
0484efb
Update http_requests.py
saimedhi Feb 21, 2024
b8b7c37
Update http_requests.py
saimedhi Feb 21, 2024
365d2f0
Update http_requests.py
saimedhi Feb 21, 2024
45e9c5b
Update http_requests.py
saimedhi Feb 21, 2024
98b6ac0
Update http_requests.py
saimedhi Feb 21, 2024
f848639
Update http_requests.py
saimedhi Feb 21, 2024
f9f6b71
Update transport.py
saimedhi Feb 21, 2024
f394b6f
Update http_requests.py
saimedhi Feb 21, 2024
e133d52
Update http_requests.py
saimedhi Feb 22, 2024
4e5e9aa
Update transport.py
saimedhi Feb 22, 2024
921cd55
Update http_requests.py
saimedhi Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions opensearchpy/connection/http_requests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
pool_maxsize: Any = None,
**kwargs: Any
) -> None:
self.kwargs=kwargs
if not REQUESTS_AVAILABLE:
raise ImproperlyConfigured(
"Please install requests to use RequestsHttpConnection."
Expand Down Expand Up @@ -166,6 +167,10 @@
ignore: Collection[int] = (),
headers: Optional[Mapping[str, str]] = None,
) -> Any:

calculate_service_time=False
if "calculate_service_time" in self.kwargs:
calculate_service_time=self.kwargs["calculate_service_time"]

Check warning on line 173 in opensearchpy/connection/http_requests.py

View check run for this annotation

Codecov / codecov/patch

opensearchpy/connection/http_requests.py#L173 

Added line #L173 was not covered by tests
url = self.base_url + url
headers = headers or {}
if params:
Expand All @@ -176,7 +181,6 @@
body = self._gzip_compress(body)
headers["content-encoding"] = "gzip" # type: ignore

start = time.time()
request = requests.Request(method=method, headers=headers, url=url, data=body)
prepared_request = self.session.prepare_request(request)
settings = self.session.merge_environment_settings(
Expand All @@ -187,13 +191,15 @@
"allow_redirects": allow_redirects,
}
send_kwargs.update(settings)
start = time.time()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the start at line 179 is fine. Any reason otherwise?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hello @VachaShah, I am adding start timer just before sending request. I think we don't need timer to start before prepared_request right?

try:
response = self.session.send(prepared_request, **send_kwargs)
duration = time.time() - start
raw_data = response.content.decode("utf-8", "surrogatepass")
except reraise_exceptions:
raise
except Exception as e:
duration = time.time() - start

Check warning on line 202 in opensearchpy/connection/http_requests.py

View check run for this annotation

Codecov / codecov/patch

opensearchpy/connection/http_requests.py#L202 

Added line #L202 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see this variable being used anywhere since ConnectionError is raised after.

self.log_request_fail(
method,
url,
Expand All @@ -207,7 +213,11 @@
if isinstance(e, requests.Timeout):
raise ConnectionTimeout("TIMEOUT", str(e), e)
raise ConnectionError("N/A", str(e), e)


# Add the service time to the raw_data
if calculate_service_time:
raw_data = raw_data.rstrip()[:-1] + f', "__client": {{"Service_Time": "{duration}"}}' + '}'

Check warning on line 219 in opensearchpy/connection/http_requests.py

View check run for this annotation

Codecov / codecov/patch

opensearchpy/connection/http_requests.py#L219 

Added line #L219 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

After a quick look, I think rather than attempting to modify the JSON string like this, it'd make more sense to just return the value from this function. ie. changing return response.status_code, response.headers, raw_data to return response.status_code, response.headers, raw_data, duration or creating some 'RequestMetricsclass that contains the duration (and potential additional future metrics) which is returned. Then in the transport class inserting that into the returned/deserialized object asmetrics` or something like that.

Adding it to the return signature of the connection class gives a clear and uniform way of returning this information across all connection implementations.

Some one with more Python prowess will hopefully be able to give a better idea of what's actually idiomatic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for this.


# raise warnings if any from the 'Warnings' header.
warnings_headers = (
(response.headers["warning"],) if "warning" in response.headers else ()
Expand Down
Loading