-
Notifications
You must be signed in to change notification settings - Fork 10.3k
HeaderPropagation: reworded registration exception for clarity #12636
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
HeaderPropagation: reworded registration exception for clarity #12636
Conversation
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.
Wordsmithing! Would like @rynowak to confirm all is in the right place (and my words are good), but otherwise LGTM
$"initialized. Register the header propagation middleware by adding 'app.{nameof(HeaderPropagationApplicationBuilderExtensions.UseHeaderPropagation)}() " + | ||
$"in the 'Configure(...)' method."; | ||
$"initialized. Register the header propagation middleware by adding 'app.{nameof(HeaderPropagationApplicationBuilderExtensions.UseHeaderPropagation)}()' " + | ||
$"in the 'Configure(...)' method. Also, do not use an HttpClient with header propagation outside of an incoming HTTP request."; |
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.
$"in the 'Configure(...)' method. Also, do not use an HttpClient with header propagation outside of an incoming HTTP request."; | |
$"in the 'Configure(...)' method. Header propagation can only be used within the context of an HTTP request."; |
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.
Updated
5c249ec
to
ea8de36
Compare
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.
Looks good! I'll take a look at the CI failures. They may not be related.
Yeah, looks like a timeout. Re-running. |
CI looks good now |
Thanks @alefranz |
The MessageHandler throws when used outside of a request.
This reword the exception to clarify that this could be another possible cause of it.
Also fixes a missing
'
in the current message.Related to #12169, but not a final fix
@rynowak I have a couple questions:
HttpContextAccessor
to distinguish between the two types of errors and have a specific message, but this can add a performance hit if not already registered. Unless we use it only if it is available and fallback to this generic exception message otherwise. However adding a dependency only to have a better error message feels wrong./cc @anurse