feat: consistent HTTP Request attributes with utility #769
Conversation
chrisholmes
requested review from
ahayworth,
arielvalentin,
dazuma,
ericmustin,
fbogsany,
mwear and
robertlaurin
as code owners
chrisholmes
changed the title
chrisholmes
changed the title
Instrumentation for HTTP clients that use this utility will now add consistent attributes for HTTP requests. The utility also has an option to hide query parameters so that they don't show up in these attributes. Query parameters often hold sensitive information, which some teams may not want to log.
chrisholmes
force-pushed
the
hide_http_client_params
branch
from
40c4da6
to
7b44979
Compare
...mentation/faraday/lib/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware.rb
Show resolved
Hide resolved
| end | ||
|
|
||
| def hide_query_params(uri) | ||
| uri.query = '' if uri.query |
There was a problem hiding this comment.
Does mutating the uri affect the callers of from_request? Should this dup the uri first?
There was a problem hiding this comment.
I haven't seen anything like it in tests, but I don't want to leave it to chance so I'll make the change.
| extend self | ||
|
|
||
| def from_request(request_method, uri, config = {}) | ||
| uri = hide_query_params(uri) if config[:hide_query_params] |
There was a problem hiding this comment.
I think there are a lot of ways to go with this implementation under the hood but overall I like this config option and think it's a reasonable thing to add. Generally speaking though, for http client helpers, it's an interesting dilemma.
On one hand, you could add more config for more use cases with a middle ground approach here to add some or all query parameters as a span attribute. hide_query_params could optionally take an allow-llst maybe, or if changed to show_query_params could look something like:
require `cgi`
def show_query_params(uri, span)
if config[:show_query_params] && config[:show_query_params].length > 0
return uri if config[:show_query_params][0] == 'all'
params = CGI::parse(uri.query)
config[:show_query_params].each do |query_param|
if params[query_param]
span.set_attribute("request.#{query_param}", params[query_param])
end
end
end
uri.query = '' if uri.query
uri
end
But, maybe the ideal end state is to allow users an arbitrary proc on the span. from_request could have a similar method like after_request and users can do whatever they want to their spans.
def after_request(span, request_method, uri, config = {})
if config[:after_request]
after_request = config[:after_request]
after_request.call(span, request_method,uri,config)
end
span
end
Anyway, just something to think about for future work but this is a great improvement so it'll be great to see this merged and available.
...ion/faraday/test/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware_test.rb
Show resolved
Hide resolved
| module Common | ||
| module HTTP | ||
| # RequestAttributes contains common helpers for setting http client attributes in spans | ||
| module RequestAttributes |
There was a problem hiding this comment.
This should be required in after client_context here, i believe
There was a problem hiding this comment.
A couple smaller changes and feedback, but overall I think this is pretty close, i'd like to make sure we reach consensus around the config name and functionality and such if anyone else has thoughts.
Also is there a specific document or comment you were referring to regarding posture around sensitive information? I was wondering this myself. i'd certainly prefer off by default for the query string.
Nice work here!
|
@ericmustin, I was referring to the the comments made here rather than a specific doc: #737 (comment) |
|
We discussed this on the weekly SIG call yesterday. These helpers don't belong in We would rather build an Additionally, we should think about common instrumentation options and how to ensure that related instrumentation uses the same option names, validations and defaults. Factoring those options out into modules that can be included in, e.g., HTTP client instrumentation, would be very useful. |
@fbogsany, this all sounds very sensible. I definitely would not have chosen to add to
Yes, there is definitely space for more reuse and abstractions here, but maybe too much for a single PR? Would it help for me to join SIG to discuss the scope of this next week? |
Sure, that's a great idea - thanks! |
|
Hello, and thank you for your contribution! We recently split Ruby instrumentation out into the Portions of this PR are related to instrumentation gems, so we'll need you to re-open it against To do that, you can:
Sorry again for the inconvenience, and thank you for contributing! |
|
👋 Hi, @chrisholmes! Thank you for your PR! As Sam mentioned, Ruby instrumentation now lives in the opentelemetry-ruby-contrib repo. We'd be happy to take another look at your work in that context. Since most of this PR is instrumentation-related, I'm going to close it. We appreciate your contribution and hope to work with you again soon. |
This change introduces a utility that can be used by instrumentation to add consistent attributes for HTTP requests. Currently, HTTP attributes are set inconsistently across clients, which can be a bit of a headache if the services you're tracing have multiple HTTP client dependencies.
The utility also has an option to hide query parameters so that they don't show up in these attributes. Query parameters often hold sensitive information, which some (hopefully all 😅 ) teams may not want to log. I've made this on by default to align other recent changes to handling sensitive information.
I've applied this utility to the
net/http,faraday, andhttpinstrumentation, but I'd also want to apply this to all the other http clients in the repository.