Service Time measurement Trail_Approach_1
#679
Conversation
saimedhi
requested review from
VachaShah,
dblock,
harshavamsi,
axeoman,
Shephalimittal and
florianvazelle
as code owners
|
Sample usage {'name': 'c3b344c0372b', |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #679 +/- ##
==========================================
- Coverage 72.14% 72.13% -0.02%
==========================================
Files 89 89
Lines 7945 7952 +7
==========================================
+ Hits 5732 5736 +4
- Misses 2213 2216 +3 ☔ View full report in Codecov by Sentry. |
|
|
||
| # Add the service time to the raw_data | ||
| if calculate_service_time: | ||
| raw_data = raw_data.rstrip()[:-1] + f', "__client": {{"Service_Time": "{duration}"}}' + '}' |
There was a problem hiding this comment.
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.
|
|
||
| # Add the service time to the raw_data | ||
| if calculate_service_time: | ||
| raw_data = raw_data.rstrip()[:-1] + f', "__client": {{"Service_Time": "{duration}"}}' + '}' |
| @@ -187,13 +191,15 @@ def perform_request( # type: ignore | |||
| "allow_redirects": allow_redirects, | |||
| } | |||
| send_kwargs.update(settings) | |||
| start = time.time() | |||
There was a problem hiding this comment.
I think the start at line 179 is fine. Any reason otherwise?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
I don't see this variable being used anywhere since ConnectionError is raised after.
|
This is a good start and shows what we're trying to achieve. What do you think about an idea where we first expose "events", then add "metrics"? Googling I found https://stackoverflow.com/questions/443885/python-callbacks-delegates-what-is-common and https://pypi.org/project/Events/ that could be a start. The client could expose events such as "request started", "finished", "errored", etc., and then another class called |
Description
Service time is calculated within the perform_request function of the Connection class ( This draft uses RequestsHttpConnection) at the point where the server call is made.
The calculated service time is added to the response or data in the connection.
I understand that this method is not ideal for returning service time and am exploring alternative options for improvement.
Issues Resolved
Related to #678
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.