-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support for logging contextual metadata #654
Comments
@akshayjshah @prashantv please let me know if this is an acceptable direction, and I can follow up with full PRs. |
Rather than adding new types which are essentially duplicates of the current interface, I think I'd prefer a new smaller type -- it accepts a context and returns the existing logger types: type ContextLogger struct{...}
func (*ContextLogger) Ctx(context.Context) zap.Logger We could have a duplicate that returns a It also allows the user to select when to pay the cost of serializing the context, rather than being forced to pay it at every log call. // if you only log a couple of times, you may want to take the cost of context serialization each time
ctxLogger.Ctx(ctx).Info(...)
ctxLogger.Ctx(ctx).Info(...)
// if you're logging many times, you can cache the serialized context in the logger
logger := ctxLogger.Ctx(ctx)
logger.Info(..)
logger.Info(..)
logger.Info(..)
logger.Warn(..)
logger.Error(..) We can still add lint rules to ensure that if there's a The |
@prashantv thanks. What you're suggesting is very similar to what I did in HotROD demo app long ago - https://github.com/jaegertracing/jaeger/blob/master/examples/hotrod/pkg/log/factory.go#L44 However, in HotROD we could also write the logs to the Span found in the Context, so that the logs could be viewed in Jaeger UI inside the actual trace. If |
@prashantv another downside of the |
I'd also prefer a smaller interface, like the one Prashant suggests (I believe we discussed this exact API in some Google Docs comments ages ago). I'm not sure we need a sugared variant; The implementation of this feature shouldn't require touching encodings or anything in the
The default
Definitely true of the single logger allocation. Any cost associated with creating more complex fields (simple fields stay on the stack) will have to paid either way - at least this approach allows us to pay that cost once, instead of once at each log site. On balance, I think that the single allocation is worth it.
I'd like this too - providing a single entry point to these two logging systems is a great idea. However, it's a bit more complex. There are at least three options to do this: One option is to implement the
We'd use the Alternatively, we can implement the The third (and to me, the best) option is to have our log-tailing agent send all messages that include a span and trace ID to the Jaeger agent, which can then treat them just like messages sent via the in-memory OpenTracing APIs. That's probably a bit more complex, but it solves this problem for all languages and logging libraries, and I think it'd also make it easier for other companies to get value out of Jaeger's logging functionality. |
I have some interesting to this issue. I hope to all |
I don't understand why this needs to be integrated into I'm currently working on a feature that shares requirements with what we're after here, but it made the most sense to have it implemented specific to my application which has its own specific metadata keys. On the other hand, I suppose you'd be adding a If so, that could allow other thirdparty libraries the opportunity to add their zap fields to the context. |
I'm definitely finding late this issue, although its the same problem for me. First of all (thats more for other people discovering this issue) - if You're using elastic apm for tracing, there is builtin functionality for zap logs - https://www.elastic.co/guide/en/apm/agent/go/master/builtin-modules.html#builtin-modules-apmzap But in general, I think what would've been great - is to have Having context for entry will allow to extract any required information from context either with custom encoder or with hook without any major break. For example |
I just found this issue after creating #898 but the ideas of having a generic way to set the context and then having some info extracting automatically stills holds. |
It is possible to extend Zap as follows:
This is how zap was used internally at Uber (when I was there), but unfortunately the extension code was not in the OSS. It was all done in additional modules, so modifying Zap itself was not necessary. |
@marcellodesales we ended up generating a wrapper around the methods which do the real work, logging and span attributes are there. Not really satisfying but it works. |
@schmurfy any reference implementation maybe would be a great start for the community :) As we see more and more users interested in this feature... |
Here's what I was thinking in terms of adding custom fields with every log request. It uses a custom zapcore.Core to wrap the ioCore and adds a field of your choosing with each log request. Your custom logic in Write could grab the metadata out of the context or any other functionality of your choosing. Your custom logic would also be responsible for any kind of caching so as not to slow down every log request. Cons to this approach would be serializing the added Fields with each log request. In my own use case I don't have a clean way of instantiating a "context logger" at the top of each request stack and using that throughout the application without breaking our DI pattern. Incurring the serialization hit in our case seemed worth it to be able to add the single WrapCore and then not have to worry about those added fields anywhere else. Thoughts? https://gist.github.com/jejikenwogu/2e605e467e188bba42707be7709d4bdb |
I believe just adding a It may not be ideal, but probably the best option is to have something like "InfoC(ctx context.Context, ...)" and all the equivalents. |
how about just add package zapctx
import (
"context"
"go.uber.org/zap"
)
type (
contextKey struct{}
)
func With(ctx context.Context, log *zap.Logger) *zap.Logger {
fs, _ := ctx.Value(contextKey{}).([]zap.Field)
return log.With(fs...)
}
func Add(ctx context.Context, fields ...zap.Field) context.Context {
if len(fields) == 0 {
return ctx
}
fs, _ := ctx.Value(contextKey{}).([]zap.Field)
return context.WithValue(ctx, contextKey{},
append(fs, fields...),
)
}
func Set(ctx context.Context, fields ...zap.Field) context.Context {
// replace existing fields
} |
Same problem you force a call to
Try to map your solution to this. |
another approach package main
import (
"context"
"go.uber.org/zap"
"go.uber.org/zap/zapcore"
)
type Valuer interface {
Value(key interface{}) interface{}
}
func Context(key string, value Valuer) zap.Field {
return zap.Object(key, valuer{value})
}
type (
valuer struct {
Valuer
}
contextKey struct{}
)
func (v valuer) MarshalLogObject(enc zapcore.ObjectEncoder) error {
fs, _ := v.Value(contextKey{}).([]zap.Field)
for i := range fs {
fs[i].AddTo(enc)
}
return nil
}
func main() {
log, _ := zap.NewDevelopment()
ctx := context.WithValue(context.Background(),
contextKey{}, []zap.Field{
zap.String("foo", "from context"),
zap.String("bar", "from context"),
},
)
log.Info("message",
zap.String("baz", "explicit"),
//zap.Context("context", ctx),
Context("context", ctx),
)
// output: {"baz": "explicit", "context": {"foo": "from context", "bar": "from context"}}
} |
also there is |
@fxrlv indeed this idea seems tempting, but is not as clean as passing the Context as the first argument per "golang" recommendation. It is a bit of an anti-pattern to see the "Context" in the middle or at the end of a function call but seems that is the only option zap authors are willing to give. Also it seems that the authors of this project do not really care about users asks, sorry but I have to say this since 40 "+1"s are ignored and no real solution is giving. |
iirc having |
So, for 2022, what do you believe it is the best approach (performance wise) to add some fields at the beginning of the flow of a request? Creating a new suggarLog withFields when creating a new context and store the logger in the context? which basically that this package does: I saw the package https://pkg.go.dev/github.com/juju/zaputil/zapctx or more simpler: https://github.com/northwesternmutual/kanali/blob/master/pkg/log/log.go Or is it out there something better? For my case we just have REST incoming request which do several operation with RabbitMQ,DB,Caches, calling other external services, etc... and we want to track the whole flow. |
const alreadyAdded = this.loggers.has(id) why this error |
this is not a good way, because most of them time you need to get some context information from context and in the construction ( New) you usually ctx has not been passed. |
@willthrom this is an example to store specific keys in the context and use it in the logger. const loggerKey = "my-logger"
// Set zap.Logger as variable in context
func Set(ctx context.Context, logger *zap.Logger) context.Context {
setup := logger
fields := make([]zap.Field, 0)
for _, key := range []string{local.TraceID, local.SpanID, local.ParentSpanID} {
if value, ok := ctx.Value(key).(string); ok {
fields = append(fields, zap.String(key, value))
}
}
if len(fields) > 0 {
setup = setup.With(fields...)
}
return context.WithValue(ctx, loggerKey, setup)
}
// Get zap.Logger from context
func Get(ctx context.Context) *zap.Logger {
if logger, ok := ctx.Value(loggerKey).(*zap.Logger); ok {
return logger
}
return zap.L()
} |
I've been using a pattern where I only store fields in the context, and have have a function that will decorate a logger with the fields of in the context: type logKeyType struct{}
// Context stores a list of zap.Field in a context.
// If the parent context had any zap.Field's they will be kept.
func Context(ctx context.Context, fields ...zap.Field) context.Context {
old := GetFields(ctx)
fields = append(old, fields...)
return context.WithValue(ctx, logKeyType{}, fields)
}
// GetFields returns the zap.Field's stored in the context or nil if none are found.
func GetFields(ctx context.Context) []zap.Field {
fields, ok := ctx.Value(logKeyType{}).([]zap.Field)
if !ok {
return nil
}
return fields
}
// WithContext gets the zap.Field's from a context and adds them to an existing logger.
func WithContext(ctx context.Context, log *zap.Logger) *zap.Logger {
return log.With(GetFields(ctx)...)
}
// With adds fields the context and return a logger with the same fields added
func With(ctx context.Context, log *zap.Logger, fields ...zap.Field) (context.Context, *zap.Logger) {
ctx = Context(ctx, fields...)
log = log.With(fields...)
return ctx, log
} The usage looks something like this: log := /* any existing zap.Logger */
ctx := Context(context.Background(), zap.String("hello", "world"))
log.Info("Just a log") // will just use the logger as is
log = WithContext(ctx, log) // creates a new logger with all fields (if any) from the context added
log.Info("A log with fields from the context") |
Hi @ptxmac , |
Finally, I had time to create a simple package for this. github.com/yuseferi/zax func main() {
logger, _ := zap.NewProduction()
ctx := context.Background()
s := NewServiceA(logger)
ctx = zax.Set(ctx, logger, []zap.Field{zap.String("trace_id", "my-trace-id")})
s.funcA(ctx)
}
type ServiceA struct {
logger *zap.Logger
}
func NewServiceA(logger *zap.Logger) *ServiceA {
return &ServiceA{
logger: logger,
}
}
func (s *ServiceA) funcA(ctx context.Context) {
s.logger.Info("func A") // it does not contain trace_id, you need to add it manually
zax.Get(ctx).Info("func A") // it will logged with "trace_id" = "my-trace-id"
} feel free to contribute to make it better folks 😄 ❤️ |
Hey guys, Basically instead of storing the whole logger in the context it just stores fields which cause better performance . example: func main() {
logger, _ := zap.NewProduction()
ctx := context.Background()
s := NewServiceA(logger)
ctx = zax.Set(ctx, zap.String("trace_id", "my-trace-id"))
// and if you want to add multiple of them at once
//ctx = zax.Set(ctx, []zap.Field{zap.String("trace_id", "my-trace-id"),zap.String("span_id", "my-span-id")})
s.funcA(ctx)
}
type ServiceA struct {
logger *zap.Logger
}
func NewServiceA(logger *zap.Logger) *ServiceA {
return &ServiceA{
logger: logger,
}
}
func (s *ServiceA) funcA(ctx context.Context) {
s.logger.Info("func A") // it does not contain trace_id, you need to add it manually
s.logger.With(zax.Get(ctx)...).Info("func A") // it will logged with "trace_id" = "my-trace-id"
} I would like to hear your thoughts |
Maybe one of ways to support it may be adding a new ctx, ok := fld.Interface.(context.Context) // fld is of type zapcore.Field. |
Requirement
context.Context
object and that context include metadata about a distributed transaction, such as distributed tracing information (trace_id, span_id, etc.) from Jaeger or Zipkin, I want to be able to tag all logs with those metadata fields, similar to how MDC works in [Java loggers][https://logback.qos.ch/manual/mdc.html].opentracing.Span
stored in the Context, i.e. translatelogger.Info(ctx, fields...)
tospanFrom(ctx).Log(fields)
What's preventing me from doing that in Zap
It is currently possible to do in Zap via
With([]Fields)
scoping of the Logger, however:Proposal
ContextLogger
, similar toSugarLogger
, which has methods similar to the regularLogger
, but always takingcontext.Context
as the first argument.SugarContextLogger
context.Context
encoding in the Entry/Encoders.-- Updated 2019-05-13
Added second requirement that came up in #654 (comment)
The text was updated successfully, but these errors were encountered: