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

mock: improve ergonomics when an ExpectedSpan is needed #3097

Open
wants to merge 2 commits into
base: hds/mock-expected-span-check
Choose a base branch
from

Conversation

hds
Copy link
Contributor

@hds hds commented Oct 3, 2024

Motivation

Many of the methods on MockCollector take an ExpectedSpan. This
often requires significant boilerplate. For example, to expect that a
span with a specific name enters and then exits, the following code is
needed:

let span = expect::span().named("span name");

let (collector, handle) = collector::mock()
    .enter(span.clone())
    .exit(span)
    .run_with_handle();

Solution

In order to make using tracing-mock more ergonomic and also more
compact, the MockCollector and MockSubscriber methods that previous
took an ExpectedSpan, are now generic over Into<ExpectedSpan>.

There are currently 3 implementations of From for ExpectedSpan which
allow the following shorthand uses:

T: Into<String> - an ExpectedSpan will be created that expects to
have a name specified by T.

let (collector, handle) = collector::mock()
    .enter("span name")
    .exit("span name")
    .run_with_handle();

&ExpectedId - an ExpectedSpan will be created that expects to have
the expected Id. A reference is taken and cloned internally because the
caller always needs to use an ExpectedId in at least 2 calls to the
mock collector/subscriber.

let id = expect::id();

let (collector, handle) = collector::mock()
    .new_span(&id)
    .enter(&id)
    .run_with_handle();

&ExpectedSpan - The expected span is taken by reference and cloned.

let span = expect::span().named("span name");

let (collector, handle) = collector::mock()
    .enter(&span)
    .exit(&span)
    .run_with_handle();

In Rust, taking a reference to an object and immediately cloning it is
an anti-pattern. It is considered better to force the user to clone
outside the API to make the cloning explict.

However, in the case of a testing framework, it seems reasonable to
prefer a more concise API, rather than having it more explicit.

To reduce the size of this PR and to avoid unnecessary churn in other
crates, the tests within the tracing repo which use tracing-mock will
not be updated to use the new Into<ExpectedSpan> capabilities. The new
API is backwards compatible and those tests can remain as they are.

Previously there were two separate implementations which would check an
`ExpectedSpan` against and actual span. One on the `ExpectedSpan` struct
itself, which took a `SpanState` as used by the `MockCollector` and
another on the `MockSubscriber` which took a `SpanRef` (from
`tracing-subscriber`).

In reality, both of these checks needed a `span::Id` and a `Metadata` to
check, but the structure was also somewhat different, with the
`MockSubscriber` span check giving better error output.

This change combines the two checks into the one on `ExpectedSpan`,
which is now generic over an `ActualSpan` implementation, which has been
provided for `SpanState`, `SpanRef`, and also `span::Id` (for the case
where no `Metadata` is available).

The better error output from `MockSubscriber` has been integrated into
that check.
Many of the methods on `MockCollector` take an `ExpectedSpan`. This
often requires significant boilerplate. For example, to expect that a
span with a specific name enters and then exits, the following code is
needed:

```rust
let span = expect::span().named("span name");

let (collector, handle) = collector::mock()
    .enter(span.clone())
    .exit(span)
    .run_with_handle();
```

In order to make using `tracing-mock` more ergonomic and also more
compact, the `MockCollector` and `MockSubscriber` methods that previous
took an `ExpectedSpan`, are now generic over `Into<ExpectedSpan>`.

There are currently 3 implementations of `From` for `ExpectedSpan` which
allow the following shorthand uses:

`T: Into<String>` - an `ExpectedSpan` will be created that expects to
have a name specified by `T`.

```rust
let (collector, handle) = collector::mock()
    .enter("span name")
    .exit("span name")
    .run_with_handle();
```

`&ExpectedId` - an `ExpectedSpan` will be created that expects to have
the expected Id. A reference is taken and cloned internally because the
caller always needs to use an `ExpectedId` in at least 2 calls to the
mock collector/subscriber.

```rust
let id = expect::id();

let (collector, handle) = collector::mock()
    .new_span(&id)
    .enter(&id)
    .run_with_handle();
```

`&ExpectedSpan` - The expected span is taken by reference and cloned.

```rust
let span = expect::span().named("span name");

let (collector, handle) = collector::mock()
    .enter(&span)
    .exit(&span)
    .run_with_handle();
```

In Rust, taking a reference to an object and immediately cloning it is
an anti-pattern. It is considered better to force the user to clone
outside the API to make the cloning explict.

However, in the case of a testing framework, it seems reasonable to
prefer a more concise API, rather than having it more explicit.

To reduce the size of this PR and to avoid unnecessary churn in other
crates, the tests within the tracing repo which use `tracing-mock` will
not be updated to use the new `Into<ExpectedSpan>` capabilities. The new
API is backwards compatible and those tests can remain as they are.
@hds hds requested review from hawkw, davidbarsky and a team as code owners October 3, 2024 21:24
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.

1 participant