-
Notifications
You must be signed in to change notification settings - Fork 887
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
Specify how Logs SDK implements Enabled #4207
Comments
Proposal A - Extend Processor Below is a proposal mostly based on how Go Logs SDK currently implements The idea is to add an The parameters of The return value of Implementation of When the user calls func (l *logger) Enabled(ctx context.Context, param log.EnabledParameters) bool {
newParam := l.newEnabledParameters(ctx, param)
for _, p := range l.provider.processors {
if enabled := p.Enabled(ctx, newParam); enabled {
return true
}
}
return false
} In order the implement filtering, the processor would need to wrap (decorate) an existing processor. Example pseudocode of a processor filtering log records by setting a minimum severity level: // LogProcessor is an [log.Processor] implementation that wraps another
// [log.Processor]. It will pass-through calls to OnEmit and Enabled for
// records with severity greater than or equal to a minimum. All other method
// calls are passed to the wrapped [log.Processor].
type LogProcessor struct {
log.Processor
Minimum api.Severity
}
// OnEmit passes ctx and r to the [log.Processor] that p wraps if the severity
// of record is greater than or equal to p.Minimum. Otherwise, record is
// dropped.
func (p *LogProcessor) OnEmit(ctx context.Context, record *log.Record) error {
if record.Severity() >= p.Minimum {
return p.Processor.OnEmit(ctx, record)
}
return nil
}
// Enabled returns if the [log.Processor] that p wraps is enabled if the
// severity of record is greater than or equal to p.Minimum. Otherwise false is
// returned.
func (p *LogProcessor) OnEnabled(ctx context.Context, param log.EnabledParameter) bool {
lvl, ok := param.Severity()
if !ok {
return true
}
return lvl >= p.Minimum && p.Processor.OnEnabled(ctx, param)
} I also want to call out that the idea of wrapping/decorating the processors is already used by the isolating processor. Personal remarks: |
Proposal B - Add filtering via Filterer Below is a proposal inspired by sampling design in tracing SDK. The idea is to add a new abstraction named The parameters of The return value of Pseudocode: type Filterer interface {
Filter(ctx context.Context, param FilterParameters) bool
} When the user calls // Enabled returns false if at least one Filterer held by the LoggerProvider
// that created the logger will return false for the provided context and param.
func (l *logger) Enabled(ctx context.Context, param log.EnabledParameters) bool {
newParam := l.newEnabledParameters(param)
for _, flt := range l.provider.filterers {
if !flt.Filter(ctx, newParam) {
return false
}
}
return true
} When the user calls func (l *logger) Emit(ctx context.Context, r log.Record) {
param:= l.toEnabledParameters(r)
if !l.Enabled(ctx, param) {
return
}
newRecord := l.newRecord(ctx, r)
for _, p := range l.provider.processors {
if err := p.OnEmit(ctx, &newRecord); err != nil {
otel.Handle(err)
}
}
} Pseudocode of a custom minimum severity level filterer : type MinSevFilterer struct {
Minimum api.Severity
}
func (f *MinSevFilterer) Filter(ctx context.Context, param log.FilterParameters) bool {
lvl, ok := param.Severity()
if !ok {
return true
}
return lvl >= p.Minimum
} This proposal assumes that each logger provider is used to define a separate processing pipeline. It is more aligned with the current specification design for all signals. Filtering is coupled to emitting log records. Therefore, a custom processor filtering implementation does not need to implement filtering on both At last such design should be easier to be added to existing SDKs as it adds a new abstraction instead of adding more responsibilities to |
I'm a little concerned that by "defining" the way that "enabled" should be implemented (especially adding layers of filtering (as part of the required implementation)) would (potentially) worse from a perf perspective than just constructing and emitting the Log that eventually gets dropped. ie. the whole point of the "Is enabled" is for avoid perf impact... |
I think it's fine to define "what" options should be used (ie the parameters) to "check" whether something is enabled or not and this should be limited. |
I am not sure if I follow.
Which part of the implementation proposal (A or B or both) are you concerned about? |
Agree. PTAL #4203 where I try to propose a minimal set of parameters. |
Prescribing that it must be done via a specific method (A or B), rather than letting the language / environment determine the best way to provide the capability. |
@MSNev the issue with not specifying behavior is that it can be specified later in a non-compatible way. Meaning, if an implementation implements A and the spec later specifies B that language SIG is non-compliant. Not having some language about implementation both stops implementations from adopting this feature for that reason and causes specification maintainer to not approve any further changes1. This issue it trying to break out of this circular logic. If you think things should be left undefined, please work with @jack-berg to reach a consensus as his statements ask for the opposite. Footnotes |
What about adding a severity level as part of the logger config? It seems to match the common log workflow of changing severity threshold for specific loggers. I'm suspicious of the idea of introducing the concept of an expandable set of |
I think that it is reasonable to improve the logging configuration than introducing a new concept.
I agree. I assume that setting a global severity threshold is also a common workflow.
PTAL at #4203 (comment) |
Per discussion with @pellared, if the logger can have an |
If we unify on the logger config, we would not be able to have different severity level support for different processors. If you have one processor that wants to send to short-term storage and hold all debug logs and a long-term storage that only store error logs and above. |
I find Proposal A giving the biggest amount of flexibility in terms of multiple processing pipelines. The alternative would be to use a "multi provider" like here: open-telemetry/opentelemetry-go#5830. However, this may decrease the performance (I can evaluate it). |
Context: #4203 (comment)
Blocked by:
The text was updated successfully, but these errors were encountered: