-
Notifications
You must be signed in to change notification settings - Fork 574
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
feat: Add method to get event fields inside a Hook #682
base: master
Are you sure you want to change the base?
Conversation
Thanks for the quick review @ccoVeille, followed up with the suggestions and added more tests. Also please see the comment about the tradeoff of using JSON Decode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for applying the changes, here are a few more feedbacks
Co-authored-by: ccoVeille <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🤞
@ccoVeille @erezrokah I'd also be very happy to have this feature merged, I happened to find this PR looking for a solution. |
I following #637 (comment) as not to expose the internal |
It would be great to merge this if possible so that we don't have to use reflection to work around this. |
Hello, it has been a while. Would it be possible to merge this PR? |
Chiming in to say I would love this feature to be merged in! |
In the end I find that to implement io.Writer interface receive the log output from zerolog and export it with OTLP is quite effective also. We can get the intended formatted log from zerolog without the need to process the internal data ourself |
Do you have a minimal example implementation? |
Sure. Here the code of the logger with the same approach of the Hook but instead of receiving zerolog.Event, it takes the byte slice from logger output and export it. The approach is similar on how we would write zerolog to a file. In this approach I didn't unmarshal the json log to get the log level because I'm lazy. If you want to can unmarshal the zerolog json to extract information before sending it over OTLP package` logging
import (
"context"
"io"
"go.opentelemetry.io/otel/log"
"go.opentelemetry.io/otel/log/global"
)
type config struct {
provider log.LoggerProvider
version string
schemaURL string
}
func newConfig(options []Option) config {
var c config
for _, opt := range options {
c = opt.apply(c)
}
if c.provider == nil {
c.provider = global.GetLoggerProvider()
}
return c
}
func (c config) logger(name string) log.Logger {
var opts []log.LoggerOption
if c.version != "" {
opts = append(opts, log.WithInstrumentationVersion(c.version))
}
if c.schemaURL != "" {
opts = append(opts, log.WithSchemaURL(c.schemaURL))
}
return c.provider.Logger(name, opts...)
}
// Option configures a Hook.
type Option interface {
apply(config) config
}
type optFunc func(config) config
func (f optFunc) apply(c config) config { return f(c) }
// WithVersion returns an [Option] that configures the version of the
// [log.Logger] used by a [OtelLogger]. The version should be the version of the
// package that is being logged.
func WithVersion(version string) Option {
return optFunc(func(c config) config {
c.version = version
return c
})
}
// WithSchemaURL returns an [Option] that configures the semantic convention
// schema URL of the [log.Logger] used by a [OtelLogger]. The schemaURL should be
// the schema URL for the semantic conventions used in log records.
func WithSchemaURL(schemaURL string) Option {
return optFunc(func(c config) config {
c.schemaURL = schemaURL
return c
})
}
// WithLoggerProvider returns an [Option] that configures [log.LoggerProvider]
//
// By default if this Option is not provided, the Logger will use the global LoggerProvider.
func WithLoggerProvider(provider log.LoggerProvider) Option {
return optFunc(func(c config) config {
c.provider = provider
return c
})
}
var _ io.WriteCloser = (*OtelLogger)(nil)
type OtelLogger struct {
logger log.Logger
}
func NewOtelLogger(name string, options ...Option) *OtelLogger {
cfg := newConfig(options)
return &OtelLogger{
logger: cfg.logger(name),
}
}
func (l *OtelLogger) Write(p []byte) (n int, err error) {
r := log.Record{}
r.SetSeverity(log.SeverityInfo)
r.SetBody(log.StringValue(string(p)))
r.SetSeverityText("INFO")
l.logger.Emit(context.Background(), r)
return len(p), nil
}
// Close implements io.Closer, and closes the current logfile.
func (l *OtelLogger) Close() error {
return nil
} And the integration with zerolog
|
Hi 👋 Thank you for this great module 🚀
Opening for feedback as an attempt to fix #493, #587, also referenced from https://github.com/open-telemetry/opentelemetry-go-contrib/pull/5918/files#diff-a9b82897838da6b8eed398e03c9590621e6c2336ff576b3a49fb684ff8bba2a1R110
This is not the cleanest approach, but could be better from using reflection. It doesn't expose the internal buffer representation format so if it ever changes
GetFields
can be modified.Feedback very much welcomed