feat: Add RequestId filter#10274
Conversation
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. |
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.
|
|
||
| The ID is generated via ``bin2hex(random_bytes(16))``, so it is a 32-character hexadecimal string. | ||
|
|
||
| To enable this filter, simply un-comment the ``requestid`` alias in the ``$required['before']`` and ``$required['after']`` array in **app/Config/Filters.php**: |
There was a problem hiding this comment.
For upgraded apps, should this also mention adding the requestid alias? Existing app/Config/Filters.php files will not have it.
There was a problem hiding this comment.
I don't think upgradation notes should be added inside normal docs. IMO, it more belongs to changelog and upgrade guide rather than here. That's why I haven't added that in first place.
There was a problem hiding this comment.
I've now added a line in upgrade_480.rst so that the apps upgrading can easily do so.
There was a problem hiding this comment.
Sorry, I should have worded that better. I meant the wording here could be adjusted since some apps may still need to add the alias first:
- To enable this filter, simply un-comment the
+ To enable this filter, add/uncomment theThanks for adding the upgrade note!
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:
| 'before' => [ | ||
| 'forcehttps', // Force Global Secure Requests | ||
| // 'requestid', // Request ID for each request |
There was a problem hiding this comment.
Should requestid run before forcehttps? Otherwise a forced HTTPS redirect can happen before the request ID is created, so after() will not add X-Request-ID.
There was a problem hiding this comment.
The test file is discovered now, but the tests still only cover before(). Maybe we should add a few after() tests too, like checking that the response gets X-Request-ID and that valid/invalid incoming IDs are reflected correctly in the response.
| 'before' => [ | ||
| 'forcehttps', // Force Global Secure Requests | ||
| // 'requestid', // Request ID for each request |
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: