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

more backports #1767

Merged
merged 9 commits into from
Dec 20, 2021
Merged

more backports #1767

merged 9 commits into from
Dec 20, 2021

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Dec 2, 2021

@hawkw hawkw requested a review from davidbarsky December 2, 2021 17:40
@hawkw hawkw requested a review from a team as a code owner December 2, 2021 17:40
@hawkw hawkw force-pushed the eliza/backport-#1716 branch from 8c06ebe to c8acb55 Compare December 3, 2021 18:05
Some of the examples still include `#[macro_use] extern crate`
statements for importing macros from `tracing` or `tracing-core`. On a
recent nightly, this results in import conflicts with the implicit
import of the documented crate in doctests:
https://github.com/tokio-rs/tracing/runs/4279736243?check_suite_focus=true

This commit removes all the `extern crate` statements from doctests. Our
MSRV is new enough that `extern crate` is not required on any of the Rust
versions we support.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw force-pushed the eliza/backport-#1716 branch from 5af0788 to 1c3d703 Compare December 19, 2021 21:22
hawkw and others added 8 commits December 19, 2021 13:47
Depends on #1737

This branch removes all remaining `extern crate` statements. Most of
these are in old code and were not removed when updating to Rust 2018.
Whoops!

Our MSRV no longer requires `extern crate`, so we don't need these. The
exception is `extern crate` statements for `std` and `alloc`, which are
still the way these libraries are included explicitly when building for
`no_std` platforms.

In some cases, the tests had to explicitly import the `span!` and
`event!` macros at every use, because their names conflict with the
`span` and `event` modules in the test support code. Oh well.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Per discussion with @hawkw in #1698 I'm adding a few simple integration
tests for the journald subscriber, to have some safety net when
implementing the actual issue in #1698.

These tests send messages of various complexity to the journal, and then
use `journalctl`'s JSON output to get them back out, to check whether
the message arrives in the systemd journal as it was intended to.

## Motivation

Increase test coverage for the journald subscriber and codify a known
good state before approaching a fix for #1698.
* attributes: implement `#[instrument(ret)]`

## Motivation

Currently, users have to explicitly call logging functions at the end of
functions if they wanted to record values being returned. For example,
```rust
fn increment(a: i32) -> i32 {
    let succ = a + 1;
    tracing::debug!(?succ);
    succ
}
```
The code above will be significantly simpler if we provide the
functionality to record returned values.

## Solution

This PR adds the exact feature we're talking here, which enables users
to write the code below instead.
```rust
#[instrument(ret)]
fn increment(a: i32) -> i32 {
    a + 1
}
```

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
See #1710: Do not write strings in Debug representation.

## Motivation

As discussed in #1710 writing strings literally makes tracing-journald
behave like other journal clients, and allows 3rd party journal readers
to extract the original value from the journal without having to
"un"-parse the Debug representation of Rust strings.

Fixes #1710.
See #1698: Properly write large payloads to journal.

I'd appreciate a very careful review; this cmsg stuff is nasty, and
while it's well documented in `cmsg(3)` I had to fiddle a bit because
the corresponding functions in libc aren't const and thus don't permit a
direct allocation of the buffer as most `cmsg` C code around does.

Closes #1698

## Motivation

Linux limits the maximum amount of data permitted for a single Unix
datagram; sending large payloads directly will fail.

## Solution

Follow systemd.io/JOURNAL_NATIVE_PROTOCOL/ and check for `EMSGSIZE` from
`send()`; in this case write the payload to a memfd, seal it, and pass
it on to journald via a corresponding SCM_RIGHTS control message.

Per discussion in #1698 this adds no dependency on `nix`, and instead
implements fd forwarding directly with some bits of unsafe `libc` code.
* Add tracing-fluent-assertions to related crates.
Lets journald subscribers survive a journald restart.

Closes #1745

## Motivation

Currently the journald subscriber immediately connects to the journald
socket.  As such I understand it'd not survive a full restart of
journald.

## Solution

Do not connect the client socket immediately; instead pass the socket
pathname every time we send a message.  This is also what upstream does.
## Motivation

Currently, `tracing-appender`'s `RollingFileAppender` does not implement
the `MakeWriter` trait. This means it can only be used by either
wrapping it in `NonBlocking`, or by wrapping it in a `Mutex`. However,
this shouldn't be strictly necessary, as `&File` implements `io::Write`.
It should thus only be necessary to introduce locking when we are in the
process of _rotating_ the log file.

## Solution

This branch adds a `MakeWriter` implementation for
`RollingFileAppender`. This is done by moving the file itself inside of
an `RwLock`, so that a read lock is acquired to write to the file. This
allows multiple threads to write to the file without contention. When
the file needs to be rolled, the rolling thread acquires the write lock
to replace the file. Acquiring the write lock is guarded by an atomic
CAS on the timestamp, so that only a single thread will try to roll the
file. This prevents other threads from immediately rolling the file
_again_ when the write lock is released.

I...should probably write tests for that, though.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw force-pushed the eliza/backport-#1716 branch from ecb7e4f to 0b8e9a6 Compare December 19, 2021 21:47
@hawkw hawkw merged commit fc52d45 into v0.1.x Dec 20, 2021
@hawkw hawkw deleted the eliza/backport-#1716 branch December 20, 2021 00:23
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.

5 participants