-
Notifications
You must be signed in to change notification settings - Fork 741
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
Readme for Microsoft.Extensions.Http.Diagnostics #4674
Conversation
|
||
These components enable enriching and redacting HTTP Client request logs. They remove built-it HTTP Client logging. | ||
|
||
In order to use the redaction feature, you need to reference the `Microsoft.Extensions.Compliance.Redaction` package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is non-trivial and we need to improve discoverability of this (reference redaction package in Http.Diagnostics
package?). I will create a work item to track that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #4675
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think we should reference the Redaction package automatically. Even when it is not used directly, the idea would be for folks referencing this package to be able to look at your sample code from the README and use it, but if we don't include the package reference they would get an error in the call to AddRedaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, I agree with you we can fix this in a separate change.
builder.Services.AddRedaction(); | ||
|
||
// Register latency context services: | ||
builder.Services.AddLatencyContext(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was also non-trivial as DI simply throws an exception if I forget to add this line. We definitely need to improve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracked under #4675
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a very small comment but this looks great otherwise. Thanks so much for taking care of this @xakep139
Co-authored-by: Chris Ross <Tratcher@Outlook.com>
@joperezr do we still merge PRs into |
null
Microsoft Reviewers: Open in CodeFlow