-
-
Notifications
You must be signed in to change notification settings - Fork 463
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
Set extra headers for outbound messaging as late as possible #600
Set extra headers for outbound messaging as late as possible #600
Conversation
Allows for in-entrypoint context data updates.
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.
Thanks @iky.
I think this same change needs applying to the Publisher
and EventDispatcher
, because this same issue is in messaging.py
:
Lines 116 to 124 in b0131f5
def get_dependency(self, worker_ctx): | |
extra_headers = encode_to_headers(worker_ctx.context_data) | |
def publish(msg, **kwargs): | |
self.publisher.publish( | |
msg, extra_headers=extra_headers, **kwargs | |
) | |
return publish |
@mattbennett Is this the same problem from here? https://github.com/nameko/nameko/pull/573/files |
Gah, yes it is! Sorry @timbu, I completely forgot about that PR. Interesting that this was introduced for the non-RPC extensions before the big refactor that landed in 3.x. |
I guess we should land #573 and then bring master into the v3 branch. |
Ah, did not see that. @timbu @mattbennett It's not just Publisher of messaging module, EventDispatcher of events module needs to be changed too, see 5c416ce |
5c416ce
to
8ed4356
Compare
I've updated the remaining places. The fixes are tiny, maybe we can merge this to 3.0.0 before #573 lands in master and then dealing with merging the two together? |
Ah, quite right. And that means #573 is missing the equivalent fix for the event dispatcher. I agree, let's make the fixes here, then #573 can land, then we'll merge master into the v3rc branch. |
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.
Just one small thing
@@ -507,10 +504,12 @@ class Client(object): | |||
""" | |||
|
|||
def __init__( | |||
self, publish, register_for_reply, service_name=None, method_name=None | |||
self, publish, register_for_reply, context_data, |
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.
Might want to add context_data
to :Parameters:
doc above.
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.
good catch, thanks! .)
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.
Awesome!
👍 |
Allows for in-entrypoint context data updates. This is useful when setting a language for example:
Another use we have is services signing up outbound calls with its own authentication.
I removed the lazy load and caching of
call_id
andcall_id_stack
of worker context. These properties are super-cheap and most of the time loaded anyway (if entrypoint contains an outbound call, or in logging debug call_id_stack is logged by the container, if tracer is on and so on...)