Skip to content
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

Open
xuorig opened this issue Mar 1, 2024 · 3 comments

Comments

@xuorig
Copy link

xuorig commented Mar 1, 2024

The MetricsLayer structure, which wraps an inner Filtered itself implements Layer.

The Filtered structure always returns always and let's the Registry decide on the "combined" final interest. However, in this case, while the MetricsLayer does call inner.register_callsite, the MetricsLayer itself is not a Filtered.

tracing-subscriber is not able to understand that MetricsLayer is in fact a per-layer filter, and this always is actually treated as a layer returning always. In turn, the registry decides to return sometimes() 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 using on_layer?

Using instrument_layer.with_filter(MetricsFilter) directly fixes the issue, but both these structures are private.

@ymgyt
Copy link
Contributor

ymgyt commented Mar 23, 2024

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 MetricsLayer::new().with_foo(). So, I decided to return MetricsLayer instead.

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 MetricsLayer::new().with_filter(MetricsFilter).

@mladedav
Copy link
Contributor

mladedav commented Jun 15, 2024

I think this should be fixable by implementing downcast_raw. tracing_subscriber uses that to find out if something has filters and Filtered can work with that.

I tried to poke around, but a random event I tried already seems to return Interest::sometimes(). Is there some externally-visible behaviour caused by this bug I could try to replicate in a test?

@xuorig
Copy link
Author

xuorig commented Sep 9, 2024

I think this should be fixable by implementing downcast_raw. tracing_subscriber uses that to find out if something has filters and Filtered can work with that.

Sorry, just coming back to this comment. It would be fixable through downcast_raw if MagicPlfDowncastMarker was public, but it's not, so I'm not sure if that would work in practice.

The problem here is when you have multiple per-layer filters that would all agree on a never callsite, the MetricsLayer currently contributes an always (which is normal for Filtered structures, since they instead give their interest throught the FILTERED static), but since the registry / layered struct does not know that it is a Filtered, the always is taken as if it wasn't a per-layer filter, and the combined interest is returned as sometimes rather than never if it was an actual Filtered.

This causes a very large overhead as enabled now starts being called on every event/span for these callsites.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants