feat: Add RequestId filter#10274
Conversation
michalsn
left a comment
michalsn
left a comment
There was a problem hiding this comment.
Thanks for working on this. I agree that having a request ID can be useful. However, I am not sure we should add this directly to IncomingRequest. I would prefer to see this as documentation only, or provide an opt-in filter that's disabled by default. This is a path similar to those followed by Symfony and Laravel.
If we provide an example or filter, it should store the value in context()->set('request_id', $id) so $logGlobalContext can pick it up, set X-Request-ID on the response in after(), and validate incoming IDs: non-empty, bounded length, and safe characters only.
Let's see what others think.
|
Thanks for working on this. I agree that having a request ID is useful. I also use this in my projects, but with a filter-based approach, and it has worked well in practice. I think that shape fits better here than adding it directly to The filter can resolve or generate the ID early, validate any incoming This avoids keeping separate request-id state on the request object, keeps the feature opt-in, and makes the ID useful for logs, response headers, and support/debug workflows. |
patel-vansh
changed the title
memleakd
left a comment
memleakd
left a comment
There was a problem hiding this comment.
Thanks for updating this. I think the filter direction is much better. I added a few comments on things that still look worth checking.
| $response->setHeader('X-Request-ID', context()->get('request_id')); | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Should this handle the case where request_id is missing from context?
If only the after filter is enabled, or context is cleared before after() runs, this currently sets X-Request-ID to an empty value. It may be safer to either resolve/generate the ID here as a fallback, or skip setting the header when no valid ID exists.
There was a problem hiding this comment.
I think skipping would be better, changed according to that.
memleakd
left a comment
memleakd
left a comment
There was a problem hiding this comment.
Thanks for the updates. This looks much better now. Just two small things left for me:
memleakd
left a comment
memleakd
left a comment
There was a problem hiding this comment.
I noticed one last thing with the filter ordering. I think requestid should stay after pagecache in after. PageCache::after() stores response headers, so if requestid runs first, we may accidentally cache X-Request-ID.
|
|
||
| $filter->before($request); | ||
|
|
||
| $requestId = context()->get('request_id'); |
There was a problem hiding this comment.
Can you also add asserts here that the request has the header line, and its value matches the one from context?
| $filter = new RequestId(); | ||
| $request = service('request', null, false); | ||
|
|
||
| $existingRequestId = 'Abc@!#$'; |
There was a problem hiding this comment.
Please add a separate test where the existing id is more than 255 chars.
| Framework-generated IDs are unique for practical purposes, however valid incoming IDs are reused. | ||
| It is added to the request's context and can be accessed via the ``request_id`` key. | ||
|
|
||
| The ID is generated via ``bin2hex(random_bytes(16))``, so it is a 32-character hexadecimal string. |
There was a problem hiding this comment.
This is an implementation detail. Do you need to expose that?
4 participants
Description
This PR adds
RequestIdfilter which will improve debugging, traceability, as well as better logging for each request.This filter is fully opt-in, that means there's no BC here. The filter line is commented so that users can easily add if needed.
Request id is generated during early lifecycle. Incoming header
X-Request-IDwill be prioritized over auto generation of request ids, so that CDN/web server/load balancer can generate request id for better traceability.If the header is not present, the framework will create request id via
bin2hex(random_bytes(16))function, that means the framework-generated ids will be of length 32.Why only
bin2hex?I initially thought of UUID v4, but due to lack of in-built support for UUID, I changed that to bin2hex. Adding a new external dependency for just this one thing would be less feasible, IMO. And also
random_bytes(16)should provide enough randomness so collision would be highly unlikely.If there is some other better way to generate id, then I would surely change to that.
Checklist: