-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Refresh EndpointConfig
request headers
#12904
Conversation
EndpointConfig
request headers
🚀 A preview of the docs have been deployed at the following URL: https://12904--rasahq-docs-rasa-v2.netlify.app/docs/rasa |
@@ -149,6 +149,9 @@ async def request( | |||
headers.update(kwargs["headers"]) |
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.
Will the changes done to the headers here be "undone" by your changes?
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.
There is a possibility that the similar keys get overwritten here. Do you have any ideas what can be done to minimise this risk, perhaps only add the tracing related header key?
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.
It depends what could be in the self.headers
, in the tracing scenarios it only contains the traceparent
key-value pair. I've had a look at where this EndpointConfig.request()
method is used in the codebase, and only in the rasa interactive
code it is used in two places with headers
keyword argument with value {"Accept": "application/json"}
, let me know if you're interested and I can share the links.
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.
only add the tracing related header key
yep sure, we can restrict to this only, however I think the risk is low for overwriting keys based on the code usage I could find of headers
keyword argument (see reply above). Let me know before making the commit if I should go ahead?
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.
If you've looked into it already then it's cool. I'm good with it
Proposed changes:
EndpointConfig
using itsheaders
attribute.rasa-plus
to the action server.Status (please check what you already did):
black
(please check Readme for instructions)