-
Notifications
You must be signed in to change notification settings - Fork 85
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
MetricsLayer
forces callsites to register as Interest::sometimes()
since it is not a Filtered
#111
Comments
I am the one who introduced MetricsFilter to MetricsLayer. Thank you for finding the bug. Initially, I considered an API like fn metrics_layer<M,S>(m: M) -> impl Layer<S>
where
M: MeterProvider,
S: Subscriber + for<'span> LookupSpan<'span> { } but I became concerned that if I wanted to add settings to MetricsLayer, I wouldn't be able to write code like Ideally, users could filter metrics events without being aware of it. However, if that's not possible, I thought it might be acceptable to make MetricsFilter public and use |
I think this should be fixable by implementing I tried to poke around, but a random event I tried already seems to return |
Sorry, just coming back to this comment. It would be fixable through The problem here is when you have multiple per-layer filters that would all agree on a This causes a very large overhead as |
The
MetricsLayer
structure, which wraps an innerFiltered
itself implementsLayer
.The
Filtered
structure always returnsalways
and let's theRegistry
decide on the "combined" final interest. However, in this case, while theMetricsLayer
does callinner.register_callsite
, theMetricsLayer
itself is not aFiltered
.tracing-subscriber
is not able to understand thatMetricsLayer
is in fact a per-layer filter, and thisalways
is actually treated as a layer returningalways
. In turn, the registry decides to returnsometimes()
for the callsite (https://github.com/tokio-rs/tracing/blob/0e3577f6f3995b92accee21e0737c25ef0f1953c/tracing-subscriber/src/subscribe/layered.rs#L463-L472)I believe this is a bug. I'd be happy to contribute a fix to this but I'd love to discuss how to best design this. One option is to let the users build the filtered themselves. The other is to have some sort of API that returns an actual
Filtered
. I'm not exactly sure if there's a way to make this work through the Layer implementation, maybe usingon_layer
?Using
instrument_layer.with_filter(MetricsFilter)
directly fixes the issue, but both these structures are private.The text was updated successfully, but these errors were encountered: