Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
OTEP: Logger.Enabled #4290
base: main
Are you sure you want to change the base?
OTEP: Logger.Enabled #4290
Changes from all commits
b78c2b3
dad7d90
8d194d6
e90776f
44c26df
0dfdb46
28c7ca4
5707a91
f26dfaa
26f915c
ba900ee
84a2779
d092b72
a5aee99
45b6c90
d258965
cf05e57
4e60d96
8096622
21b1702
36bcf8d
fa7ad76
18424ce
2bf0540
e907d2e
9cd2311
9f3ed05
e108b90
b782c8f
1bc5bb4
ce6d2a6
6978351
6d25690
b963292
e1e727b
aee7348
93860bf
e4cd587
ebab731
8b98332
2bbdab7
472d151
96fbf43
4ee1696
a6a87a9
509c501
222425d
0e6c0ae
07cf515
0c555f9
48c1e11
9431fac
be8343e
1a5761e
c5c8f3b
76d59e3
257fbf3
ccc1b92
bb36dd3
fe2772b
84205c3
a298337
b01b35d
cb2dcbd
04da313
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, this is configurable on a per-processor level, not just SDK wide, so you can have alternate destinations for different log severity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I will add this use case. Also given later I already described:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be doing too much. Why not just focus on adding logger enabled like the title indicates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These number references are confusing as the seem to be referencing something from another section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having two "layers" of setting up the severity filtering may be very confusing for the users.
The behavior of
LoggerConfig.min_severity
could be achieved also via a filter processor that takes into consideration theInstrumentationScope
when filtering by severity level.Most probably for languages like Java
LoggerConfig.min_severity
won't be helpful as the severity level is already set on the bridged logging libraries.The important piece is to support cases like described here: https://github.com/open-telemetry/opentelemetry-specification/pull/4290/files#r1925688450 which is not possible via
LoggerConfig.min_severity
@jack-berg, @trask, any opinion?
@jack-berg, are all OTel SDKs are supposed to support
LoggerConfig
(assuming it gets stabilized) or is it is assumed as an optional feature? Couldn't we provide processors which result in the same behavior?CC @MrAlias
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
psuedo-code of configuration that could be possible when we use processors instead of logger config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: I think that
LoggerConfig
functionality could be replaced with aLogRecordProcessor
decorator that implements the same thing. I may try to prototype it.