Extract gRPC interceptors from lookout

[carlosms]

Lookout contains two gRPC interceptors in the grpchelper package, see NewServer and DialContext in helper.go.

The Ctxlog interceptor requires that the analyzers also have these interceptors to pass around the log fields like this: lookoutd client -> analyzer server -> analyzer client -> lookoutd server.

If we move the Ctxtlog interceptor to this repo it probably makes sense to move also the Log interceptor, to have uniform logs for lookoutd and all the analyzers.

I'm not sure about extracting the util/ctxlog package to this repo. Maybe we just want to have methods to set and get map[string]interface{} in the context, using context.WithValue(), ctx.Value().

Ideally the python code should have the same functionality to get the log fields from the gRPC metadata.

[se7entyse7en]

@carlosms

The Ctxlog interceptor requires that the analyzers also have these interceptors to pass around the log fields like this: lookoutd client -> analyzer server -> analyzer client -> lookoutd server.

Could you please elaborate a bit more? Having a wider picture would help a lot.

So basically the idea is to move the followings from lookout repo to lookout-sdk repo:

• CtxlogStreamServerInterceptor
• CtxlogUnaryServerInterceptor
• CtxlogStreamClientInterceptor
• CtxlogUnaryClientInterceptor
• LogStreamServerInterceptor
• LogUnaryServerInterceptor
• LogStreamClientInterceptor
• LogUnaryClientInterceptor

right?

And also to add some utility functions to get/set map[string]interface{} in contexts.

Ideally the python code should have the same functionality to get the log fields from the gRPC metadata.

Maybe we could add a separate issue for this.

[carlosms]

The Ctxlog interceptor requires that the analyzers also have these interceptors to pass around the log fields like this: lookoutd client -> analyzer server -> analyzer client -> lookoutd server.

Could you please elaborate a bit more? Having a wider picture would help a lot.

Yes, see src-d/lookout#248.

Basically, in lookout we add log fields to every log entry, like event-id, repo, github.pr.
But there are actions that come from a gRPC request (like GetChanges), and we didn't have those log fields. So we had log entries like "fetching references" or whatever that did not give any hint as to what PR triggered that initially, or what analyzer initialized the gRPC request.

This was solved in src-d/lookout#359, but that code requires collaboration from the analzyer. The flow of the log-fields is like this:
lookoutd client -> analyzer server -> analyzer client -> lookoutd server.

The lookout client will always send the log fields, and the lookout server will always take the log fields if they are present. But the analyzer code needs to get the fields from the incoming gRPC requests (NotifyReview/Push), and send them in their requests to lookout (GetChanges/GetFiles).

So basically the idea is to move the followings from lookout repo to lookout-sdk repo:

• CtxlogStreamServerInterceptor
• CtxlogUnaryServerInterceptor
• CtxlogStreamClientInterceptor
• CtxlogUnaryClientInterceptor
• LogStreamServerInterceptor
• LogUnaryServerInterceptor
• LogStreamClientInterceptor
• LogUnaryClientInterceptor

right?

And also to add some utility functions to get/set map[string]interface{} in contexts.

Yes. As I said, I'm not sure about moving the ctxlog package here too. It would make sense to standardize the logs in all the Go analyzers (using go-log), but maybe it's too much bloat for an SDK library. And then we would need to find a similar functionality for python... So I think for now it's best to make those utilities to get/set should deal with map[string]interface{}, not Logger.

Ideally the python code should have the same functionality to get the log fields from the gRPC metadata.

Maybe we could add a separate issue for this.

I don't mind having 2 PRs for this issue, or to open a new issue for python. But I think it's important to keep both languages on par, if possible.

[smacker]

I totally agree with Carlos on using map[string]interface{} in sdk instead of bringing full ctxlog.
At least for now.

[se7entyse7en]

So if I understood correctly the main concern about bringing here ctxlog is actually the fact that it uses go-log, right? Coz if this is the case we could just move ctxlog here by removing the go-log dependency, and currently, it only uses the log.Fields alias and the log.New factory.

And if we're not using go-log here, should I use the stdlib log for logging?

[smacker]

the main concern about bringing here

I think that in the current state sdk shouldn't provide any solution for logging. Each analyzer can use any library for that. To do logging "right" is a difficult problem especially for a library that will be used by many different applications.

And if we're not using go-log here, should I use the stdlib log for logging?

sdk shouldn't log anything (for now). It should just extract log fields from lookout request and send it back when analyzer calls lookout. And maybe provide some simple function that returns map so an analyzer can include it to its logging solution. (but include or not is up to analyzer)

[se7entyse7en]

I'm gonna mimic what has been done in #77 for python. No need to open new issue, this is going to be closed once Go and Python SDK are aligned.

[se7entyse7en]

Moving this to TODO until I'm actively working on it again.