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

backport a bunch of stuff #2255

Merged
merged 11 commits into from
Jul 29, 2022
Merged

backport a bunch of stuff #2255

merged 11 commits into from
Jul 29, 2022

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Jul 29, 2022

IceSentry and others added 11 commits July 28, 2022 14:55
## Motivation

The `RollingFileAppender` currently only supports a filename suffix. A
lot of editors have support for log files using the `.log` extension. It
would be nice to be able to configure what gets added after the date.

## Solution

- Add a `Builder::filename_suffix` method, taking a string.
  - If the string is non-empty, this gets appended to the filename after
    the date.
  - This isn't an `AsRef<Path>` because it's not supposed to be a `Path`
- Update the date appending logic to handle cases when the suffix or
  prefix is empty
  - Split each part with a `.` so the final output would be
    `prefix.date.suffix`
  - Make sure to remove unnecessary `.` when a prefix or suffix is empty
-  Add tests related to those changes

## Notes

It would probably be nicer to simply have a completely configurable file
name format, but I went with the easiest approach that achieved what I
needed.

Closes #1477
## Motivation

Builders not marked `#[must_use]` can not be initialized sometimes,
causing silent failures.
Eg.
```rust
fn main() {
    tracing_subscriber::fmt();
    tracing::info!("hello");
}
```
won't print anything.

## Solution

Added `#[must_use]` to builder types in the tracing-subscriber crate.
Motivation:
Currently, there is no way to publish metrics via tracing.

Solution:
Update the tracing-opentelemetry crate to publish metrics for event
fields that contain specific prefixes in the name.

Right now, we lazily instantiate and store one metrics object
per-callsite, but a future improvement that we should add to tracing
itself is the ability to store data per-callsite, so that we don't have
to do a HashMap lookup each time we want to publish a metric.

Co-authored-by: Eliza Weisman <[email protected]>
Co-authored-by: David Barsky <[email protected]>
In the upstream `opentelemetry` crate, the `trace` and `metrics`
features are gated by separate feature flags. This allows users who are
only using OpenTelemetry for tracing, or who are only using it for
metrics, to pick and choose what they depend on.

Currently, the release version of `tracing-opentelemetry` only provides
tracing functionality, and therefore, it only depends on `opentelemetry`
with the `trace` feature enabled. However, the metrics support added in
#2185 adds a dependency on the `opentelemetry/metrics` feature. This is
currently always enabled. We should probably follow the same approach as
upstream `opentelemetry`, and allow enabling/disabling metrics and
tracing separately.

This branch adds a `metrics` feature to `tracing-opentelemetry`, and
makes the `MetricsLayer` from #2185 gated on the `metrics` feature.
This feature flag is on by default, like the upstream
`opentelemetry/metrics` feature, but it can be disabled using
`default-features = false`.

We should probably do something similar for the tracing components of
the crate, and make them gated on a `trace` feature flag, but adding a
feature flag to released APIs is not semver-compatible, so we should
save that until the next breaking release.
## Motivation

This makes it possible to fully "override" some base `Targets` filter
with another (e.g. user-supplied) filter. Without some way to obtain the
default, only explicit targets can be overridden (via `IntoIter` and
friends). 

See also #1790 (comment)

## Solution

We can add a method to `Targets` that filters the underlying
`DirectiveSet` for the default directive. This works because
`DirectiveSet::add` will replace directives with the same
`target`/`field_names`, which is always `None`/`vec![]` for the
directive added by `with_default` (and in fact we are only concerned
with `target`, since no other `Targets` API allows adding directives
with a `None` target).

Ideally the method would be named `default`, corresponding to
`with_default`, however this conflicts with `Default::default` and so
would be a breaking change (and harm ergonomics). `default_level` seemed
a better name than `get_default`, since "getters" of this style are
generally considered unidiomatic<sup>[citation needed]</sup>.

Example usage:

```rust
let mut filter = Targets::new().with_target("some_module", LevelFilter::INFO);

// imagine this came from `RUST_LOG` or similar
let override: Targets = "trace".parse().unwrap();

// merge the override
if let Some(default) = override.default_level() {
    filter = filter.with_default(default);
}
for (target, level) in override.iter() {
    filter = filter.with_target(target, level);
}
```

Closes #1790
These implementations mirror those provided by tracing-core for
`Collect` on `Box<C>` and `Arc<C>`.
## Motivation

I've wanted to serialize fieldset of current span.

## Solution

Expose already existing `SerializeFieldSet` for users by implementing `AsSerde` for `FieldSet`.
## Motivation

Like for `Subscriber` and `Layer`, allow per-layer `Filter`s to filter
based on event fields.

## Solution

Add `Filter::event_enabled`, plumb it through the combinator
implementations, and call it from `Filtered`.

The bit I'm the least confident about is the check in `Registry`'s
implementation, but I *think* it matches what `event` is doing and
everything seems to function correctly.
## Motivation

Doc typo.

## Solution

Fix.
## Motivation

When a global default dispatcher has already been set, the
`dispatch::set_global_default` function fails with a
`SetGlobalDefaultError`. The `fmt::Display` impl for this type includes
a message explaining that the error indicates that a global default has
already been set, but the `fmt::Debug` impl is derived, and just looks
like this:
```
SetGlobalDefaultError { _no_construct: () }
```
which isn't particularly helpful.

Unfortunately, when `unwrap()`ping or `expect()`ing a `Result`, the
`fmt::Debug` implementation for the error type is used, rather than
`fmt::Display`. This means that the error message will not explain to
the user why setting the global default dispatcher failed, which is a
bummer.

## Solution

This branch replaces the derived `Debug` impl with a manually
implemented one that prints the same message as the `Display` impl, but
formatted like a tuple struct with a single string field. This avoids
emitting `Debug` output that's *just* a textual human-readable message,
rather than looking like Rust code, but ensures the message is visible
to the user when writing code like
```rust
tracing::dispatch::set_global_default(foo).unwrap();
```

The mesasge now looks like:
```
SetGlobalDefaultError("a global default trace dispatcher has already been set")
```
which should be a bit easier to debug.
* Explain filter.not() w.r.t. event_enabled

Co-authored-by: David Barsky <[email protected]>
Co-authored-by: Eliza Weisman <[email protected]>
@hawkw hawkw requested review from a team and jtescher as code owners July 29, 2022 00:03
@hawkw hawkw enabled auto-merge (rebase) July 29, 2022 00:03
@hawkw hawkw merged commit ce07005 into v0.1.x Jul 29, 2022
@hawkw hawkw deleted the eliza/backport-2225 branch July 29, 2022 00:13
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.

9 participants