Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Add a new requestID to Log fields #157

Merged
merged 7 commits into from
Apr 8, 2023
Merged

Add a new requestID to Log fields #157

merged 7 commits into from
Apr 8, 2023

Conversation

EngHabu
Copy link
Contributor

@EngHabu EngHabu commented Apr 7, 2023

TL;DR

Add a request ID contextutils fiels.

ctx = contextutils.WithRequestID(ctx, "request-123")
logger.Infof(ctx, "this happened")

"this happened" will be tagged with the request id

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Tracking Issue

fixes flyteorg/flyte#3577

Signed-off-by: Haytham Abuelfutuh <[email protected]>
eapolinario
eapolinario previously approved these changes Apr 7, 2023
@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Merging #157 (8eac382) into master (0dbe3c2) will decrease coverage by 0.21%.
The diff coverage is 100.00%.

@@            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     
Flag Coverage Δ
unittests 67.96% <100.00%> (-0.21%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
contextutils/context.go 80.76% <100.00%> (+0.50%) ⬆️

... 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 <[email protected]>
wild-endeavor
wild-endeavor previously approved these changes Apr 7, 2023
Copy link
Contributor

@wild-endeavor wild-endeavor left a 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?

@EngHabu
Copy link
Contributor Author

EngHabu commented Apr 7, 2023

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 <[email protected]>
wild-endeavor
wild-endeavor previously approved these changes Apr 7, 2023
katrogan
katrogan previously approved these changes Apr 7, 2023
@EngHabu EngHabu dismissed stale reviews from katrogan and wild-endeavor via 8eb86c5 April 8, 2023 00:50
eapolinario
eapolinario previously approved these changes Apr 8, 2023
@EngHabu
Copy link
Contributor Author

EngHabu commented Apr 8, 2023

@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...

Copy link
Contributor

@eapolinario eapolinario left a 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!

@EngHabu EngHabu merged commit 3b737b5 into master Apr 8, 2023
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* Add a new requestID to Log fields

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Also set the requestID on grpc outgoing requests

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* set key and value

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* lint :(

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Use more commonly used string for request id

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* using request-id instead per PR Comment discussion

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* reverting to x- prefix to keep OOB behavior sane

Signed-off-by: Haytham Abuelfutuh <[email protected]>

---------

Signed-off-by: Haytham Abuelfutuh <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Housekeeping] Add a RequestID field to contextutils and logger
4 participants