-
Notifications
You must be signed in to change notification settings - Fork 701
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
core: change Interest
-combining to play nicer with multiple subscribers
#920
Conversation
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
cc @jeromegn, this should fix the filter leaking you reported in #902. However, we still need to figure out a better strategy for invalidating the callsite cache at the end of a subscriber lifetime, for performance reasons. This change fixes incorrect filtering by ensuring that the current subscriber is always asked whether it will enable a span/event when multiple subscribers disagree on whether or not it should be enabled. However, this means that if we have two subscribers, and one is dropped, we will continue asking the current subscriber for every callsite, even if it would rather return a cached interest, so that we can skip filtering. |
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
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.
looks good, sounds good
// Iterate over the subscribers in the registry, and — if they are | ||
// active — register the callsite with them. | ||
let mut interests = self | ||
.dispatchers | ||
.iter() | ||
.filter_map(|registrar| registrar.try_register(meta)); | ||
|
||
// Use the first subscriber's `Interest` as the base value. | ||
let interest = if let Some(interest) = interests.next() { | ||
// Combine all remaining `Interest`s. | ||
interests.fold(interest, Interest::and) | ||
} else { | ||
// If nobody was interested in this thing, just return `never`. | ||
Interest::never() | ||
}; |
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.
👍
…bers (#927) ## Motivation Currently, when multiple subscribers are in use, the way that `Interest` values are combined results in surprising behavior. If _any_ subscriber returns `Interest::always` for a callsite, that interest currently takes priority over any other `Interest`s. This means that if two threads have separate subscribers, one of which enables a callsite `always`, and the other `never`, _both_ subscribers will always record that callsite, without the option to disable it. This is not quite right. Instead, we should check whether the current subscriber will enable that callsite in this case. This issue is described in #902. ## Solution This branch changes the rules for combining `Interest`s so that `always` is only returned if _both_ `Interest`s are `always`. If only _one_ is `always`, the combined interest is now `sometimes`, instead. This means that when multiple subscribers exist, `enabled` will always be called on the _current_ subscriber, except when _all_ subscribers have indicated that they are `always` or `never` interested in a callsite. I've added tests that reproduce the issues with leaky filters. Fixing this revealed an additional issue where `tracing-subscriber`'s `EnvFilter` assumes that `enabled` is only called for events, and never for spans, because it always returns either `always` or `never` from `register_callsite` for spans. Therefore, the dynamic span matching directives that might enable a span were never checked in `enabled`. However, under the new interest-combining rules, `enabled` *may* still be called even if a subscriber returns an `always` or `never` interest, if *another* subscriber exists that returned an incompatible interest. This PR fixes that, as well. Depends on #919 This was previously approved by @yaahc as PR #920, but I accidentally merged it into #919 _after_ that branch merged, rather than into master (my bad!). Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Motivation
Currently, when multiple subscribers are in use, the way that
Interest
values are combined results in surprising behavior.
If any subscriber returns
Interest::always
for a callsite, thatinterest currently takes priority over any other
Interest
s. This meansthat if two threads have separate subscribers, one of which enables a
callsite
always
, and the othernever
, both subscribers will alwaysrecord that callsite, without the option to disable it. This is not
quite right. Instead, we should check whether the current subscriber
will enable that callsite in this case.
Solution
This branch changes the rules for combining
Interest
s so thatalways
is only returned if both
Interest
s arealways
. If only one isalways
, the combined interest is nowsometimes
, instead. This meansthat when multiple subscribers exist,
enabled
will always be called onthe current subscriber, except when all subscribers have indicated
that they are
always
ornever
interested in a callsite.I've added tests that reproduce the issues with leaky filters.
Fixing this revealed an additional issue where
tracing-subscriber
'sEnvFilter
assumes thatenabled
is only called for events, and neverfor spans, because it always returns either
always
ornever
fromregister_callsite
for spans. Therefore, the dynamic span matchingdirectives that might enable a span were never checked in
enabled
.However, under the new interest-combining rules,
enabled
may stillbe called even if a subscriber returns an
always
ornever
interest,if another subscriber exists that returned an incompatible interest.
This PR fixes that, as well.
Depends on #919