-
Notifications
You must be signed in to change notification settings - Fork 94
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
Observer for http requests #154
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #154 +/- ##
=======================================
Coverage 92.57% 92.57%
=======================================
Files 6 8 +2
Lines 2666 2693 +27
=======================================
+ Hits 2468 2493 +25
- Misses 198 200 +2 ☔ View full report in Codecov by Sentry. |
@mnezerka PLS sign CLA licence for your user. |
@@ -20,7 +20,7 @@ | |||
# -- Project information ----------------------------------------------------- | |||
|
|||
project = 'PyOData' | |||
copyright = '2019 SAP SE or an SAP affiliate company' | |||
copyright = '2021 SAP SE or an SAP affiliate company' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be nice, separate PR, immediately merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, you update copyright, when you touch the code. This is change is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to separate commit to make it easier to cherry pick this change.
@@ -1048,6 +1050,8 @@ def association_set(self, set_name, namespace=None): | |||
except KeyError: | |||
pass | |||
|
|||
raise KeyError('Association set does not exist') | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be nice, separate PR, immediately merged, unrelated to the new observer class.
docs/usage/advanced.rst
Outdated
There are cases where you need access to the transport protocol (http). For | ||
example: you need to read value of specific http header. Pyodata provides | ||
simple mechanism to observe all http requests and access low lever properties | ||
directly from underlying engine (**python requests**). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not true. Pyodata does not have underlying engine (python requests) - it is networking library agnostic, dependency to requests is not direct in requirements.txt, by readme https://github.com/SAP/python-pyodata#usage it only expect Requests library Session-like object. Pyodata should work by design with other http networking libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only issue with the paragraph is that it mentions "python requests", otherwise I see no untrue parts :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that was the only line this comment was referring to :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I slightly rephrased this last sentence. Reference to python requests is sill there, but just as an example.
I think that this PR - apart from lines marked for another PR - should be closed and not merged. Rationale - unnecessary code and workaround without PR exists. Point 1 - It bring unnecessary code, which may be even tied to requests library only, while pyodata does not strictly use Point 2 - for the problems at hand - get to the response HTTP code, headers etc at runtime, when used with request Session instance, existing mechanism of Requests Hooks can be used, see requests documentation Sample code:
output:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request change - isolate the 3 lines outside of scope of Observer to new PR.
@phanak-sap Regarding your sample:
Are you sure you will be able to pair individual requests with calls of |
@phanak-sap I asked @mnezerka to implement this PR. I need to be able to get Headers for some requests. What's your proposal? |
Each response instance is calling the hooked function on itself in the Requests lib. I don't understand what kind of gluing you are talking about, but the I don't see problem for threads or asynchronous usage. |
Well, Point 2 in my comment is the proposed workaround for this use case. Is there anything that is accessible only by this new change and not by the Requests own hook system? If yes, I withdraw my case. If not, I don't see the necessity for the PR. |
The point 2 is good, I didn't know about Requests Hooks. However, I though I will change Michal's observer to a decorator, so consumer can do automatic post-processing of retrieved data. SAP BusinessGateway returns logs in HTTP headers and thought we could imbue the logs to return values of the method execute(). |
If you call:
so you don't know which call was executed first. Request hooks will call your callback twice. How do you know the originating reuqest? I understand that print_url gets instance of request or response, but this instance is from network layer (requests) and not pyodata. And this was my point - will you be able to match it to pyodata requests? So will you be able to say which one was for employee1 and which for employee2? |
@mnezerka OK, I seems to understand now. Since you are always talking about "hard to find originating request for it" or " pair individual requests with calls of print_url hook" and the Description of the PR is "This change allows caller to observe technical details (e.g. headers, body, etc.) of http requests that are sent in the background of odata calls." it gave me a wrong impression. It is true that in the hook workaround, you are outside of scope of the ODataHttpResponse.execute() and you see only the URL for the response, not the protected values in the pyodata scope above it (what Entitiset etc.). So far it never seemed it was a problem, use case seemed to be logging of responses based on status code/error in a header. But you can have anything in the hook. For the purpose of control flow, there could be for example an exception, so you know if the problem is in the employee1 or employe2 in the runtime. Example modified script, for use case of some say batch read/creation of some entities, which sometimes fails and we want to log what entity was having problem. Observer is another way, but with the necessity of pyodata code change (which I argue is not needed).
Output:
Any other use case which the Observer class must exists? I still think for the practical usage, hooks do just fine. Async/Threading should work IMHO just fine, each response instance and its hook call is isolated to another, if you think otherwise pls show me sample which fails. |
@jfilak OK, this one looks like is unsolvable by the hook, since it operates above and below the execute() method. Is this still needed or the proposed workaround with throwing the exception solve the same use case? In relevance to changing to decorator.. they look a bit nicer, but are a bit harder to debug. If the observer will be introduced, I would prefer current Michal's solution. |
Real use case and also motivation for this pull request: |
After some investigation, it seems to me that the whole "hooks will not work in async way / Imagine you are sharing instance of pyodata client (and request session) by more threads or other asynchronous constructions/pools" is just mental exercise "what if", while nobody so far really tried it. Because it seems it will not work anyway and with Pyodata at the moment you are stuck in sequential requests whether you like it or not.
It would be nice to see the actual working async/multithreading example of pyodata usage - to really prove the hook workaround is somehow not feasible. |
This change allows caller to observe technical details (e.g. headers, body, etc.) of http requests that are sent in the background of odata calls. ver 2: Incorporated review comments: * changed documentation - section where python requests are mentioned * removed copyright year increment - will be added as separate commit
@jfilak you stated that you would like to re-write it differently to decorator (in this comment ](#154 (comment))). Should it be merged as as? Change to draft and commit changes? Dropped (the need for the observer seems to be now lower in priority)? |
This change allows caller to observe technical details (e.g. headers, body, etc.) of http requests that are sent in the background of odata calls.