-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Spike] sdk/log: Change FilterProcessor to Filterer #5825
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5825 +/- ##
=====================================
Coverage 84.5% 84.5%
=====================================
Files 272 273 +1
Lines 22734 22739 +5
=====================================
+ Hits 19224 19231 +7
+ Misses 3167 3165 -2
Partials 343 343 |
@open-telemetry/go-approvers, PTAL. |
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 like the idea of having a similar design in the trace provider.
But I find this naming, Filterer,
a little strange, as I am not sure there is a word for that. Maybe Enabler
? 🤔
@@ -30,6 +29,7 @@ const ( | |||
type providerConfig struct { | |||
resource *resource.Resource | |||
processors []Processor | |||
filterers []Filterer |
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.
Just like the sampler
in TracerProvider
, could we only have a single instance here? If no one is complaining about the current implementation of the sampler
, maybe it is ok to have just one instance.
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 this approach is simpler and more consistent with how processing works.
For reference, the samplers can be decorators e.g. ParentBased Sampler. There is nothing technically preventing creating a filterer decorator. However, I feel that accepting multiple filterers increases the ease of use.
Initially I wanted to name it |
param.SetSeverity(r.Severity()) | ||
if !l.Enabled(ctx, param) { |
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.
Maybe it should be possible to make it possible to filter out based on more parameters than passed via Logger.Enabled? E.g. log record body, attributes, etc.
This could be enhanced later without making any breaking change.
Another feedback received during calls is that this proposal is less flexible and makes declaring some pipelines harder. For example, how to define 2 log record processing pipelines with different filters? For such case the filterer would have to be attached to a processor (not globally for the whole SDK) Chaining filters that are coupled to emitted logs is also awkward e.g. // Both processors also implement the Filterer interface.
etw := &ETWLogProcessor{}
proc := &RateLimitingProcessor{
Processor: etw,
Duration: time.Second,
Size: 1000,
}
provider := log.NewLoggerProvider(
log.WithProcessor(proc),
log.WithFilterer(proc),
log.WithFilterer(etw),
) It makes decorating processors more bug-prone. |
Towards open-telemetry/opentelemetry-specification#4207
Based on Proposal B
This is another idea of implementing filtering to the Logs SDK.
I think this design is more in the spirit of the specification and current SDK design. The initial implementation of
Enabled
was designed when wrapping processors was also needed for implementing log record mutations during processing as log records were passed by value (and not by pointer).This approach should make adding custom filtering easier for the users in most cases (see the updated
ExampleFilterer
).There is a little (mostly CPU) performance overhead caused by looping over filterers in
logger.Emit
, but I do not find it significant.