[reflect] sdk/log/xlog: Add FilterProcessor and EnabledParameters #6286

pellared · 2025-02-07T09:24:11Z

This introduces a new go.opentelemetry.io/otel/sdk/log/xlog module.
I created it because of two reasons:

Add the possibility to filter based on the resource and scope which is available for the SDK. The scope information is the most important as it gives the possibility to e.g. filter out logs emitted for a given logger. Thus e.g. Add min_severity to LoggerConfig opentelemetry-specification#4364 is not necessary. See OTEP: Logger.Enabled opentelemetry-specification#4290 (comment) for more context.
I think we may want to stabilize the Logs SDK before Add Enabled to LogRecordProcessor opentelemetry-specification#4363 is done and stabilized. Therefore, this feature is added a separate module. go.opentelemetry.io/otel/sdk/log module does not expose these experimental feature in its exported API; it is only available in docs and example test. The user would have to explicitly opt-in by using the go.opentelemetry.io/otel/sdk/log/xlog module which will always have v0.x.y versions. Hopefully at some point of time the features are added to go.opentelemetry.io/otel/sdk/log, deprecated in go.opentelemetry.io/otel/sdk/log/xlog and afterwards removed from go.opentelemetry.io/otel/sdk/log/xlog . I think that for at one release we would need to support both the deprecated go.opentelemetry.io/otel/sdk/xlog.FilterProcessor and new go.opentelemetry.io/otel/sdk/log.FilterProcessor to facilitate the migration process. The module name is inspired by xerrors.xlog is under sdk/log so that it could have access to packages under sdk/log/internal if necessary.
This is also related to Document strategy to support experimental methods on stable interfaces #5882 (comment)
This will also help me with working towards Add Enabled to LogRecordProcessor opentelemetry-specification#4363 as I would have a good example how we do support it.

The SDK supports xlog using reflect (duck typing) and the sdk/log production code does not depend on xlog so users should never experience build failures because of breaking changes in xlog.

EDIT: The performance difference is huge because of using reflect. It makes the feature even almost unusable as in most cases calling Enabled (when all Processors are FilterProcessors) would be a lot more expensive than creating a heavy log record. But maybe it is a cost that we need to accept when providing experimental feature with a new types? The reflect overhead would be removed if the types would be stabilized and moved to go.opentelemetry.io/otel/sdk/log.

goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/log
cpu: 13th Gen Intel(R) Core(TM) i7-13800H
                 │   old.txt   │                  new.txt                  │
                 │   sec/op    │     sec/op      vs base                   │
LoggerEnabled-20   4.589n ± 1%   1977.500n ± 3%  +42992.18% (p=0.000 n=10)

                 │   old.txt    │            new.txt             │
                 │     B/op     │     B/op      vs base          │
LoggerEnabled-20   0.000Ki ± 0%   1.811Ki ± 6%  ? (p=0.000 n=10)

                 │  old.txt  │           new.txt            │
                 │ allocs/op │ allocs/op   vs base          │
LoggerEnabled-20   0.00 ± 0%   16.00 ± 0%  ? (p=0.000 n=10)

Maybe I should just use this PR as a prototype for sake of open-telemetry/opentelemetry-specification#4363. Then we could add the FilterProcessor straight to the go.opentelemetry.io/otel/sdk/log. However, then I have no idea how long we would need to wait for stability of Logs SDK.

The other alternative is to go with #6271 to achieve performance for the cost of coupling the sdk/log to sdk/log/xlog (similarly to what Collector does). I think the performance overhead is less acceptable than the potential build failures that users can run into. Trade-offs...

Co-authored-by: Sam Xie <[email protected]>

pellared · 2025-02-07T09:33:10Z

I think that we can make it working if we use reflect instead of type assertions.

pellared · 2025-02-07T11:01:37Z

sdk/log/go.mod

 	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


What is interesting if the dependency is used only used in tests then it does not come up as an indirect dependency for modules using go.opentelemetry.io/otel/sdk/log 🎉

It looks like the Go module graph pruning has been improved at some point.

I think that it means that we could get rid of test modules 😉

This is a bit strange. I've just tried that for otelhttp, and the SDK does come as an indirect dependency.

Then I think we cannot rely on the Go modules pruning and I will get rid of this dependency.

Adressed 08ac146

This is a bit strange. I've just tried that for otelhttp, and the SDK does come as an indirect dependency.

@dmathieu , I tried it once more and actually it does NOT come as an indirect dependency.

See: open-telemetry/opentelemetry-go-contrib#6763

I had to run make go-mod-tidy twice. Probably if we used crosslink tidylist then it would not be necessary

We can could consider revert 08ac146. But I am fine with the current approach as well. It looks cleaner. I propose to keep the current way.

Ah, it looks like the difference is that you used a different package name. So that'd be the trick.

Ah, it looks like the difference is that you used a different package name. So that'd be the trick.

I do not follow. The SDK is used in tests of both of otelhttp and otelhttp_test packages.

Yes, but when I tried it, I kept the otelhttp package name for the tests. I think that's why the dependency still showed up.

Some tests still use it. Can you double check it? Make sure to run make go-mod-tidy more than once 😉

pellared · 2025-02-07T13:39:43Z

versions.yaml

      - go.opentelemetry.io/otel/schema
 excluded-modules:
  - go.opentelemetry.io/otel/internal/tools
+  - go.opentelemetry.io/otel/sdk/log/xlog


I think that relying on git hashes may be very annoying for the users.
It would be hard for them to keep sdk/log and sdk/log/xlog in sync.

We are making commits very frequently so they may even not bump it

It would be hard to figure it out which version of xlog is compatible with log with we do any breaking change.

On the other side, we can wait for making a release until people actually complain.

Thoughts?

For comparison, the collector keeps x packages within the normal release process.

I have doubts about the idea, but if we want to allow folks to be able to get very fresh code, we could setup auto nightly/weekly releases for the x packages. Then, they wouldn't rely on commits, but would remain on the latest.

CHANGELOG.md

dmathieu

Would it be possible to add a linter to ensure no stable module uses an x one?

CHANGELOG.md

Co-authored-by: Damien Mathieu <[email protected]>

pellared · 2025-02-11T13:17:49Z

Would it be possible to add a linter to ensure no stable module uses an x one?

I created #6298 to avoid scope creeping.
I added it to Logs GA project to make sure we address it before stabilizing the logs.

pellared · 2025-02-12T13:46:32Z

Would it be possible to add a linter to ensure no stable module uses an x one?

I created #6298 to avoid scope creeping. I added it to Logs GA project to make sure we address it before stabilizing the logs.

I found some time and I added the rule: 25937e0

pellared · 2025-02-13T13:45:18Z

I think that the performance overhead is huge (probably not acceptable, it would only make sense for users to call in case of enormous log records) and that we should reconsider #6271.

pellared · 2025-02-13T19:41:11Z

SIG meeting notes:
We agreed that we can move FilterProcessor directly to sdk/log as Logs SDK does not look to be stabilized soon.

Per #6271 (comment) > We agreed that we can move `FilterProcessor` directly to `sdk/log` as Logs SDK does not look to be stabilized soon. - Add the possibility to filter based on the resource and scope which is available for the SDK. The scope information is the most important as it gives the possibility to e.g. filter out logs emitted for a given logger. Thus e.g. open-telemetry/opentelemetry-specification#4364 is not necessary. See open-telemetry/opentelemetry-specification#4290 (comment) for more context. - It is going be an example for open-telemetry/opentelemetry-specification#4363 There is a little overhead (IMO totally acceptable) because of data transformation. Most importantly, there is no new heap allocation. ``` goos: linux goarch: amd64 pkg: go.opentelemetry.io/otel/sdk/log cpu: 13th Gen Intel(R) Core(TM) i7-13800H │ old.txt │ new.txt │ │ sec/op │ sec/op vs base │ LoggerEnabled-20 4.589n ± 1% 319.750n ± 16% +6867.75% (p=0.000 n=10) │ old.txt │ new.txt │ │ B/op │ B/op vs base │ LoggerEnabled-20 0.000Ki ± 0% 1.093Ki ± 13% ? (p=0.000 n=10) │ old.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ LoggerEnabled-20 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹ ¹ all samples are equal ``` `Logger.Enabled` is still more efficient than `Logger.Emit` (benchmarks from #6315). ``` goos: linux goarch: amd64 pkg: go.opentelemetry.io/otel/sdk/log cpu: 13th Gen Intel(R) Core(TM) i7-13800H BenchmarkLoggerEmit/5_attributes-20 559934 2391 ns/op 39088 B/op 1 allocs/op BenchmarkLoggerEmit/10_attributes-20 1000000 5910 ns/op 49483 B/op 5 allocs/op BenchmarkLoggerEnabled-20 1605697 968.7 ns/op 1272 B/op 0 allocs/op PASS ok go.opentelemetry.io/otel/sdk/log 10.789s ``` Prior art: - #6271 - #6286 I also created for tracking purposes: - https://github.com/open-telemetry/opentelemetry-go/issues/6328

pellared and others added 24 commits February 5, 2025 20:30

mv sdk/log/internal/x sdk/xlog

96a79ee

Introduce xlog module with FilterProcessor and EnabledParameters

6282e71

Update changelog

69ff66d

Add trace context fields

bf61d44

Test FilterProcessor functionality

5b3c8ca

Update changelog

f24fc64

go mod tidy

fffc077

Merge branch 'main' into sdk/xlogs

55adcc4

Merge branch 'main' into sdk/xlogs

008cf4d

Remove span context fields from EnabledParameters

4df6669

Simplify doc comments

d31f329

go mod tidy

fe5ae63

Move sdk/xlog to sdk/log/xlog

4e54827

Fix hyperlink

abe7833

Update README.md

d5817c6

Merge branch 'main' into sdk/xlogs

c23714b

Merge branch 'main' into sdk/xlogs

cb91d27

Update sdk/log/example_test.go

71376d1

Co-authored-by: Sam Xie <[email protected]>

Update sdk/log/logger.go

e6b34ae

Co-authored-by: Sam Xie <[email protected]>

Format comment

4ff41e1

Add missing period

628f916

Do not release xlog

98d6811

Improve Go doc comments

5cf1eaf

sdk/log to use internal copy of interface instead of xlog

9037771

pellared mentioned this pull request Feb 7, 2025

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

Closed

pellared commented Feb 7, 2025

View reviewed changes

pellared added 3 commits February 7, 2025 12:23

Cleanup go mod

3a07761

code snippet: TestReflectFilterProcessorPlayground

c56e9da

Support xlog with duck typing using reflect

91d84a9

pellared requested a review from dmathieu as a code owner February 7, 2025 13:33

pellared commented Feb 7, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pellared added 3 commits February 7, 2025 15:55

Update CHANGELOG.md

7075931

Update CHANGELOG.md

648e21f

Merge branch 'main' into xlog-not-used-by-sdk

2941fb3

pellared mentioned this pull request Feb 7, 2025

OTEP: Logger.Enabled open-telemetry/opentelemetry-specification#4290

Closed

Merge branch 'main' into xlog-not-used-by-sdk

2a93c9f

dmathieu reviewed Feb 11, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

Apply suggestions from code review

9da340a

Co-authored-by: Damien Mathieu <[email protected]>

pellared mentioned this pull request Feb 11, 2025

Add a linter to ensure no stable module uses xlog #6298

Closed

dmathieu approved these changes Feb 11, 2025

View reviewed changes

pellared added 3 commits February 11, 2025 19:11

Merge branch 'main' into xlog-not-used-by-sdk

d55948e

Remove usage of xlog in SDK

08ac146

Merge branch 'main' into xlog-not-used-by-sdk

9475ed8

pellared mentioned this pull request Feb 12, 2025

Remove otelhttp/test module open-telemetry/opentelemetry-go-contrib#6763

Merged

Add otel-experimental depguard rule

25937e0

pellared added 2 commits February 12, 2025 14:46

Merge branch 'main' into xlog-not-used-by-sdk

e4d1ebf

Merge branch 'main' into xlog-not-used-by-sdk

2ac7f71

pellared changed the title ~~sdk/log/xlog: Add FilterProcessor and EnabledParameters~~ [reflect] sdk/log/xlog: Add FilterProcessor and EnabledParameters Feb 13, 2025

dashpole mentioned this pull request Feb 13, 2025

Document strategy to support experimental methods on stable interfaces #5882

Open

pellared closed this Feb 13, 2025

pellared mentioned this pull request Feb 14, 2025

sdk/log: Add FilterProcessor and EnabledParameters #6317

Merged

pellared deleted the xlog-not-used-by-sdk branch May 20, 2025 09:00

pellared mentioned this pull request Jun 9, 2025

Remove test modules open-telemetry/opentelemetry-go-contrib#7444

Closed

[reflect] sdk/log/xlog: Add FilterProcessor and EnabledParameters #6286

[reflect] sdk/log/xlog: Add FilterProcessor and EnabledParameters #6286

Uh oh!

Conversation

pellared commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pellared commented Feb 7, 2025

Uh oh!

pellared Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmathieu Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

pellared Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pellared Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

pellared Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmathieu Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

pellared Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmathieu Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

pellared Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

pellared Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmathieu Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dmathieu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pellared commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pellared commented Feb 12, 2025

Uh oh!

pellared commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pellared commented Feb 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pellared commented Feb 7, 2025 •

edited

Loading

pellared Feb 7, 2025 •

edited

Loading

pellared Feb 11, 2025 •

edited

Loading

pellared Feb 12, 2025 •

edited

Loading

pellared Feb 12, 2025 •

edited

Loading

pellared Feb 7, 2025 •

edited

Loading

pellared commented Feb 11, 2025 •

edited

Loading

pellared commented Feb 13, 2025 •

edited

Loading