Skip to content

feat: Added support for logging of HTTP Headers #4803

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

Merged
merged 26 commits into from
Sep 1, 2022

Conversation

zspeaks
Copy link
Contributor

@zspeaks zspeaks commented Jul 27, 2022

What this PR does:
Adds support for the addition of the contents of HTTP headers to logs.
Which issue(s) this PR fixes:
Fixes #4799

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@alvinlin123
Copy link
Contributor

@zspeaks can you also share an example how the actual log looks like?

@zspeaks
Copy link
Contributor Author

zspeaks commented Jul 28, 2022

Here's an example of what the logging looks like. Note: for the purposes of this example, I'm sending simulated requests with known headers and contents. In an actual running scenario, these additions to the logs would appear alongside the other information in the logs NOT on their own as they do here. With that said, here's examples for 1, 2 and 3 headers being included in the request.
Screen Shot 2022-07-28 at 9 27 00 AM

@zspeaks zspeaks reopened this Aug 15, 2022
zspeaks and others added 4 commits August 23, 2022 18:45
Added Header Propagation to Ingester and Querier
Signed-off-by: Zach Speaks <[email protected]>
Signed-off-by: Zachary Speaks <[email protected]>
Signed-off-by: Zach Speaks <[email protected]>
Signed-off-by: Zachary Speaks <[email protected]>
Signed-off-by: Zach Speaks <[email protected]>
Signed-off-by: Zachary Speaks <[email protected]>
Signed-off-by: Zachary Speaks <[email protected]>
Copy link
Contributor

@danielblando danielblando left a comment

Choose a reason for hiding this comment

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

LGTM.
Just remember to update changelog

Signed-off-by: Zachary Speaks <[email protected]>
Comment on lines 155 to 160
func InjectHeadersIntoHTTPRequest(headerMap map[string]string, request *http.Request) {
for header, contents := range headerMap {
request.Header.Add(HeaderPropagationStringForRequestLogging, header)
request.Header.Add(HeaderPropagationStringForRequestLogging, contents)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Could we pass the headers as is instead of creating a special header (httpheaderforwardingforlogging)?

@alanprot
Copy link
Member

alanprot commented Sep 1, 2022

Nice!!

@alanprot alanprot merged commit a795d53 into cortexproject:master Sep 1, 2022
@danielblando danielblando mentioned this pull request Oct 21, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP Header Logging
4 participants