This is already added by pb.NewServerWithInterceptors. The only difference is that in the implementation here on lookout, we pop the "app" key. Why is it done? Maybe we need to expose an api to set which keys to exclude or something like that?

does it mean that with this code we call both CtxlogStreamServerInterceptor from sdk and from lookout?

Yup, I guess that it is actually called twice, but I haven't run it.

They look similar, but both the sdk pb.Ctxlog*Interceptor and lookout Ctxlog*Interceptor are needed.

pb.CtxlogStreamClientInterceptor takes the log fields from ctx.Value(logFieldsKeyContext), but the value for logFieldsKeyContext needs to be populated first by the lookout CtxlogStreamClientInterceptor, calling pb.AddLogFields.

Similarly, pb.CtxlogStreamServerInterceptor takes the log fields and stores them into the context with the key pb.logFieldsKeyContext. This value is now accesible with pb.GetLogFields, but the rest of the lookout code assumes that the fields are accessed with the lookout ctxlog.Get() function. So the lookout CtxlogStreamServerInterceptor is needed to copy values from the sdk pb.logFieldsKeyContext to lookout ctxlog.logFieldsKey.

About removing the app key, it is a convention we use in our deployments. Each application adds a log field app=appname. This way we can filter messages easily. The interceptors here remove them to avoid having a message from an analyzer contain that app=lookoutd field.

pb.CtxlogStreamClientInterceptor takes the log fields from ctx.Value(logFieldsKeyContext), but the value for logFieldsKeyContext needs to be populated first by the lookout CtxlogStreamClientInterceptor, calling pb.AddLogFields

But aren't you here actually reading the log fields from the same place you're adding here?

I mean ctxlog.Fields(ctx) reads from const logFieldsKey ctxKey = 0 and pb.AddLogFields is adding them in const logFieldsKeyContext ctxKey = 0.

They are actually 2 different types, to avoid collisions. This is the recommended way to define the keys in the context docs.

I guess we could export pb.logFieldsKeyContext and use it in lookout ctxlog, and remove the extra interceptors. But then the app field would jump over to the analyzer logs...

ping @se7entyse7en

@smacker thanks for pinging.

Sorry for delaying this 😞, I forgot that this has a pending discussion.

Instead of exporting pb.logFieldsKeyContext isn't it possible to directly use the ctxlog package of lookout with the utilities in pb.context.go? By doing this we're able to put the log fields directly with the "right" key, and we can remove the extra interceptor.

For sure even if we do this right now, we won't be able to have the same behavior as we're not able to remove the app key. But if we can get rid of the ctxlog package in favor of pb.context.go, I'd then open an issue on lookout-sdk to think about a way to filter out logfields keys.

WDYT?

We can't remove ctxlog completely. We need Get(ctx context.Context) log.Logger and WithLogFields(ctx context.Context, fields log.Fields) (context.Context, log.Logger). But I agree it would be better if we could re-use sdk/pb/context.go there.

At least it worth to try and if there are some big issues that I we don't see right now, we can merge this PR as it is and create a separate issue. But on the first glance, it should be easy to update.

I also agree that app field filtering better put in sdk. It's convention we use and better to enforce it on sdk level.

we can merge this PR as it is and create a separate issue.

Sure, I'd open a separate issue for that. I'm gonna open an issue about trying to reuse sdk/pb/context.go

I also agree that app field filtering better put in sdk. It's convention we use and better to enforce it on sdk level.

Does this convention applies to each dependant on lookout-sdk? I mean are we going on lookout-sdk to blindly remove the app field or generically provide a way to filter whatever fields a dependant would like to remove? I'm opening an issue for this too.

This also is already added.

Same here

leaving this as a reminder before merging