From 4e16a936b6f7ab9f3be291c70bf26d0354602116 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 27 Dec 2023 13:53:03 +0100 Subject: [PATCH] Comments --- log/DESIGN.md | 4 +- log/doc.go | 85 ++++++++++++++++++++++++++++++++++++++-- log/embedded/embedded.go | 2 +- log/logger.go | 9 +++++ log/noop/noop.go | 2 +- log/provider.go | 6 +++ 6 files changed, 101 insertions(+), 7 deletions(-) diff --git a/log/DESIGN.md b/log/DESIGN.md index 6794abb1bc4..3c17706be62 100644 --- a/log/DESIGN.md +++ b/log/DESIGN.md @@ -59,7 +59,7 @@ The log bridges can use [`sync.Pool`](https://pkg.go.dev/sync#Pool) for reducing the number of allocations when mapping attributes. The bridge implementation should do its best to pass -the `ctx` containing the trace context from the call-site +the `ctx` containing the trace context from the caller so it can later passed via `Emit`. Re-constructing a `context.Context` with [`trace.ContextWithSpanContext`](https://pkg.go.dev/go.opentelemetry.io/otel/trace#ContextWithSpanContext) and [`trace.NewSpanContext`](https://pkg.go.dev/go.opentelemetry.io/otel/trace#NewSpanContext) @@ -99,7 +99,7 @@ logger.Emit(ctx, Record{Severity: log.SeverityInfo, Body: "Application started." ### API implementation -If the implementation processes the asynchronously, +If the implementation processes the record asynchronously, then it has to copy record attributes, in order to avoid use after free bugs and race condition. diff --git a/log/doc.go b/log/doc.go index f87b9383f50..1d127d566c1 100644 --- a/log/doc.go +++ b/log/doc.go @@ -1,7 +1,86 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -// Package log provides the OpenTelemetry Bridge API. -// -// TODO: comment. +/* +Package log defines the OpenTelemetry Bridge API. +Application code should not call this API directly. +The API is supposed to be used by a log bridge implementation, +which is an adapter between an existing logging library and the OpenTelemetry. + +Existing logging libraries generally provide a much richer set of features +than what is defined in OpenTelemetry. +It is not a goal of OpenTelemetry to ship a feature-rich logging library. + +# Bridge Implementations + +The bridge implementation should allow passing +the [context.Context] containing the trace context from the caller +to [Logger]'s Emit method. + +The log bridges can use [sync.Pool] +for reducing the number of allocations when mapping attributes. + +# API Implementations + +This package does not conform to the standard Go versioning policy, all of its +interfaces may have methods added to them without a package major version bump. +This non-standard API evolution could surprise an uninformed implementation +author. They could unknowingly build their implementation in a way that would +result in a runtime panic for their users that update to the new API. + +The API is designed to help inform an instrumentation author about this +non-standard API evolution. It requires them to choose a default behavior for +unimplemented interface methods. There are three behavior choices they can +make: + + - Compilation failure + - Panic + - Default to another implementation + +All interfaces in this API embed a corresponding interface from +[go.opentelemetry.io/otel/log/embedded]. If an author wants the default +behavior of their implementations to be a compilation failure, signaling to +their users they need to update to the latest version of that implementation, +they need to embed the corresponding interface from +[go.opentelemetry.io/otel/log/embedded] in their implementation. For +example, + + import "go.opentelemetry.io/otel/log/embedded" + + type LoggerProvider struct { + embedded.LoggerProvider + // ... + } + +If an author wants the default behavior of their implementations to a panic, +they need to embed the API interface directly. + + import "go.opentelemetry.io/otel/log" + + type LoggerProvider struct { + log.LoggerProvider + // ... + } + +This is not a recommended behavior as it could lead to publishing packages that +contain runtime panics when users update other package that use newer versions +of [go.opentelemetry.io/otel/log]. + +Finally, an author can embed another implementation in theirs. The embedded +implementation will be used for methods not defined by the author. For example, +an author who wants to default to silently dropping the call can use +[go.opentelemetry.io/otel/log/noop]: + + import "go.opentelemetry.io/otel/log/noop" + + type LoggerProvider struct { + noop.LoggerProvider + // ... + } + +It is strongly recommended that authors only embed +[go.opentelemetry.io/otel/log/noop] if they choose this default behavior. +That implementation is the only one OpenTelemetry authors can guarantee will +fully implement all the API interfaces when a user updates their API. +*/ package log // import "go.opentelemetry.io/otel/log" diff --git a/log/embedded/embedded.go b/log/embedded/embedded.go index 9993ecf73a8..30bda5c4f7b 100644 --- a/log/embedded/embedded.go +++ b/log/embedded/embedded.go @@ -2,7 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 // Package embedded provides interfaces embedded within -// the [OpenTelemetry Logs Bridge API]. +// the OpenTelemetry Logs Bridge API. // // Implementers of the [OpenTelemetry Logs Bridge API] can embed the relevant type // from this package into their implementation directly. Doing so will result diff --git a/log/logger.go b/log/logger.go index f75c9c9c9d0..d929a5e66f4 100644 --- a/log/logger.go +++ b/log/logger.go @@ -10,9 +10,18 @@ import ( ) // Logger TODO: comment. +// +// Warning: Methods may be added to this interface in minor releases. See +// package documentation on API implementation for information on how to set +// default behavior for unimplemented methods. type Logger interface { embedded.Logger // Emit TODO: comment. + // + // This method should not modify the record's attributes. + // + // This method should be safe to call concurrently. + // It should copy the record's attributes if the record is processed asynchronously. Emit(ctx context.Context, record Record) } diff --git a/log/noop/noop.go b/log/noop/noop.go index 2d6e4dd2ebc..266c9bc22fe 100644 --- a/log/noop/noop.go +++ b/log/noop/noop.go @@ -1,7 +1,7 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -// Package noop provides an implementation of the [OpenTelemetry Logs Bridge API] that +// Package noop provides an implementation of the OpenTelemetry Logs Bridge API that // produces no telemetry and minimizes used computation resources. // // Using this package to implement the [OpenTelemetry Logs Bridge API] will effectively diff --git a/log/provider.go b/log/provider.go index c1f78d10440..0938c88a8b0 100644 --- a/log/provider.go +++ b/log/provider.go @@ -9,10 +9,16 @@ import ( ) // LoggerProvider TODO: comment. +// +// Warning: Methods may be added to this interface in minor releases. See +// package documentation on API implementation for information on how to set +// default behavior for unimplemented methods. type LoggerProvider interface { embedded.LoggerProvider // Logger TODO: comment. + // + // This method should be safe to call concurrently. Logger(name string, options ...LoggerOption) Logger }