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.

Sign up for GitHub

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

[prior art] sdk/log/xlog: Add FilterProcessor and EnabledParameters #6271

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from
+243 −123
Draft

[prior art] sdk/log/xlog: Add FilterProcessor and EnabledParameters #6271

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
96a79ee
mv sdk/log/internal/x sdk/xlog
pellared Feb 5, 2025
6282e71
Introduce xlog module with FilterProcessor and EnabledParameters
pellared Feb 5, 2025
69ff66d
Update changelog
pellared Feb 5, 2025
bf61d44
Add trace context fields
pellared Feb 5, 2025
5b3c8ca
Test FilterProcessor functionality
pellared Feb 5, 2025
f24fc64
Update changelog
pellared Feb 5, 2025
fffc077
go mod tidy
pellared Feb 5, 2025
55adcc4
Merge branch 'main' into sdk/xlogs
pellared Feb 5, 2025
008cf4d
Merge branch 'main' into sdk/xlogs
pellared Feb 6, 2025
4df6669
Remove span context fields from EnabledParameters
pellared Feb 6, 2025
d31f329
Simplify doc comments
pellared Feb 6, 2025
fe5ae63
go mod tidy
pellared Feb 6, 2025
4e54827
Move sdk/xlog to sdk/log/xlog
pellared Feb 6, 2025
abe7833
Fix hyperlink
pellared Feb 6, 2025
d5817c6
Update README.md
pellared Feb 6, 2025
c23714b
Merge branch 'main' into sdk/xlogs
pellared Feb 6, 2025
cb91d27
Merge branch 'main' into sdk/xlogs
pellared Feb 7, 2025
71376d1
Update sdk/log/example_test.go
pellared Feb 7, 2025
e6b34ae
Update sdk/log/logger.go
pellared Feb 7, 2025
4ff41e1
Format comment
pellared Feb 7, 2025
628f916
Add missing period
pellared Feb 7, 2025
98d6811
Do not release xlog
pellared Feb 7, 2025
5cf1eaf
Improve Go doc comments
pellared Feb 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- The `go.opentelemetry.io/otel/semconv/v1.30.0` package.
The package contains semantic conventions from the `v1.30.0` version of the OpenTelemetry Semantic Conventions.
See the [migration documentation](./semconv/v1.30.0/MIGRATION.md) for information on how to upgrade from `go.opentelemetry.io/otel/semconv/v1.28.0`(#6240)
- Make the initial release of `go.opentelemetry.io/otel/sdk/log/xlog`.
This new module contains experimental features of the OpenTelemetry Logs SDK.
This module is unstable and breaking changes may be introduced.
See our [versioning policy](VERSIONING.md) for more information about these stability guarantees. (#6271)
- Add `FilterProcessor` and `EnabledParameters` in `go.opentelemetry.io/otel/sdk/log/xlog` (#6271).

### Changed

Expand Down
3 changes: 3 additions & 0 deletions exporters/otlp/otlplog/otlploggrpc/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ require (
github.com/pmezard/go-difflib v1.0.0 // indirect
go.opentelemetry.io/auto/sdk v1.1.0 // indirect
go.opentelemetry.io/otel/metric v1.34.0 // indirect
go.opentelemetry.io/otel/sdk/log/xlog v0.0.0-00010101000000-000000000000 // indirect
golang.org/x/net v0.34.0 // indirect
golang.org/x/sys v0.30.0 // indirect
golang.org/x/text v0.22.0 // indirect
Expand All @@ -44,3 +45,5 @@ replace go.opentelemetry.io/otel/log => ../../../../log
replace go.opentelemetry.io/otel/trace => ../../../../trace

replace go.opentelemetry.io/otel/metric => ../../../../metric

replace go.opentelemetry.io/otel/sdk/log/xlog => ../../../../sdk/log/xlog
3 changes: 3 additions & 0 deletions exporters/otlp/otlplog/otlploghttp/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ require (
github.com/pmezard/go-difflib v1.0.0 // indirect
go.opentelemetry.io/auto/sdk v1.1.0 // indirect
go.opentelemetry.io/otel/metric v1.34.0 // indirect
go.opentelemetry.io/otel/sdk/log/xlog v0.0.0-00010101000000-000000000000 // indirect
golang.org/x/net v0.34.0 // indirect
golang.org/x/sys v0.30.0 // indirect
golang.org/x/text v0.22.0 // indirect
Expand All @@ -44,3 +45,5 @@ replace go.opentelemetry.io/otel/sdk => ../../../../sdk
replace go.opentelemetry.io/otel/metric => ../../../../metric

replace go.opentelemetry.io/otel/log => ../../../../log

replace go.opentelemetry.io/otel/sdk/log/xlog => ../../../../sdk/log/xlog
3 changes: 3 additions & 0 deletions exporters/stdout/stdoutlog/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ require (
github.com/pmezard/go-difflib v1.0.0 // indirect
go.opentelemetry.io/auto/sdk v1.1.0 // indirect
go.opentelemetry.io/otel/metric v1.34.0 // indirect
go.opentelemetry.io/otel/sdk/log/xlog v0.0.0-00010101000000-000000000000 // indirect
golang.org/x/sys v0.30.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)
Expand All @@ -34,3 +35,5 @@ replace go.opentelemetry.io/otel/trace => ../../../trace
replace go.opentelemetry.io/otel/sdk => ../../../sdk

replace go.opentelemetry.io/otel/metric => ../../../metric

replace go.opentelemetry.io/otel/sdk/log/xlog => ../../../sdk/log/xlog
12 changes: 8 additions & 4 deletions log/logger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,13 @@ type Logger interface {
// Enabled returns whether the Logger emits for the given context and
// param.
//
// The passed param is likely to be a partial record with only the
// bridge-relevant information being provided (e.g a param with only the
// Severity set). If a Logger needs more information than is provided, it
// This is useful for users that want to know if a [Record]
// will be processed or dropped before they perform complex operations to
// construct the [Record].
//
// The passed param is likely to be a partial record information being
// provided (e.g a param with only the Severity set).
// If a Logger needs more information than is provided, it
// is said to be in an indeterminate state (see below).
//
// The returned value will be true when the Logger will emit for the
Expand All @@ -46,7 +50,7 @@ type Logger interface {
// exist (e.g. performance, correctness).
//
// The param should not be held by the implementation. A copy should be
// made if the record needs to be held after the call returns.
// made if the param needs to be held after the call returns.
//
// Implementations of this method need to be safe for a user to call
// concurrently.
Expand Down
2 changes: 1 addition & 1 deletion sdk/log/doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ at a single endpoint their origin is decipherable.
See [go.opentelemetry.io/otel/log] for more information about
the OpenTelemetry Logs Bridge API.
See [go.opentelemetry.io/otel/sdk/log/internal/x] for information about the
See [go.opentelemetry.io/otel/sdk/log/xlog] for information about the
experimental features.
*/
package log // import "go.opentelemetry.io/otel/sdk/log"
15 changes: 6 additions & 9 deletions sdk/log/example_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
logapi "go.opentelemetry.io/otel/log"
"go.opentelemetry.io/otel/log/global"
"go.opentelemetry.io/otel/sdk/log"
"go.opentelemetry.io/otel/sdk/log/xlog"
)

// Initialize OpenTelemetry Logs SDK and setup logging using a log bridge.
Expand Down Expand Up @@ -52,6 +53,7 @@ func Example() {
}

// Use a processor that filters out records based on the provided context.
// It also demonstrates the use of experimental [xlog.FilterProcessor].
func ExampleProcessor_filtering() {
// Existing processor that emits telemetry.
var processor log.Processor = log.NewBatchProcessor(nil)
Expand Down Expand Up @@ -84,13 +86,8 @@ type ContextFilterProcessor struct {
log.Processor

lazyFilter sync.Once
// Use the experimental FilterProcessor interface
// (go.opentelemetry.io/otel/sdk/log/internal/x).
filter filter
}

type filter interface {
Enabled(ctx context.Context, param logapi.EnabledParameters) bool
// Support the experimental FilterProcessor interface.
filter xlog.FilterProcessor
}
pellared marked this conversation as resolved.
Show resolved Hide resolved

func (p *ContextFilterProcessor) OnEmit(ctx context.Context, record *log.Record) error {
Expand All @@ -100,9 +97,9 @@ func (p *ContextFilterProcessor) OnEmit(ctx context.Context, record *log.Record)
return p.Processor.OnEmit(ctx, record)
}

func (p *ContextFilterProcessor) Enabled(ctx context.Context, param logapi.EnabledParameters) bool {
func (p *ContextFilterProcessor) Enabled(ctx context.Context, param xlog.EnabledParameters) bool {
p.lazyFilter.Do(func() {
if f, ok := p.Processor.(filter); ok {
if f, ok := p.Processor.(xlog.FilterProcessor); ok {
p.filter = f
}
})
Expand Down
3 changes: 3 additions & 0 deletions sdk/log/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ require (
go.opentelemetry.io/otel v1.34.0
go.opentelemetry.io/otel/log v0.10.0
go.opentelemetry.io/otel/sdk v1.34.0
go.opentelemetry.io/otel/sdk/log/xlog v0.0.0-00010101000000-000000000000
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this mean when this is stabilized, this module will depend on an unstable module we control?

Copy link
Member Author

@pellared pellared Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, but I think the impact would be acceptable.

This would require the users to use versions that are compatible. This would affect mostly users that opt-in to use xlog so that it is a direct dependency. FWIW most people do not bump indirect dependencies.

Personaly, I think it is an acceptable trade-off.

Similarly we depend on unstable github.com/google/go-cmp and golang.org/x/sys and I do not thing it prevents as from stabilization of the modules that use it.

I think it is good to not depend on unstable modules but it is not always feasible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly we depend on unstable github.com/google/go-cmp and golang.org/x/sys and I do not thing it prevents as from stabilization of the modules that use it.

These are not maintained by us though.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would require the users to use versions that are compatible. This would affect mostly users that opt-in to use xlog so that it is a direct dependency. FWIW most people do not bump indirect dependencies.

I think this is the crux of the problem. We need to provide stability guarantees for the SDK we release. I don't think saying "people do not bump indirect dependencies" is a valid way to get around this.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it will share the same issue we had with cross-module internal dependencies, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if this will work with type assertions when the argument is an experiental type. But I will double check.

If not then I think I can make a longer list of arguments for Enabled (context, severity, scope, resource) instead of EnabledPararamters (which could be introduced when the feature is going to be stabilized).

I will try this in a separate PR(s) to have a comparison between different designs.

Thanks a lot for your feedback 🙏

Copy link
Member

@XSAM XSAM Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If not then I think I can make a longer list of arguments for Enabled (context, severity, scope, resource) instead of EnabledPararamters (which could be introduced when the feature is going to be stabilized).

Are you suggesting we are going to use the same EnabledParameters for Logger.Enabled and FilterProcessor.Enabled? This will lead us to an awkward position if we need to change FilterProcessor.Enabled after stabilizing the Logger.

I think we can copy experimental interface definitions to the stable module if we want to make it stable. After that, we drop the use of experimental interfaces in SDK. This will break all existing processors that implement the experimental interfaces, but it seems fine, as they are experimental features.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you suggesting we are going to use the same EnabledParameters for Logger.Enabled and FilterProcessor.Enabled?

No, we need scope.Instrumentation and resource.Resource. I meant something like

type FiltererProcessor interface {
   Enabled(context.Context, log.Severity, scope.Instrumentation, resource.Resource) bool
}

Copy link
Member Author

@pellared pellared Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We can use tooling to copy experimental interface definitions to the stable module if we want to ensure syncing, but not having the to-be-stable module depend on the unstable module prevents a set of user bugs and is preferential.

As suspected. It does not work with type assertions. However, I think we can try use reflect instead to make it working.

The benefit of this approach is that it should never break the compilation for the users. It mitigates the problem if they happen to run in a conflict and have incompatible version of xlog (e.g. transient dependencies).

The main drawback for users is no compile-time guarantee for people that opt-in in to an experimental feature.
The other drawback is the complexity of the implementation (on our side) because of using reflection.
The minor drawbacks are a little more overhead

It seems reasonable to favor stability guarantees. It also looks like a safer, less disruptive approach for our users. For sure it is worth creating a PR that tries doing it so that we can compare both approaches.

The next alternative is to have an experimental interface like

type FiltererProcessor interface {
   Enabled(context.Context, log.Severity, scope.Instrumentation, resource.Resource) bool
}

This way we would be able to use type assertions, but I think it is a bad idea given we would like to introduce EnabledParameters. It would make migration from experimental to stable very awkward and inconvenient. I do not like this and I do not want to go forward. I think this would look very hacky and unprofessional. At last we would not be able to use such approach for more complex parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

#6286 is ready for review.

go.opentelemetry.io/otel/trace v1.34.0
)

Expand All @@ -29,6 +30,8 @@ replace go.opentelemetry.io/otel/trace => ../../trace

replace go.opentelemetry.io/otel/sdk => ../

replace go.opentelemetry.io/otel/sdk/log/xlog => ./xlog

replace go.opentelemetry.io/otel/log => ../../log

replace go.opentelemetry.io/otel => ../..
35 changes: 0 additions & 35 deletions sdk/log/internal/x/README.md
View file
Open in desktop

This file was deleted.

47 changes: 0 additions & 47 deletions sdk/log/internal/x/x.go
View file
Open in desktop

This file was deleted.

12 changes: 9 additions & 3 deletions sdk/log/logger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
"go.opentelemetry.io/otel/log"
"go.opentelemetry.io/otel/log/embedded"
"go.opentelemetry.io/otel/sdk/instrumentation"
"go.opentelemetry.io/otel/sdk/log/internal/x"
"go.opentelemetry.io/otel/sdk/log/xlog"
"go.opentelemetry.io/otel/trace"
)

Expand Down Expand Up @@ -50,14 +50,20 @@ func (l *logger) Emit(ctx context.Context, r log.Record) {
// processed, true will be returned by default. A value of false will only be
// returned if it can be positively verified that no Processor will process.
func (l *logger) Enabled(ctx context.Context, param log.EnabledParameters) bool {
p := xlog.EnabledParameters{
Resource: *l.provider.resource,
InstrumentationScope: l.instrumentationScope,
Severity: param.Severity,
}

// If there are more Processors than FilterProcessors we cannot be sure
pellared marked this conversation as resolved.
Show resolved Hide resolved
// that all Processors will drop the record. Therefore, return true.
//
// If all Processors are FilterProcessors, check if any is enabled.
return len(l.provider.processors) > len(l.provider.fltrProcessors) || anyEnabled(ctx, param, l.provider.fltrProcessors)
return len(l.provider.processors) > len(l.provider.fltrProcessors) || anyEnabled(ctx, p, l.provider.fltrProcessors)
}

func anyEnabled(ctx context.Context, param log.EnabledParameters, fltrs []x.FilterProcessor) bool {
func anyEnabled(ctx context.Context, param xlog.EnabledParameters, fltrs []xlog.FilterProcessor) bool {
for _, f := range fltrs {
if f.Enabled(ctx, param) {
// At least one Processor will process the Record.
Expand Down
51 changes: 40 additions & 11 deletions sdk/log/logger_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/log"
"go.opentelemetry.io/otel/sdk/instrumentation"
"go.opentelemetry.io/otel/sdk/log/xlog"
"go.opentelemetry.io/otel/sdk/resource"
"go.opentelemetry.io/otel/trace"
)
Expand Down Expand Up @@ -224,11 +225,17 @@ func TestLoggerEnabled(t *testing.T) {
p1 := newFltrProcessor("1", true)
p2WithDisabled := newFltrProcessor("2", false)

emptyResource := resource.Empty()
res := resource.NewSchemaless(attribute.String("key", "value"))

testCases := []struct {
name string
logger *logger
ctx context.Context
expected bool
name string
logger *logger
ctx context.Context
expected bool
expectedP0Params []xlog.EnabledParameters
expectedP1Params []xlog.EnabledParameters
expectedP2Params []xlog.EnabledParameters
}{
{
name: "NoProcessors",
Expand All @@ -241,41 +248,63 @@ func TestLoggerEnabled(t *testing.T) {
logger: newLogger(NewLoggerProvider(
WithProcessor(p0),
WithProcessor(p1),
), instrumentation.Scope{}),
WithResource(res),
), instrumentation.Scope{Name: "scope"}),
ctx: context.Background(),
expected: true,
expectedP0Params: []xlog.EnabledParameters{{
Resource: *res,
InstrumentationScope: instrumentation.Scope{Name: "scope"},
}},
expectedP1Params: nil,
},
{
name: "WithDisabledProcessors",
logger: newLogger(NewLoggerProvider(
WithProcessor(p2WithDisabled),
WithResource(emptyResource),
), instrumentation.Scope{}),
ctx: context.Background(),
expected: false,
ctx: context.Background(),
expected: false,
expectedP2Params: []xlog.EnabledParameters{{}},
},
{
name: "ContainsDisabledProcessor",
logger: newLogger(NewLoggerProvider(
WithProcessor(p2WithDisabled),
WithProcessor(p0),
WithResource(emptyResource),
), instrumentation.Scope{}),
ctx: context.Background(),
expected: true,
ctx: context.Background(),
expected: true,
expectedP2Params: []xlog.EnabledParameters{{}},
expectedP0Params: []xlog.EnabledParameters{{}},
},
{
name: "WithNilContext",
logger: newLogger(NewLoggerProvider(
WithProcessor(p0),
WithProcessor(p1),
WithResource(emptyResource),
), instrumentation.Scope{}),
ctx: nil,
expected: true,
ctx: nil,
expected: true,
expectedP0Params: []xlog.EnabledParameters{{}},
expectedP1Params: nil,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
// Clean up the records before the test.
p0.params = nil
p1.params = nil
p2WithDisabled.params = nil

assert.Equal(t, tc.expected, tc.logger.Enabled(tc.ctx, log.EnabledParameters{}))
assert.Equal(t, tc.expectedP0Params, p0.params)
assert.Equal(t, tc.expectedP1Params, p1.params)
assert.Equal(t, tc.expectedP2Params, p2WithDisabled.params)
})
}
}
10 changes: 5 additions & 5 deletions sdk/log/processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import (
// or with other methods. It is the responsibility of the Processor to manage
// this concurrency.
//
// See [go.opentelemetry.io/otel/sdk/log/internal/x] for information about how
// a Processor can be extended to support experimental features.
// See [go.opentelemetry.io/otel/sdk/log/xlog.FilterProcessor] for information about how
// a Processor can support experimental filtering feature.
type Processor interface {
// OnEmit is called when a Record is emitted.
//
Expand All @@ -30,11 +30,11 @@ type Processor interface {
// Handler.
//
// The SDK invokes the processors sequentially in the same order as
// they were registered using [WithProcessor].
// they were registered using WithProcessor.
// Implementations may synchronously modify the record so that the changes
// are visible in the next registered processor.
// Notice that [Record] is not concurrent safe. Therefore, asynchronous
// processing may cause race conditions. Use [Record.Clone]
// Notice that Record is not concurrent safe. Therefore, asynchronous
// processing may cause race conditions. Use Record.Clone
// to create a copy that shares no state with the original.
OnEmit(ctx context.Context, record *Record) error

Expand Down
Loading