subscriber: fix level hint not considering value filters #907
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hawkw
added
kind/bug
This fixes an issue with `EnvFilter`'s `max_level_hint` not considering that span field value filters may enable *any* span, regardless of the level enabled inside that span. This is because the field value must be recorded to be known. We may wish to change this behavior to only check spans up to the selected level, but this isn't what it does currently. This branch fixes the wrong max level hint. This bug was not caught due to an issue where hinting was too permissive, but fixing that bug uncovers this issue. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
force-pushed
the
eliza/fix-field-filter-hints
branch
from
e1c8642
to
a88b955
Compare
yaahc
approved these changes
Aug 10, 2020
yup, #908 depends on this, since fixing that bug uncovered this bug. |
hawkw
added a commit
that referenced
this pull request
Aug 10, 2020
## Motivation Currently, when rebuilding the interest cache, the max level is calculated by tracking a max level, and comparing it with each active `Subscriber`'s max level hint, and setting the max value to the hint if the hint is higher than the current value. This is correct. However, the max level in this calculation starts at `TRACE`. This means that regardless of what any subscriber returns for its hint, after rebuilding the interest cache, the max level will *always* be `TRACE`. This is wrong — it means that the fast path will not be taken in may cases where it *should* be. The max level calculation was started at `TRACE` rather than `OFF`, because when this code was written with it starting at `OFF`, all the tests broke, because the test subscribers don't provide hints. This was the incorrect solution to that problem, however. This caused the tests to break because we were *ignoring* all `None` hints, and not changing the current max value when a subscriber returns `None`. This means that if all subscribers returned `None` for their max level hint, the max would remain `OFF`. However, what we *should* have done is assume that if a subscriber doesn't provide a hint, it won't be filtering based on levels, and assume that the max level is `TRACE` for that subscriber. Whoopsie! ## Solution Now, the max level should be calculated correctly for subscribers that *do* provide hints, and should still be `TRACE` if a subscriber does not have a hint. I've also added new tests to ensure that the global max level filter is used when provided, by asserting that `enabled` is not called with spans that would be excluded by the max level. These tests fail on master, but pass after this change. Additionally, note that fixing this bug revealed a separate issue in `tracing-subscriber`'s implementation of `max_level_hint` for `EnvFilter`. When the `EnvFilter` includes directives that filter on span field values, it will enable all spans with fields with that name, so that events *within* the span can be enabled up to the filtering directive's level. This may not be the ideal behavior, but it's what we're doing currently, and we have tests for it. Therefore, it was necessary to change the `max_level_hint` implementation to take this into account by always returning `TRACE` when there are field value filters. That means that this branch requires #907 order for the `tracing-subscriber` tests to pass. Fixes: #906 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Aug 11, 2020
Fixed - **env-filter**: Incorrect max level hint when filters involving span field values are in use (#907) - **registry**: Fixed inconsistent span stacks when multiple registries are in use on the same thread (#901) Changed - **env-filter**: `regex` dependency enables fewer unused feature flags (#899) Thanks to @bdonlan and @jeromegn for contributing to this release!
hawkw
added a commit
that referenced
this pull request
Aug 11, 2020
### Fixed - **env-filter**: Incorrect max level hint when filters involving span field values are in use (#907) - **registry**: Fixed inconsistent span stacks when multiple registries are in use on the same thread (#901) ### Changed - **env-filter**: `regex` dependency enables fewer unused feature flags (#899) Thanks to @bdonlan and @jeromegn for contributing to this release!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes an issue with
EnvFilter'smax_level_hintnot consideringthat span field value filters may enable any span, regardless of the
level enabled inside that span. This is because the field value must be
recorded to be known.
We may wish to change this behavior to only check spans up to the
selected level, but this isn't what it does currently. This branch fixes
the wrong max level hint.
This bug was not caught due to an issue where hinting was too permissive
(#906), but fixing that bug uncovers this issue. A subsequent PR fixing
hint calculation introduces new test failures without this change.
Signed-off-by: Eliza Weisman eliza@buoyant.io