Skip to content
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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

erezrokah
Copy link

@erezrokah erezrokah commented Jul 22, 2024

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

@erezrokah erezrokah changed the title feat: Add method to get event fields feat: Add method to get event fields inside a Hook Jul 22, 2024
event.go Outdated Show resolved Hide resolved
event.go Outdated Show resolved Hide resolved
event.go Outdated Show resolved Hide resolved
@erezrokah
Copy link
Author

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

Copy link

@ccoVeille ccoVeille left a 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

event.go Show resolved Hide resolved
event.go Show resolved Hide resolved
event.go Outdated Show resolved Hide resolved
erezrokah and others added 3 commits July 22, 2024 22:23
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
@erezrokah erezrokah requested a review from ccoVeille July 22, 2024 19:32
event_test.go Outdated Show resolved Hide resolved
Copy link

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🤞

@chkp-omris
Copy link

@ccoVeille @erezrokah I'd also be very happy to have this feature merged, I happened to find this PR looking for a solution.
May I suggest that the implementation simply returns the internal buffer buf to avoid forcing JSON decoding on the user?

@erezrokah
Copy link
Author

erezrokah commented Aug 11, 2024

@ccoVeille @erezrokah I'd also be very happy to have this feature merged, I happened to find this PR looking for a solution.
May I suggest that the implementation simply returns the internal buffer buf to avoid forcing JSON decoding on the user?

I following #637 (comment) as not to expose the internal buf. If we expose buf and then later there's a need to change the internal representation of the event to something else it will be harder to do

@mark-ten9
Copy link

It would be great to merge this if possible so that we don't have to use reflection to work around this.

@nduong-ol
Copy link

Hello, it has been a while. Would it be possible to merge this PR?

@christopher-taylor
Copy link

Chiming in to say I would love this feature to be merged in!

@nduong-ol
Copy link

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

@christopher-taylor
Copy link

christopher-taylor commented Oct 6, 2024

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?

@nduong-ol
Copy link

nduong-ol commented Oct 7, 2024

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

       var writers []io.Writer {
		zerolog.ConsoleWriter{Out: os.Stdout, TimeFormat: time.RFC3339},
                NewOtelLogger("app-name"),
	}

	logLevel := zerolog.InfoLevel
	logContext := zerolog.New(io.MultiWriter(writers...)).Level(logLevel).With().Timestamp()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[question] How to access with fields inside a hook
6 participants