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 48 commits
ae8ef9a
3ddff38
f75f983
a724fc6
c636a57
fae752b
5a4e371
32c39dd
acd70b1
1e6243e
9275c7a
38e4c98
dc07a15
0957402
50298f1
3cf5017
765166a
2503bdc
9bb6185
ee60483
c596398
a4257ef
7c0b7de
21b28e5
0e66e57
3db75b9
33d36c5
f16513d
ab8e05e
cc0eb1e
537a3c0
5a76201
5046347
b62b2c2
ada00e4
ba779dd
a3401bf
81bb7e0
65683dd
db796ec
637ec17
ec21e17
27fa6c8
af8f08b
568b2b7
cb3b52a
bdc1eea
5db20fc
216a218
e5d91d5
5820653
5d35d3c
80073ea
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.
I think we'll (eventually) have more config options for severity, sampling, and other things, e.g. a few extra scenarios I can think of:
We'll need to think how to structure them on
LoggerConfig
API so it's extendable but only introduce options we have an immediate need for).E.g. I think we may need something like
LoggerConfig.sampling_strategy = always_on | span_based
instead ofdisabled_on_sampled_out_spans
.Also we'd need to consider having a callback in addition to pure declarative config (e.g. for cases like sampling based on spans, but also startup logs are all in). For this we should consider having a sampler-like interface which would have implementations similar to tracer sampler and then we might want to steal declarative config from
tracer_provider.sampler
.TL;DR:
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.
Adding a field is a backwards compatible change. I agree that we should add new fields (options) only if we have a big demand for it.
For more ad I think the
LogRecordProcessor
comes into play. It should handle the most advanced scenarios.I will do my best to describe it "Future possibilities" section.
IMO extensibility (
LogRecordProcessor.Enabled
) is more important as it can also solve the (3) and (4) use cases even without the config extensions.I would like to know if there is a use cases that the proposal of adding
LogRecordProcessor.Enabled
would not solve.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 added "Alternatives: Dynamic Evaluation in LoggerConfig"
I added "Future possibilities: Extending LoggerConfig
We would need to closer look at the alternatives when prototyping and during the Development phase of the spec.
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.
what would you expect processors to do inside
Enabled
? If this is a delegating processor, would it call the underlying one? When I'm implementing a processor that does filtering, I should assume it's in the certain position in the pipeline?I.e. it sounds like there should be just one processor in the pipeline that decides if log should be created. If so, should it be a processor responsibility at all or can we call this component
LogFilter
,LogSampler
, etc and make it a configurable behavior independent of the processor?If this component returns true, all processing pipelines would get log record and can drop it if they are filtering too.
If it returns false, none of them should. This is the same as you have outlined in the otep, just extracts filtering from processing.
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.
Some examples of logging filters:
Here's a good old .NET ILogger filter
or good old log4j
I.e. composition instead of inheritance - when the logging is configured, each appender/provider pipeline gets one dedicated filter independent of the appender/provider logic.
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.
Depends on the processor.
Enabled
, returns results only for itself. Example: https://github.com/open-telemetry/opentelemetry-rust-contrib/blob/30a3f4eeac0be5e4337f020b7e5bfe0273298858/opentelemetry-user-events-logs/src/logs/exporter.rs#L158-L165. Use case (5).I think is that it does not suites well (5) nor (6) use cases.
For (5) would someone configuring an user_events exporter would have to configure a filterer as well as processor?
For (6) it would not be even possible to e.g. allowing distinct minimum severity levels for different processorsas the filters would run before the processors.
The other way is that the processors themselves would accept a filterer, but I do not think we want to Logs SDK to become https://logging.apache.org/log4j/2.x/manual/architecture.html (which by the way does not offer an
Enabled
API).Filtering processor mentioned before wraps an existing processor so it is following the principle to favor composition over inheritance.
I added "Alternatives: Separate LogRecordFilterer Abstraction".
We would need to closer look at the alternatives when prototyping and during the Development phase of the spec.