Update Flask middleware to not use full URL with query params as a span name #725
Conversation
part of the span name. Including query strings means that the span can potentially contain sensitive data (a lot of times query params can contain things such as API keys, etc).
Kami
force-pushed
the
flask_plugin_dont_include_query_params_in_name
branch
from
35d1bd8 to
8d94791
Compare
|
I've added a test case and a changelog entry. If others think it would be reasonable to add a config option (which defaults to Either as part of this PR or as a separate PR once this one is approved and merged. |
Kami
force-pushed
the
flask_plugin_dont_include_query_params_in_name
branch
from
8d94791 to
e383d22
Compare
| tracer.add_attribute_to_current_span( | ||
| HTTP_METHOD, flask.request.method) | ||
| tracer.add_attribute_to_current_span( | ||
| HTTP_URL, str(flask.request.url)) | ||
| HTTP_URL, str(flask.request.base_url)) |
There was a problem hiding this comment.
Using method + " " + base_url as span name looks good.
Using base_url in HTTP span attribute is against the specification.
I suggest creating a GitHub issue in the spec repo and send PR to update the specification, then implement the spec in Python SDK.
There was a problem hiding this comment.
Thanks, I will update the code so it will follow the specification.
There was a problem hiding this comment.
Awesome! FYI we're also open to making the spec better (including privacy & security concerns), any recommendations/ideas are very welcomed.
There was a problem hiding this comment.
Thanks for the contribution!
There is a balance of the data verbosity versus privacy.
The proposed change on HTTP span attribute is against the current specification, need to sort it out in the spec first.
The changes to span name looks fine though. I'd suggest that we proceed with the span change and revert the http.url change for now.
|
You are welcome. Here is the revert - 55447e2. I will submit feedback against specification. Thanks. |
Great! And @Kami you're very welcomed to help us to design/review the OpenTelemetry APIs. You can read more from https://opentelemetry.io, in a nutshell OpenCensus and OpenTracing are now merging into the same OpenTelemetry project under CNCF, and we'll be porting the majority of extension functionalities to OpenTelemetry. |
|
@Kami please help to rebase, thanks! |
|
This one has been superseded by #746. |
This pull request updates Flask middleware so it doesn't use full URL with query params as a spam name.
A lot of times query parameters can contain sensitive data (things such as API keys etc, since not all the services support sending such information via headers so query params are often used as a fallback in such scenarios).
We could perhaps add config option to include query parameters as an attribute with an option for blacklisted attributes (this does increase the complexity of the code and adds additional processing time and overhead to the middleware though).
TODO