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

[HttpClientFactory] Do not log query string by default #103769

Merged
merged 21 commits into from
Jul 4, 2024

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Jun 20, 2024

Since query strings often contain sensitive information, we decided to avoid including them in HttpClientFactory logs by default. For use-cases where logging query string is necessary and safe, query string logging can be turned on globally by an AppContext switch System.Net.Http.DisableUriRedaction .

In .NET 10 we plan to support more sophisticated redaction scenarios.

@antonfirsov antonfirsov added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. area-Extensions-HttpClientFactory needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Jun 20, 2024
@antonfirsov antonfirsov added this to the 9.0.0 milestone Jun 20, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Copy link
Member

@CarnaViire CarnaViire left a comment

Choose a reason for hiding this comment

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

See comments above

@stephentoub
Copy link
Member

We don't want to expose a programmatic option for this on HttpClientFactoryOptions or somewhere similar?

@antonfirsov
Copy link
Member Author

antonfirsov commented Jun 21, 2024

We don't want to expose a programmatic option for this on HttpClientFactoryOptions or somewhere similar?

We weren't able to agree on an API for this an related requests, like support for url.template, Activity enrichment and filtering.

Many customers (including our first-parties) would find the usability of a HttpClientFactory-scoped Uri-redaction API limited. In order to deliver a consistent solution for all the requests in the telemetry space, we decided to push this out to .NET 10.

@antonfirsov antonfirsov removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 1, 2024
- Use a shared switch to opt-out from redaction entirely (System.Net.Http.DisableUriRedaction)
- Simplify and optimize redaction logic, always omit Fragment when redacting
- Adjust and consolidate tests
@antonfirsov antonfirsov marked this pull request as ready for review July 2, 2024 20:58
@antonfirsov
Copy link
Member Author

antonfirsov commented Jul 2, 2024

@CarnaViire @MihaZupan with e557330, this is ready for review again.

@antonfirsov antonfirsov removed their assignment Jul 2, 2024
@antonfirsov antonfirsov assigned antonfirsov and unassigned MihaZupan Jul 2, 2024
Copy link
Member

@CarnaViire CarnaViire left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@antonfirsov
Copy link
Member Author

The LibraryImportGenerator.Tests failure is unrelated.

@antonfirsov antonfirsov merged commit 64efe26 into dotnet:main Jul 4, 2024
81 of 83 checks passed
antonfirsov added a commit that referenced this pull request Jul 10, 2024
Add the following standard tags to the HTTP Request Activities started in DelegatingHandler:

http.request.method
http.request.method_original
server.address
server.port
url.full

error.type
http.response.status_code
network.protocol.version

Just like in #103769, url.full is being redacted by removing UserInfo and the query string, while exposing a System.Net.Http.DisableQueryRedaction switch for opting-out from the latter.
@github-actions github-actions bot locked and limited conversation to collaborators Aug 4, 2024
@liveans liveans self-assigned this Oct 1, 2024
@liveans
Copy link
Member

liveans commented Oct 7, 2024

Breaking change issue is created for this: dotnet/docs#42792

@liveans liveans removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-HttpClientFactory breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants