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

Add request.id #2005

Merged
merged 3 commits into from
Jan 19, 2021
Merged

Add request.id #2005

merged 3 commits into from
Jan 19, 2021

Conversation

ahopkins
Copy link
Member

This adds a convenience property on the Request instance for tracking: request.id. The property will default to the value of the X-REQUEST-ID header if it is set. If not, it will fallback to a UUID.

There is a class method that will allow you to override the default (Request.generate_id).

@ahopkins ahopkins requested review from ashleysommer and vltr January 18, 2021 12:15
@codecov
Copy link

codecov bot commented Jan 18, 2021

Codecov Report

Merging #2005 (d84d2e9) into master (6c03dd8) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2005      +/-   ##
==========================================
+ Coverage   91.48%   91.52%   +0.04%     
==========================================
  Files          29       29              
  Lines        3323     3341      +18     
  Branches      583      585       +2     
==========================================
+ Hits         3040     3058      +18     
  Misses        201      201              
  Partials       82       82              
Impacted Files Coverage Δ
sanic/config.py 100.00% <ø> (ø)
sanic/request.py 99.25% <100.00%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c03dd8...d84d2e9. Read the comment docs.

@ashleysommer
Copy link
Member

Is this in response to a Feature request? I don't really see the benefit of doing this, but since the UUID is lazy and only generated on the first access to request.id, its probably not a problem to merge in.

In the past, when I need to have a unique ID for a request, I use the built-in python identity fn id(request).

@ahopkins
Copy link
Member Author

I had seen something in a somewhat older issue which triggered the idea. And, I had seen some related questions recently on SO. I thought about using id or some variant on itertools.count, but I think using a UUID is a well understood pattern.

As for the lazy, that was 100% the point. I did not want this to start throwing IDs where they are not needed. It feels like a somewhat nice compliment to request.token, which also could be made cacheable like request.id.

@ahopkins ahopkins merged commit 0d7e2f0 into master Jan 19, 2021
@ahopkins ahopkins deleted the request-id branch January 19, 2021 02:25
@alextsil
Copy link

alextsil commented Feb 6, 2021

I've been looking for a way to scope my sqlalchemy sessions to a per-request basis (docs here).
I've used request.__hash__() but its hashes are not 100% unique and could cause trouble.

This new ID property solves the issue neatly 👍

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.

3 participants