Skip to content

INFOPLAT-1035: logger implement otelzap #1359

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

Merged
merged 20 commits into from
Jul 28, 2025

Conversation

kirqz23
Copy link
Contributor

@kirqz23 kirqz23 commented Jul 9, 2025

This PR introduces OtelZapCore, a custom zapcore.Core implementation that emits structured logs to OpenTelemetry using the OTLP protocol.

It also integrates this core into the existing logger package, allowing all logs to be exported via OTEL while preserving existing logging interfaces.

@kirqz23 kirqz23 requested a review from a team as a code owner July 9, 2025 12:34
@kirqz23 kirqz23 requested a review from krehermann July 9, 2025 12:34
@kirqz23 kirqz23 marked this pull request as draft July 9, 2025 12:34
Copy link

github-actions bot commented Jul 9, 2025

👋 kirqz23, thanks for creating this pull request!

To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team.

Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks!


// NewOtelCore initializes an OpenTelemetry Core for exporting logs in OTLP format
func NewOtelZapCore(loggerProvider otellog.LoggerProvider, opts ...Option) zapcore.Core {
logger := loggerProvider.Logger(instrumentationLibName)
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this do and is this correct?

iiuc it implies that all logs will refer to github.com/smartcontractkit/chainlink-common as opposed to the actual caller

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally agree, we should make it configurable for the logging app

Copy link
Contributor

@pkcll pkcll Jul 9, 2025

Choose a reason for hiding this comment

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

instrumentationLibName should be configurable

@kirqz23 kirqz23 marked this pull request as ready for review July 9, 2025 14:08
type Option func(c *OtelZapCore)

// NewOtelCore initializes an OpenTelemetry Core for exporting logs in OTLP format
func NewOtelZapCore(loggerProvider otellog.LoggerProvider, opts ...Option) zapcore.Core {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets pass logger interface here to avoid indirect dependency

Suggested change
func NewOtelZapCore(loggerProvider otellog.LoggerProvider, opts ...Option) zapcore.Core {
func NewOtelZapCore(logger otellog.Logger, opts ...Option) zapcore.Core {

@@ -102,6 +104,11 @@ func NewWithSync(w io.Writer) Logger {
return &logger{zap.New(core).Sugar()}
}

// NewWithOtelZapCore returns a new Logger using an OpenTelemetry Zap Core.
func NewWithOtelZapCore(loggerProvider otellog.LoggerProvider) Logger {
Copy link
Contributor

@pkcll pkcll Jul 9, 2025

Choose a reason for hiding this comment

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

Suggested change
func NewWithOtelZapCore(loggerProvider otellog.LoggerProvider) Logger {
func NewWithOtelProvider(loggerProvider otellog.LoggerProvider, loggerName string) Logger {

@@ -102,6 +104,11 @@ func NewWithSync(w io.Writer) Logger {
return &logger{zap.New(core).Sugar()}
}

// NewWithOtelZapCore returns a new Logger using an OpenTelemetry Zap Core.
func NewWithOtelZapCore(loggerProvider otellog.LoggerProvider) Logger {
return &logger{zap.New(NewOtelZapCore(loggerProvider)).Sugar()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return &logger{zap.New(NewOtelZapCore(loggerProvider)).Sugar()}
return &logger{zap.New(NewOtelZapCore(loggerProvider.Logger(loggerName))).Sugar()}

Copy link
Contributor

@pkcll pkcll Jul 14, 2025

Choose a reason for hiding this comment

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

nit: maybe worth using the same name for the file and the package, otelzap seems concise and clear
Consider renaming pkg/logger/otellogger/zap_otel.go to pkg/logger/otelzap/otelzap.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

pkcll
pkcll previously approved these changes Jul 14, 2025

// NOTE: [beholder.SetGlobalOtelProviders](https://github.com/smartcontractkit/chainlink-common/blob/27faefc9ce454c8aa2b1b7484e377ea3e8996bba/pkg/beholder/global.go#L50)
// must be called before using `NewOtelLogger` otherwise otelglobal.GetLoggerProvider() returns noop provider
func NewOtelLogger(name string) otellog.Logger {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the expected use of this function? We did a very deliberate conversion away from global logging a long time ago, and we shouldn't encourage that pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Co-authored-by: Jordan Krage <[email protected]>
@@ -102,6 +102,12 @@ func NewWithSync(w io.Writer) Logger {
return &logger{zap.New(core).Sugar()}
}

// NewWithCores returns a new Logger with one or more zapcore.Core.
// If multiple cores are provided, they are combined using zapcore.NewTee.
func NewWithCores(cores ...zapcore.Core) Logger {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to add some tests for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do we want to test here? This function just returns the logger with zapcore tee functionality

@kirqz23 kirqz23 requested a review from jmank88 July 28, 2025 17:41
@jmank88 jmank88 merged commit 7331df7 into main Jul 28, 2025
15 of 18 checks passed
@jmank88 jmank88 deleted the feature/INFOPLAT-1035-logger-implement-otelzap branch July 28, 2025 18:47
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.

5 participants