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

Pass X-Request-Id if present #3072

Merged
merged 6 commits into from
Sep 22, 2023

Conversation

paunik
Copy link
Contributor

@paunik paunik commented Sep 21, 2023

Pass the value from the X-Request-Id to the logger context

  • pass the X-Request-Id header value if present, pass generated UUID if the header not present

Mentioned:

Why:

Inspiration:

- pass the X-Request-Id header value if present, pass generated UUID if the header not present
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine, aside from the conflict markers left unresolved.

Sources/Vapor/Request/Request.swift Outdated Show resolved Hide resolved
@gwynne gwynne added enhancement New feature or request semver-patch Internal changes only labels Sep 21, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2023

Codecov Report

Merging #3072 (fdb632d) into main (a4b0715) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3072   +/-   ##
=======================================
  Coverage   76.21%   76.22%           
=======================================
  Files         211      211           
  Lines        7821     7824    +3     
=======================================
+ Hits         5961     5964    +3     
  Misses       1860     1860           
Files Changed Coverage Δ
Sources/Vapor/HTTP/Headers/HTTPHeaders+Name.swift 74.07% <ø> (ø)
Sources/Vapor/Request/Request.swift 76.34% <100.00%> (+0.78%) ⬆️

@gwynne gwynne requested a review from MahdiBM September 22, 2023 02:34
Copy link
Contributor

@dannflor dannflor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautamous

@gwynne gwynne enabled auto-merge (squash) September 22, 2023 06:09
@gwynne gwynne merged commit d79fad4 into vapor:main Sep 22, 2023
15 checks passed
@penny-for-vapor
Copy link

These changes are now available in 4.83.1

Copy link
Contributor

@MahdiBM MahdiBM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly considering this header doesn't make an appearance in MDN, I wouldn't hard-code it like this 🤔

I would instead add a configuration option so other users can configure it "based on headers", and there they could specify the "X-Request-ID" header.

I know this complicates the solution, but we have "tracing" and "task-locals" for logging now which enable you to this yourself to some extent, and caring about this particular header seems out of scope for Vapor to me.

To be clear I still like the idea and I can think of some nice use cases for it.

@paunik paunik deleted the feature/pass-x-request-id-if-present branch September 22, 2023 06:34
@gwynne
Copy link
Member

gwynne commented Sep 22, 2023

@MahdiBM I disagree - X-Request-ID is not an official standard, true, but it is a de facto one. (Enough so, even for people to have gone to the trouble of noting that RFC 6648 deprecated the X- prefix on headers and remarked on the need to find a new name.) Unless you can cite links to any alternate names commonly used for the same purpose, configurability would just bloat the API surface and force users to do additional work without adding any real value.

(And as for it being out of scope, we put Vapor into the scope of caring about request IDs when we made it generate random ones by default.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semver-patch Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants