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.

Sign up for GitHub

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

Jump to bottom

core: change Interest-combining to play nicer with multiple subscribers #927

Merged
merged 7 commits into from
Aug 13, 2020
Merged

core: change Interest-combining to play nicer with multiple subscribers #927

merged 7 commits into from
Aug 13, 2020

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Aug 13, 2020

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 Interests. 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 Interests so that always
is only returned if both Interests 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!).

@hawkw hawkw requested a review from yaahc August 13, 2020 17:43
@hawkw hawkw requested a review from a team as a code owner August 13, 2020 17:43
@hawkw hawkw self-assigned this Aug 13, 2020
hawkw added 7 commits August 13, 2020 11:07
@hawkw
add tests for filter leaks across subscribers
7833f81 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
fix tests
12550d1 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
core: fix incorrect Interest combining
0b31096 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
core: make Interest::and simpler
38967a7 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
subscriber: make EnvFilter handle spans in enabled
e9f537f 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
rm dbg
2040ccd 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
fix unnecessary clone
8eae391 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw force-pushed the eliza/filter-leak-test branch from 6d1b96e to 8eae391 Compare August 13, 2020 18:09
davidbarsky
davidbarsky approved these changes Aug 13, 2020
View reviewed changes
Copy link
Member

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving since this was already approved!

@hawkw hawkw merged commit 2a5da8d into master Aug 13, 2020
hawkw added a commit that referenced this pull request Aug 21, 2020
@hawkw
core: prepare to release 0.1.15
a25848c 
Fixed

- When combining `Interest` from multiple subscribers, if the interests
  differ, the current subscriber is now always asked if a callsite
  should be enabled (#927)

Added

- Internal API changes to support optimizations in the `tracing` crate
  (#943)
- **docs**: Multiple fixes and improvements (#913, #941)
@hawkw hawkw mentioned this pull request Aug 21, 2020
Merged
hawkw added a commit that referenced this pull request Aug 23, 2020
@hawkw
core: prepare to release 0.1.15 (#951)
0454d6b 
### Fixed

- When combining `Interest` from multiple subscribers, if the interests
  differ, the current subscriber is now always asked if a callsite
  should be enabled (#927)

### Added

- Internal API changes to support optimizations in the `tracing` crate
  (#943)
- **docs**: Multiple fixes and improvements (#913, #941)
hawkw added a commit that referenced this pull request Sep 8, 2020
@hawkw
subscriber: prepare to release 0.1.12
fbff9d4 
Fixed

- **env-filter**: Fixed a regression where `Option<Level>` lost its
  `Into<LevelFilter>` impl ([#966])
- **env-filter**: Fixed `EnvFilter` enabling spans that should not be
  enabled when multiple subscribers are in use ([#927])

Changed

- **json**: `format::Json` now outputs fields in a more readable order
  ([#892])
- Updated `tracing-core` dependency to 0.1.16

Added

- **fmt**: Add `BoxMakeWriter` for erasing the type of a `MakeWriter`
  implementation ([#958])
- **fmt**: Add `TestWriter` `MakeWriter` implementation to support
  libtest output capturing ([#938])
- **layer**: Add `Layer` impl for `Option<T> where T: Layer` ([#910])
- **env-filter**: Add `From<Level>` impl for `Directive` ([#918])
- Multiple documentation fixes and improvements

Thanks to @Pothulapati, @samrg472, @bryanburgers, @keetonian, and
@SriRamanujam for contributing to this release!

[#927]: #927
[#966]: #966
[#958]: #958
[#892]: #892
[#938]: #938
[#910]: #910
[#918]: #918
@hawkw hawkw mentioned this pull request Sep 8, 2020
Merged
hawkw added a commit that referenced this pull request Sep 9, 2020
@hawkw
subscriber: prepare to release 0.2.12 (#970)
3dd5c03 
# 0.2.12 (September 8, 2020)

### Fixed

- **env-filter**: Fixed a regression where `Option<Level>` lost its
  `Into<LevelFilter>` impl ([#966])
- **env-filter**: Fixed `EnvFilter` enabling spans that should not be enabled
  when multiple subscribers are in use ([#927])
  
### Changed

- **json**: `format::Json` now outputs fields in a more readable order ([#892])
- Updated `tracing-core` dependency to 0.1.16

### Added

- **fmt**: Add `BoxMakeWriter` for erasing the type of a `MakeWriter`
  implementation ([#958])
- **fmt**: Add `TestWriter` `MakeWriter` implementation to support libtest
  output capturing ([#938])
- **layer**: Add `Layer` impl for `Option<T> where T: Layer` ([#910])
- **env-filter**: Add `From<Level>` impl for `Directive` ([#918])
- Multiple documentation fixes and improvements

Thanks to @Pothulapati, @samrg472, @bryanburgers, @keetonian, 
and @SriRamanujam for contributing to this release!

[#927]: #927
[#966]: #966
[#958]: #958
[#892]: #892
[#938]: #938
[#910]: #910
[#918]: #918
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidbarsky davidbarsky davidbarsky approved these changes

@yaahc yaahc Awaiting requested review from yaahc

Assignees

@hawkw hawkw

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hawkw @davidbarsky