Add remove log context field functionality

[anaghsys]

[praveensanap]

context from @anaghsys Its a complement to AddToLogContext but which allows the key to be present only for a certain portion of processing. Ie. if a particular key like order_id is desired in the logs, I wish to log it only when the code enters a processing path where the order_id is available, and remove it after that. Without this func, all keys once added to context will remain forever and always be logged. This allows us to remove a log key from being printed if its not required in the code path.

[praveensanap]

For printing custom things we can always directly print it using log.Info(), log.Debug() etc. because AddToContext and such functions should be used inside library function like Interceptors etc. surfacing these functions directly in request path would create a dependency to the framework libraries, might cause problems in upgrading. Also I feel we should add only special fields in log context that is support by platform for all services.

[anaghsys]

AddToContext and such functions should be used inside library function like Interceptors etc.

We can say should be, but we obviously can't enforce it. To allow us to modify existing code which doesn't follow the should be strategy, this func will help us clean up the logs piecemeal (ie. modify individual code paths without affecting the overall logging structure).

surfacing these functions directly in request path would create a dependency to the framework libraries

If, as you say, it should be used inside library functions, and those library functions are called directly in the request path due to the use of the interceptors, then we already have a dependency - its just not visible immediately. We should remember that hidden dependencies do not not imply no dependencies.

Also I feel we should add only special fields in log context that is support by platform for all services.

This seems like a comment directed at the use of this function, not at the purpose of this function. With this function, as with the already present AddToLogContext as of date, we cannot enforce an allowlist of "special fields" that are able to be logged.

Such behavior would make these logging functions too restrictive and inflexible. The decision about which field to log should not be made by the library/framework - it should provide a method to do so, but the actual field itself should be decided by a service owner maintaining their service.

For printing custom things we can always directly print it using log.Info(), log.Debug() etc.

Agree with this, and this is the happy path. But if we have attributes that must be available in many, many, log statements, then this strategy is cumbersome and lends itself to a lot of repetition. It is exactly this problem which these functions solve.

[ankurs]

https://github.com/carousell/Orion/pull/118#issuecomment-1201974988

I agree with @anaghsys

[achichen]

For printing custom things we can always directly print it using log.Info(), log.Debug() etc.

This is definitely verbose. We have similar requirement as @anaghsys that we need some information to be present in the log in a process.

because AddToContext and such functions should be used inside library function like Interceptors etc. surfacing these functions directly in request path would create a dependency to the framework libraries, might cause problems in upgrading.

Just to raise a use case: now the only way to add named key-value data to notified sentry log is via AddToLogContext. This is quite common and useful in our team's code base.

Also I feel we should add only special fields in log context that is support by platform for all services.

@praveensanap could you elaborate more on this?

It seems the debate is between whether log context is a utility for application or specific for platform. @praveensanap could you share your concern on "open it" to application usage?