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

Do you use LevelFilter with < or <=? #1993

Closed
CAD97 opened this issue Mar 17, 2022 · 5 comments · Fixed by #2767
Closed

Do you use LevelFilter with < or <=? #1993

CAD97 opened this issue Mar 17, 2022 · 5 comments · Fixed by #2767

Comments

@CAD97
Copy link
Contributor

CAD97 commented Mar 17, 2022

Bug Report

Version

│   └── tracing v0.1.32
│       ├── tracing-attributes v0.1.20 (proc-macro)
│       └── tracing-core v0.1.23
├── tracing v0.1.32 (*)
├── tracing-appender v0.2.1
│   └── tracing-subscriber v0.3.9
│       ├── tracing-core v0.1.23 (*)
│       └── tracing-log v0.1.2
│           └── tracing-core v0.1.23 (*)
└── tracing-subscriber v0.3.9 (*)

Platform

Microsoft Windows Version 21H2 (OS Build 19044.1586)

Crates

tracing-core

Description

When checking if a level is (statically) enabled, is it Level::TRACE < STATIC_MAX_LEVEL or Level::TRACE <= STATIC_MAX_LEVEL?

LevelFilter docs:

If a Level is considered less than a LevelFilter, it should be considered enabled; if greater than or equal to the LevelFilter, that level is disabled. See LevelFilter::current for more details.

LevelFilter::current docs:

If a given span or event has a level higher than the returned LevelFilter, it will not be enabled. However, if the level is less than or equal to this value, the span or event is not guaranteed to be enabled; the subscriber will still filter each callsite individually.

The type docs seem to say that is_enabled = level < filter and is_disabled = level >= filter.

The method docs seem to say that is_disabled = level > filter and is_enabled = level <= filter.

Which one is it?

@j4nk3e
Copy link

j4nk3e commented Nov 4, 2022

After reading the docs again, the only thing that makes sense is the 2nd one.
Otherwise, the LevelFilter::OFF would be useless since it would have the same impact as LevelFilter::Error and it would be impossible to have a filter that let's everything through:
https://github.com/tokio-rs/tracing/blob/68f830d392b3b2fc07704d107d223414a1291da4/tracing-core/src/metadata.rs#L231-#L233

@djc
Copy link
Contributor

djc commented Jan 30, 2023

FWIW, I also got mighty confused with the documentation for LevelFilter. Would be nice if that was fixed.

@maddiemort
Copy link

This inconsistency confused me today too. It seems like the docs for LevelFilter were incorrectly updated in response to #1669. To clarify:

#1669 pointed out that the docs said:

  • level < filter -> disabled
  • level >= filter -> enabled

The docs were updated to say:

  • level < filter -> enabled
  • level >= filter -> disabled

...but I think they should have been updated to say:

  • level <= filter -> enabled
  • level > filter -> disabled

@loganmzz
Copy link

loganmzz commented Aug 20, 2023

Just few notes:

  • level word is little bit confusing, but still catchable, here. May be effective, actual, current or recorded would have been better. And I also have changed from filter to filtering :)

  • Having recorded <= filtering = enabled seems too much close to internal representation (I suppose, not checked). In common opinion, levels are sorted from "criticity": TRACE (smallest) to ERROR (highest). ALL and OFF are just special meanings. It could be interpreted as negative and positive infinities (aka unreachable, aka uninstantiable).

@loganmzz
Copy link

Ok, I haven't checked PartialOrd impl. I agree with @nerosnm !

maddiemort added a commit to maddiemort/tracing that referenced this issue Oct 19, 2023
These docs were updated (in response to tokio-rs#1669), but the "or equal to"
phrase should have been moved to the other case.

Fixes: tokio-rs#1993
hawkw pushed a commit that referenced this issue Oct 19, 2023
These docs were updated (in response to #1669), but the "or equal to"
phrase should have been moved to the other case.

Fixes: #1993

## Motivation

When these docs were updated in response to #1669, they were updated
incompletely (a level that is equal to a filter is _enabled_, not
_disabled_) and therefore remained incorrect.

## Solution

The docs were updated, moving the "or equal to" phrase to the other
case.
davidbarsky pushed a commit that referenced this issue Nov 7, 2023
These docs were updated (in response to #1669), but the "or equal to"
phrase should have been moved to the other case.

Fixes: #1993

## Motivation

When these docs were updated in response to #1669, they were updated
incompletely (a level that is equal to a filter is _enabled_, not
_disabled_) and therefore remained incorrect.

## Solution

The docs were updated, moving the "or equal to" phrase to the other
case.
hawkw pushed a commit that referenced this issue Nov 7, 2023
These docs were updated (in response to #1669), but the "or equal to"
phrase should have been moved to the other case.

Fixes: #1993

## Motivation

When these docs were updated in response to #1669, they were updated
incompletely (a level that is equal to a filter is _enabled_, not
_disabled_) and therefore remained incorrect.

## Solution

The docs were updated, moving the "or equal to" phrase to the other
case.
kaffarell pushed a commit to kaffarell/tracing that referenced this issue Nov 21, 2023
…-rs#2767)

These docs were updated (in response to tokio-rs#1669), but the "or equal to"
phrase should have been moved to the other case.

Fixes: tokio-rs#1993

## Motivation

When these docs were updated in response to tokio-rs#1669, they were updated
incompletely (a level that is equal to a filter is _enabled_, not
_disabled_) and therefore remained incorrect.

## Solution

The docs were updated, moving the "or equal to" phrase to the other
case.
kaffarell pushed a commit to kaffarell/tracing that referenced this issue Feb 14, 2024
…-rs#2767)

These docs were updated (in response to tokio-rs#1669), but the "or equal to"
phrase should have been moved to the other case.

Fixes: tokio-rs#1993

## Motivation

When these docs were updated in response to tokio-rs#1669, they were updated
incompletely (a level that is equal to a filter is _enabled_, not
_disabled_) and therefore remained incorrect.

## Solution

The docs were updated, moving the "or equal to" phrase to the other
case.
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

Successfully merging a pull request may close this issue.

5 participants