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

[#1185] Outgoing requests tracking #524

Merged
merged 4 commits into from
Mar 15, 2023

Conversation

vaszig
Copy link
Contributor

@vaszig vaszig commented Mar 8, 2023

This is an approach according to the fact that we already use requests library (same for the external libraries we use for the APIs), by adding a hook to each request (based on a session). Also this approach was discussed with Sergei and Bart at some point.

@Bartvaderkin @alextreme feel free to have a first look and if you agree I can continue with making this more stable.

@alextreme
Copy link
Member

I think this would be a neat way to do this. Couple of thoughts:

  • a Django setting to toggle this on/off (I think the default should be on if installed, but this allows it to easily be disabled if not wanted/necessary)
  • Tests :P
  • Split this off into a separate/external Django app to allow easy reuse (can be before or after we've tested it out in OIP)

Looking further this only works for requests when making use of requests-sessions, correct? So this does mean that plain requests.get() calls would need to be rewritten. I'd prefer to also track requests when sessions aren't used (so we have a drop-in component for this), but I don't know if that's possible and for an initial version that we can try out as long as we see the BRP and ZGW requests being sent this would be sufficient.

@vaszig
Copy link
Contributor Author

vaszig commented Mar 9, 2023

Looking further this only works for requests when making use of requests-sessions, correct? So this does mean that plain requests.get() calls would need to be rewritten. I'd prefer to also track requests when sessions aren't used (so we have a drop-in component for this), but I don't know if that's possible and for an initial version that we can try out as long as we see the BRP and ZGW requests being sent this would be sufficient.

Requests library sends under the hood all requests through requests.Session, so a plain request with requests.get("url") is logged as well (https://github.com/psf/requests/blob/main/requests/api.py#L58) I hope this is what you mean.

2023-03-09_08-43

@alextreme
Copy link
Member

Compromise is to put this in src/external_api_logging/ to allow easier migration to a third-party Django app

src/open_inwoner/conf/base.py Outdated Show resolved Hide resolved
src/log_outgoing_requests/log_requests.py Outdated Show resolved Hide resolved
src/log_outgoing_requests/models.py Show resolved Hide resolved
src/log_outgoing_requests/admin.py Outdated Show resolved Hide resolved
Comment on lines 27 to 28
reqhdrs=self._formatHeaders(record.req.headers),
reshdrs=self._formatHeaders(record.res.headers),
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the header information for debugging, but I'm not sure if we can store that in logs. @alextreme can pitch in on that.

Copy link
Member

Choose a reason for hiding this comment

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

The other headers should be fine, but I'd recommend to avoid logging the Authorization header

src/log_outgoing_requests/admin.py Show resolved Hide resolved
src/open_inwoner/conf/base.py Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #524 (20eaf77) into develop (95cbe9d) will increase coverage by 0.04%.
The diff coverage is 96.81%.

@@             Coverage Diff             @@
##           develop     #524      +/-   ##
===========================================
+ Coverage    96.47%   96.52%   +0.04%     
===========================================
  Files          527      538      +11     
  Lines        19023    19206     +183     
===========================================
+ Hits         18353    18538     +185     
+ Misses         670      668       -2     
Impacted Files Coverage Δ
src/log_outgoing_requests/log_requests.py 88.23% <88.23%> (ø)
src/log_outgoing_requests/handlers.py 93.33% <93.33%> (ø)
src/log_outgoing_requests/admin.py 94.73% <94.73%> (ø)
src/log_outgoing_requests/models.py 95.83% <95.83%> (ø)
src/log_outgoing_requests/apps.py 100.00% <100.00%> (ø)
src/log_outgoing_requests/formatters.py 100.00% <100.00%> (ø)
...c/log_outgoing_requests/migrations/0001_initial.py 100.00% <100.00%> (ø)
src/log_outgoing_requests/tests/test_logging.py 100.00% <100.00%> (ø)
src/open_inwoner/conf/base.py 95.23% <100.00%> (+0.17%) ⬆️

... and 11 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

blank=True,
default="",
help_text=_("The url of the outgoing request."),
)
hostname = models.CharField(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd put a comment here to note that .hostname is obviously part of .url but we also have it as a field so we can filter on it in the admin.

Comment on lines +80 to +83
@cached_property
def query_params(self):
parsed_url = urlparse(self.url)
return parsed_url.query
Copy link
Contributor

@Bartvaderkin Bartvaderkin Mar 15, 2023

Choose a reason for hiding this comment

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

This is ok for now since we only take the query, but if we'd want more url components you'd cache the urlparse() result and access parts of it like so:

@cached_property
def url_parsed(self):
    return urlparse(self.url)

@property
def query_params(self):
    return self.url_parsed.query

@property
def scheme(self):
    return self.url_parsed.scheme

@alextreme alextreme merged commit 9a3e12e into develop Mar 15, 2023
@alextreme alextreme deleted the feature/1185-track-external-api-requests branch March 15, 2023 14:40
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