-
Notifications
You must be signed in to change notification settings - Fork 23
Add a new requestID to Log fields #157
Conversation
Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
Codecov Report
@@ Coverage Diff @@
## master #157 +/- ##
==========================================
- Coverage 68.16% 67.96% -0.21%
==========================================
Files 69 69
Lines 4081 4083 +2
==========================================
- Hits 2782 2775 -7
- Misses 1141 1149 +8
- Partials 158 159 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
wild-endeavor
left a comment
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.
why is this the only one with metadata though?
|
Because it's the only one relevant to the outgoing requests. If you have a scenario in mind for one of the other fields being relevant to a server, we can update that as well... |
Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
|
@eapolinario did more testing, nginx, I've not tested traefik, seem to only look for x-request-id, otherwise it'll generate a new request_id (see).. this template can be overridden but I don't think it's worth having people go through that just to plumb through a request id... Sorry but I'm reverting to the old, wish to be deprecated x- prefix... |
Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
eapolinario
left a comment
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.
Makes sense. Thanks for double checking!
* Add a new requestID to Log fields Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Also set the requestID on grpc outgoing requests Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * set key and value Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * lint :( Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Use more commonly used string for request id Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * using request-id instead per PR Comment discussion Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * reverting to x- prefix to keep OOB behavior sane Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> --------- Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
TL;DR
Add a request ID contextutils fiels.
"this happened"will be tagged with the request idType
Are all requirements met?
Tracking Issue
fixes flyteorg/flyte#3577