Skip to content

Conversation

@carlosms
Copy link
Contributor

Part of #84.

While migrating lookout I noticed that we can't use pb.AddFields inside a client interceptor. This is because pb.DialContextWithInterceptors was executing Ctxlog*ClientInterceptor before the user provided ones.
With the change in this PR the interceptors are called in this order:

user client interceptors -> lookout-sdk client Ctxlog interceptor -> ...gRPC call...
-> lookout-sdk server Ctxlog interceptor -> user server interceptors

Instead of the previous one:

lookout-sdk client Ctxlog interceptor -> user client interceptors -> ...gRPC call...
-> lookout-sdk server Ctxlog interceptor -> user server interceptors

@se7entyse7en can you please check if we can have a similar problem in the python API?

Copy link
Contributor

@se7entyse7en se7entyse7en left a comment

Choose a reason for hiding this comment

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

👍 Thanks also for the tests!
I'm gonna open an issue for the Python part just to not forget about it, I'm gonna check it next to what I'm doing.

@se7entyse7en
Copy link
Contributor

I was thinking that actually the Wrapped context AFAIR is actually not available at all in the interceptors as the api exposes a different data structure. If we want to enable to user to handle log fields inside the interceptors also for the Python SDK I think that we need to open an issue and investigate how we can approach it.

@carlosms carlosms merged commit ec29958 into src-d:master Mar 22, 2019
@carlosms carlosms deleted the interceptors-order branch March 22, 2019 17:22
@se7entyse7en
Copy link
Contributor

I had a look and it is always possible to handle log fields inside client interceptors as it is handled differently. But it's not documented how to do it, it's just used for client logger interceptors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants