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

Add request and response header tags to Rack #389

Merged

Conversation

delner
Copy link
Contributor

@delner delner commented Apr 3, 2018

This pull requests allows users to define which HTTP headers appear as tags on their rack.request span.

To do so, add the following option to your configuration:

Datadog.configure do |c|
  # Add this request and response header as tags
  c.use :rack, headers: { request: ['Accept-Encoding'], response: ['Content-Encoding'] }
end

Will result in the following tags:

  • http.request.headers.accept_encoding
  • http.response.headers.content_encoding

By default, :headers is set to { response: ['Content-Type', 'X-Request-ID'] }.

This pull request also removes http.request_id in favor of http.request.headers.x_request_id and http.response.headers.x_request_id.

@delner delner added the integrations Involves tracing integrations label Apr 3, 2018
@delner delner self-assigned this Apr 3, 2018
@delner delner force-pushed the feature/add_request_and_response_header_tags_to_rack branch 2 times, most recently from 0aec286 to 4ccdcdb Compare April 3, 2018 19:04
@delner delner added this to the 0.13.0 milestone Apr 5, 2018
@delner delner changed the base branch from 0.12-dev to 0.13-dev April 13, 2018 17:31
@delner delner force-pushed the feature/add_request_and_response_header_tags_to_rack branch from 4ccdcdb to 1047403 Compare April 13, 2018 20:13
Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

I'd say to refactor the integration a bit so that it's generic and configurable for collecting headers.


# Constants for request headers
module RequestHeaders
CACHE_CONTROL = 'http.request.headers.cache_control'.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed, probably it's better to make it configurable as an option (easy accessible from rack and rails and sinatra) so that users can define the list of headers they want to collect (even custom ones).

Then we have to pick the right defaults, so I'd advocate to collect what we're collecting right now (e.g. request_id) and then double check with our security team for the rest.

The Agent, still has the capability to redact / obfuscate automatically collected headers and is configurable as defined in DataDog/datadog-trace-agent#398.

Sounds good @delner ?

@delner delner force-pushed the feature/add_request_and_response_header_tags_to_rack branch 2 times, most recently from 4a7c12c to e711573 Compare April 16, 2018 19:23
{}.tap do |result|
env.each do |name, value|
# Normalize the value, check if it's permitted.
tag = Datadog::Ext::HTTP::Headers.to_tag(name.to_s.sub('HTTP_', ''))
Copy link
Contributor

Choose a reason for hiding this comment

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

let's do it the other way around; iterate over the whitelist instead (that is smaller) instead of the headers list that is in the order of thousands of elements

@delner delner force-pushed the feature/add_request_and_response_header_tags_to_rack branch from e711573 to 12da569 Compare April 16, 2018 20:49
@delner delner force-pushed the feature/add_request_and_response_header_tags_to_rack branch from 3f18f73 to a8a6f8a Compare April 16, 2018 21:24
@delner
Copy link
Contributor Author

delner commented Apr 16, 2018

@palazzem Should we have separate lists for request and response headers? e.g. headers: { request: ['Accept-Encoding'], response: ['Content-Encoding'] }

@palazzem
Copy link
Contributor

@delner I think so, probably it's way better so we differentiate a possible "clash" between request and response

@palazzem palazzem merged commit f4631ea into 0.13-dev Apr 17, 2018
@palazzem palazzem deleted the feature/add_request_and_response_header_tags_to_rack branch April 17, 2018 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants