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

chore: backport roughly a year's worth of changes #2728

Merged
merged 34 commits into from
Oct 1, 2023

Conversation

@davidbarsky davidbarsky changed the base branch from master to v0.1.x September 26, 2023 00:15
hds and others added 6 commits September 26, 2023 14:02
The `tracing-mock` crate provides a mock collector (and a subscriber for
use by the tests in the `tracing-subscriber` crate) which is able to
make assertions about what diagnostics are emitted.

These assertions are defined by structs that match on events, span, and
their fields and metadata. The structs that matched these objects have
been called, up until now, mocks, however this terminology may be
misleading, as the created objects don't mock anything.

There were two different names for similar functionality with `only()`
and `done()` on fields and collectors/subscribers respectively. Using a
single name for these may make it easier to onboard onto `tracing-mock`.

To reduce confusion, these structs have been split into two categories:
mocks and expectations.

Additionally, the `done()` function on the `Collector` and `Subscriber`
mocks has been replaced with `only()`. This matches the similar function
for `ExpectedField`, and may be more intuitive.

The mocks replace some component in the tracing ecosystem when a library
is under test. The expectations define the assertions we wish to make
about traces received by the mocks.

Mocks (per module):
* collector - `MockCollector`, no change
* subscriber - `MockSubscriber`, renamed from `ExpectSubscriber`

Expectations (per module):
* event - `ExpectedEvent`, renamed from `MockEvent`
* span - `ExpectedSpan`, renamed from `MockSpan`
* field - `ExpectedField` and `ExpectedFields`, renamed from `MockField`
  and `Expected`. Also `ExpectedValue` renamed from `MockValue`.
* metadata - `ExpectedMetadata`, renamed from `Expected`

Refs: #539
* mock: change helper functions to `expect::<thing>`

The current format of test expectations in `tracing-mock` isn't ideal.
The format `span::expect` requires importing `tracing_mock::<thing>` which
may conflict with imports from other tracing crates, especially
`tracing-core`.

So we change the order and move the functions into a module called
`expect` so that:
* `event::expect` becomes `expect::event`
* `span::expect` becomes `expect::span`
* `field::expect` becomes `expect::field`

This format has two advantages.
1. It reads as natural English, e.g "expect span"
2. It is no longer common to import the modules directly.

Regarding point (2), the following format was previously common:

```rust
use tracing_mock::field;

field::expect();
```

This import of the `field` module may then conflict with importing the
same from `tracing_core`, making it necessary to rename one of the
imports.

The same code would now be written:

```rust
use tracing_mock::expect;

expect::field();
```

Which is less likely to conflict.

This change also fixes an unused warning on `MockHandle::new` when the
`tracing-subscriber` feature is not enabled.

Refs: #539
There has been interest around publishing `tracing-mock` to crates.io
for some time. In order to make this possible, documentation and some
code clean up is needed.

Specifically **I** want to have access to `tracing-mock` within parts of
`tokio` to write tests that ensure that the correct calling location is
picked up for all `spawn*` functions when the `tracing` feature is
enabled.

This change starts that process by adding a README for `tracing-mock`.
The README follows the standard format for all `tracing` crates and
includes 2 examples. The README is included in the `lib.rs` docs using
`#[doc = include_str!(...)]`, so that the same documentation is included
in the crate, and the examples are tested when running doctests.

The README describes steps when using the `tracing` 1.0 from crates.io
and `tracing-mock` from the `v0.1.x` branch. The tests target the
current branch.

Co-authored-by: David Barsky <[email protected]>
Co-authored-by: Eliza Weisman <[email protected]>
The error message suggesting that you remove a semicolon to return a
value (instead of unit) was updated slightly in rust-lang/rust#102650,
which was included in Rust 1.66.0.

This causes one of the tests in tracing-attributes to fail. We fix it by
using the updated error message.
There has been interest around publishing tracing-mock to crates.io
for some time. In order to make this possible, documentation and some
code clean up is needed.

The `event` module needs documentation and examples.

This change adds documentation to the event module and all the public
APIs within it. This includes doctests on all the methods which serve as
examples.

The following pattern was applied to the description of most methods:
- Short description of expectation
- Additional clarification (where needed)
- Description of cases that cause the expectation to fail
- Examples
  - Successful validation
  - Unsuccesful validation

Two changes were also made in the text provided to the user when an
assertion fails for `with_explicit_parent` or `with_contextual_parent`.

One small API changes is also included:

The method `in_scope` has been placed behind the `tracing-subscriber`
feature flag as it currently only works with the `MockSubscriber`, not
with the `MockCollector`. If the feature flag is active and it is used
to set a non-empty scope, the `MockCollector` will panic with
`unimplemented` during validation.

Refs: #539
There has been interest around publishing `tracing-mock` to crates.io
for some time. In order to make this possible, documentation and some
code clean up is needed.

The `subscriber` module needs documentation and examples.

This change adds documentation to the `subscriber` module and all the public
APIs within it. This includes doctests on all the methods which serve as
examples.

The `MockSubscriberBuilder::record` method was removed as its
functionality is not implemented.

Previously, the `MockSubscriber` would verify the scope of an
`ExpectedEvent`, even if `in_scope` hadn't been called. In this case,
that would check that an event was not in a span if `in_scope` had not
been called. `tracing-subscriber` all adhere to this pattern. However it
is different to the behavior of all other expectation methods, where an
explicit call is needed to expect something, otherwise nothing is
checked. As such, the behavior has been modified to align with the rest
of the crate. The previous behavior can be achieved by calling
`in_scope(None)` to verify that an event has no scope. The documentation
for `in_scope` has been updated with an example for this case.

The tests in `tracing-subscriber` which previously verified *implicitly*
that an event had no scope (by not calling `in_scope` at all) have *not*
been modified. It is my opinion that this implicit behavior was never
required.

Refs: #539
@davidbarsky davidbarsky force-pushed the davidbarsky/backport-changes branch from 10d1084 to 7a5e7d3 Compare September 27, 2023 00:07
@davidbarsky davidbarsky changed the title Backport lotsa changes chore: backport roughly a year's worth of changes Sep 27, 2023
@hawkw
Copy link
Member

hawkw commented Sep 27, 2023

I'm pretty sure some of the commits in here were not backported because they weren't relevant to the v0.1.x branch. For example, 394207a relates to the custom CSS we add to the rustdoc on v0.2.x to indicate that it's not the release version of tracing, which doesn't need to be on v0.1.x.

@hawkw
Copy link
Member

hawkw commented Sep 27, 2023

I'm also not really sure if 9d98bb5 needed to be backported: it reverted a commit that we never merged to v0.1.x, so now it just changes some docs link references.

At a glance, those are the only commits that I think didn't need to be backported.

@davidbarsky
Copy link
Member Author

Thanks, I'll remove those commits from this branch. https://github.com/davidbarsky/tracing-patcher might've been a little too aggressive.

arifd and others added 3 commits September 27, 2023 11:08
As part of upgrading syn to 2.0 (e.g.,
#2516), we need to bump the MSRV
to 1.56. As part of this PR, I've:
- Updated the text descriptions of what would be an in-policy MSRV bump
  to use more recent versions of rustc. The _niceness_ of said version
  numbers are purely coincidental.
- I've removed some of the exceptions made in CI.yml in order to support
  some crates with a higher MSRV.
This PR adds two new accessor functions that are useful for creating a
structured serde implementation for tracing.

This is a sort of "distilled" version of
#2113, based on the `v0.1.x`
branch.

As it is unlikely that "structured serde" will be 1:1 compatible with
the existing JSON-based `tracing-serde` (or at least - I'm not sure how
to do it in a reasonable amount of effort), these functions will allow
me to make a separate crate to hold me over until breaking formatting
changes are possible in `tracing-serde`.

CC @hawkw, as we've discussed this pretty extensively
@davidbarsky davidbarsky force-pushed the davidbarsky/backport-changes branch 2 times, most recently from 7ecb3fa to c6d96fd Compare September 27, 2023 18:46
@davidbarsky
Copy link
Member Author

davidbarsky commented Sep 27, 2023

Quick updates:

  • I've removed 394207a and 9d98bb5 from this branch.
  • fixed the tracing-journald compilation issues (didn't catch this because it was a clean rebase and I'm on a Mac, so it didn't compile in the first place).
  • Fixed 8372ba3 to ensure that its tests reference fmt::Layer instead of fmt::Subscriber, which has a very different meaning on v0.1.x.
    • I also changed 8372ba3's commit message to reference fmt::Layer instead of fmt::Subscriber to make it a little less confusing, but I can undo this change if it's a little too fishy.
  • I've missed a necessary change in f23de16 while rebasing, but I'll fix that after I post this comment (EDIT: now fixed. __CALLSITE is now used everywhere.)

@davidbarsky davidbarsky force-pushed the davidbarsky/backport-changes branch from c6d96fd to a023c42 Compare September 27, 2023 19:01
@davidbarsky
Copy link
Member Author

CI is currently failing due to tracing/tests/macros_redefined_core.rs's usage of extern crate self as core;. I've made a minimal reproduction.

@davidbarsky
Copy link
Member Author

@hawkw: I'm not sure when tracing/tests/macros_redefined_core.rs started causing issues, as building against different commits in this this branches's history still caused that compilation issue. To unblock/surface any other issues, I'm going to remove the test for now and try to figure it out later.

@steveklabnik (feel free to unsubscribe yourself, Steve! Just trying to give credit) suggested that it might be a lookup bug in rustc, noting that extern crate self as core is deeply fucked up and the compiler assumes that nobody is gonna add a crate named core without also doing no_core. However, I compiled my branch with 1.56.0 (with some version patching, of course) and still encountered that compilation issue, so I'm not sure it was a recent regression.

@steveklabnik
Copy link
Contributor

Just to be 1000% clear that was a guess of mine, I don't actually know if the compiler makes assumptions around core, just that it wouldn't shock me if it did.

hds and others added 8 commits September 27, 2023 14:57
There has been interest around publishing `tracing-mock` to crates.io
for some time. In order to make this possible, documentation and some
code clean up is needed.

This change adds documentation to the collector module itself and to all the
public APIs in the module. This includes doctests on all the methods
that serve as examples.

Additionally the implementation for the `Expect` struct has been moved
into the module with the definition, this was missed in #2369.

Refs: #539
…std prelude (#2621)

Currently, in many places, macros do not use fully qualified names for
items exported from the prelude. This means that naming collisions
(`struct Some`) or the removal of the std library prelude will cause
compilation errors.

- Identify and use fully qualified names in macros were we previously
  assumed the Rust std prelude. We use `::core` rather than `::std`.
- Add
  [`no_implicit_prelude`](https://doc.rust-lang.org/reference/names/preludes.html#the-no_implicit_prelude-attribute)
  to `tracing/tests/macros.rs`. I'm unsure if this is giving us good
  coverage - can we improve on this approach? I'm not confident I've
  caught everything.
The test relies on TEST_CALLSITE_1 and TEST_CALLSITE_2 to have
different addresses. However, as they are zero-sized types, this
is not guaranteed.

This fixes the test failure with LLVM 17 and certain optimization
options reported at rust-lang/rust#114699.
## Motivation

Sharing tooling.

## Solution

Adds `clippy-tracing` to related crates.

Closes #2627
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

overall, this looks good. i noticed some accidental uses of the tracing v0.2 type names in the tests, can we change those back to the v0.1.x names?

other than that, this looks great to me!

tracing-subscriber/src/fmt/fmt_layer.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/fmt/fmt_layer.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/fmt/fmt_layer.rs Show resolved Hide resolved
tracing-subscriber/tests/layer_filters/main.rs Outdated Show resolved Hide resolved
tracing-subscriber/tests/layer_filters/option.rs Outdated Show resolved Hide resolved
tracing-subscriber/tests/layer_filters/option.rs Outdated Show resolved Hide resolved
tracing-subscriber/tests/layer_filters/option.rs Outdated Show resolved Hide resolved
tracing/tests/no_subscriber.rs Outdated Show resolved Hide resolved
@davidbarsky
Copy link
Member Author

I've noticed a few more mentions of 0.2 naming after pushing, sorry. I'll fix those up.

dmlary and others added 16 commits September 29, 2023 16:41
It's necessary at times to be able to disable ANSI color output for
rust utilities using `tracing`.  The informal standard for this is the
`NO_COLOR` environment variable described here: https://no-color.org/

Further details/discussion in #2388

This commit updates `fmt::Layer` to check the `NO_COLOR`
environment variable when determining whether ANSI color output is
enabled by default. As described in the spec, any non-empty value set
for `NO_COLOR` will cause ANSI color support to be disabled by default.

If the user manually overrides ANSI color support, such as by calling
`with_ansi(true)`, this will still enable ANSI colors, even if
`NO_COLOR` is set. The `NO_COLOR` env var only effects the default
behavior.

Fixes #2388
This branch fixes a handful of new warnings which have shown up after
updating to Rust 1.72.0.

This includes:

* `clippy::redundant_closure_call` in macros --- allowed because the
  macro sometimes calls a function that isn't a closure, and the closure
  is just used in the case where it's not a function.
* Unnecessary uses of `#` in raw string literals that don't contain `"`
  characters.
* Dead code warnings with specific feature flag combinations in
  `tracing-subscriber`.

In addition, I've fixed a broken RustDoc link that was making the
Netlify build sad.
## Motivation

The instrument macro currently doesn't work with the "log" crate
feature: #2585

## Solution

Change the generated code to create a span if either
`tracing::if_log_enabled!` or `tracing::level_enabled!`. I'm not sure
how to add a test for this or if this is the best solution.

Fixes #2585
## Motivation

There are a few errors in the file appender docs - this fixes them.

It also wasn't clear/apparent to me that you can create a non-rolling
file appender with the `rolling` module - this calls that out more
clearly.

## Solution

Updates to docs.
## Motivation

The `.folded` format expects a `;`-separated list of the stack function,
optionally followed up by a sample count.

The implementation before this commit added a blank space after each `;`
which made parsers, such as `inferno-flamegraph` mis-interpret the data.

Furthermore, normally one wouldn't expect the filename and line-number
in such stack trace.

## Solution

Remove white-space between `;` for the generated file and remove
filename and line-number by default.
…2709)

## Motivation

`#[tracing::instrument]` uses `unreachable!()` macro which needlessly
expands to panicking and formatting code. It only needs any `!` type.

## Solution

`loop {}` works just as well for a `!` type, and it crates less work for
the compiler. The code is truly unreachable, so the message would never
be useful. Rust used to be concerned about semantics of empty loops in
LLVM, but this [has been solved](https://reviews.llvm.org/D85393).
## Motivation

Fixes: #1566

## Solution

This change removes the maximum of 32 fields limitation using const
generics.

Having this arbitrary restriction in place to prevent stack overflows
feels a little misplaced to me since stack size varies between
environments.
## Motivation

The motivation is #1426.  Currently, event names are set to a default
value of `event file:line`, which (1) doesn't allow for customization,
and (2) prevents events from working for some opentelemetry endpoints
(they often use the event name `exception` to designate something like a
panic).

## Solution

Went through the event macros, and added new parameterization that
allows for setting the `name`.  In addition, added some comments, and
reordering, to make life a bit better for the next person that comes
along to edit those macros.  Finally, added tests for the macro
expansion alongside the existing tests with a reasonable amount of
coverage (though, admittedly, more could be added for all of the macro
invocation types

Fixes #1426
It's currently not possible to customize how messages will get send to journald.

This became apparent in #2425, where first a specific API got designed, but then
it was decided that users should not get restricted in only a subset of fields,
but should be able to simply choose by themselves what fields get set with what
values.

So in a sense, this is the successor/rework of #2425.

Allow custom fields to be set in tracing-journald.

- [x] How should we deal with fields that also get supplied by other options?
  For example, setting `SYSLOG_IDENTIFIER` here and also setting
  `.with_syslog_identifier()` will send said field twice, potentially with
  differing values. Is that a problem?
    - Answer: No, this is not a problem.

Closes #2425
I've found myself in the case where I wanted to have customized event field name
for different trait implementations. In fact, these implementations are
completely unrelated (in separate applications), so, in this use case, I find
more readable to have `foo="some_id"` and `bar=16` instead of `resource="foo"
value="some_id"` and `resource=bar value=16`

Because events only accept identifier or literal as field name, this is quite
cumbersome/impossible to do. A simple solution could be to make events accept
constant expression too; in my use case, I could then add a associated constant
to my trait.

This PR proposes a new syntax for using constant field names:
```rust
tracing::debug!({ CONSTANT_EXPR } = "foo");
```
This is the same syntax than constant expression, so it should be quite intuitive.

To avoid constant expression names conflict, internal variables of macro
expansion have been prefixed with `__`, e.g. `__CALLSITE`.

Co-authored-by: Joseph Perez <[email protected]>
Co-authored-by: Eliza Weisman <[email protected]>
## Motivation

Adding a relevant library to the list of `tracing`-enabled crates.

## Solution

Added to READMEs and documentation.
…2690)

Issue #2080 explains that it's not
possible to soundly use
[`tracing_subscriber::fmt::time::LocalTime`](https://docs.rs/tracing-subscriber/latest/tracing_subscriber/fmt/time/struct.LocalTime.html)
in a multithreaded context. It proposes adding alternative time formatters that
use the [chrono crate](https://docs.rs/chrono/latest/chrono/) to workaround
which is what this PR offers.

A new source file 'chrono_crate.rs' is added to the 'tracing-subscriber'
package implementing `mod chrono_crate` providing two new tag types `LocalTime`
and `Utc` with associated `time::FormatTime` trait implementations that call
`chrono::Local::now().to_rfc3339()` and `chrono::Utc::now().to_rfc3339()`
respectively. Simple unit-tests of the new functionality accompany the
additions.
---------

Co-authored-by: David Barsky <[email protected]>
Co-authored-by: Shayne Fletcher <[email protected]>
As part of landing #2728, I noticed that the `-Zminimal-versions` check fails
due to proc-macro2 1.0.40 referencing a since-removed, nightly-only
feature (`proc_macro_span_shrink`). Since this change doesn't change the MSRV
of `proc-macro2` (or `tracing`, for that matter), this feels like a safe change
to make.
It's currently awkward to have an optional per-layer filter.

Implement `Filter<L>` for `Option<F> where F: Filter<L>`, following the example
of `Layer`. A `None` filter passes everything through.

Also, it looks like Filter for Arc/Box doesn't pass through all the methods, so
extend the `filter_impl_body` macro to include them.

This also adds a couple of tests and updates the docs.

---------

Co-authored-by: David Barsky <[email protected]>
Co-authored-by: Ryan Johnson <[email protected]>
Co-authored-by: Eliza Weisman <[email protected]>
@davidbarsky davidbarsky force-pushed the davidbarsky/backport-changes branch from edc911f to aa51104 Compare September 29, 2023 23:44
@davidbarsky
Copy link
Member Author

I've noticed a few more mentions of 0.2 naming after pushing, sorry. I'll fix those up.

Alright, I think I got rid of any 0.2 terminology.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

looks great, thank you so much!

@hawkw hawkw merged commit 7f0cc09 into v0.1.x Oct 1, 2023
53 checks passed
@hawkw hawkw deleted the davidbarsky/backport-changes branch October 1, 2023 17:46
hawkw pushed a commit that referenced this pull request Oct 1, 2023
hawkw pushed a commit that referenced this pull request Oct 1, 2023
As part of landing #2728, I noticed that the `-Zminimal-versions` check fails
due to proc-macro2 1.0.40 referencing a since-removed, nightly-only
feature (`proc_macro_span_shrink`). Since this change doesn't change the MSRV
of `proc-macro2` (or `tracing`, for that matter), this feels like a safe change
to make.
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
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 this pull request may close these issues.