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

Ensure 'res' and 'err' inherit 'target' #2184

Merged
merged 13 commits into from
Jul 1, 2022
Merged

Conversation

tbraun96
Copy link
Contributor

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.

this looks good to me, thank you!

it might be nice to add a test; the change is simple enough that I'm sure it works, but adding a test would ensure that future changes don't accidentally break this...

it might also be good to mention in the docs (maybe around here) that the ret and err events inherit the span's target when it is overridden.

@tbraun96
Copy link
Contributor Author

tbraun96 commented Jun 29, 2022

I would be interested to see how we'd test for this. I know code would need to be added under tests/ret.rs and tests/err.rs, though, I don't know how I'd assert the output would contain the declared target.

(On an unrelated note, skip_all is deprecated on master, but not on the latest version on crates.io. What is the suggested way to disable all fields?)

@hawkw
Copy link
Member

hawkw commented Jun 29, 2022

I would be interested to see how we'd test for this. I know code would need to be added under tests/ret.rs and tests/err.rs, though, I don't know how I'd assert the output would contain the declared target.

The mock subscriber implementation (in tracing-mock) has functions for expecting a span or event with a given target. This is currently used in tracing-attributes/tests/target.rs for expecting spans to have a configured target:

let (collector, handle) = collector::mock()
.new_span(span::mock().named("custom_target").with_target("my_target"))

There's also a function for expecting that an event has a given target. This isn't currently used in tracing-attributes' tests, but it is used for the tests for other crates. For example:

.event(event::mock().at_level(Level::DEBUG).with_target("stuff"))
.event(event::mock().at_level(Level::WARN).with_target("stuff"))

We would essentially just want to write a test similar to the existing tests in tests/ret.rs and tests/err.rs which calls a function with #[instrument(target = "something", err)] (and ret), and add a with_target("something") call when configuring the mock event to expect.

Hope that helps, please let me know if you have additional questions!

@hawkw
Copy link
Member

hawkw commented Jun 29, 2022

(On an unrelated note, skip_all is deprecated on master, but not on the latest version on crates.io. What is the suggested way to disable all fields?)

Just continue using skip_all. The master branch has in-flight breaking changes for an eventual 0.2 release of tracing, and in tracing-attributes 0.2, #[instrument] fields will be opt-in rather than opt-out, so skipping them will no longer be necessary. However, that breaking change isn't going to be released in the near future, so you should just continue using skip_all. Upcoming tracing-attributes releases will be published from the v0.1.x branch until all the breaking changes for 0.2 are ready.

@tbraun96
Copy link
Contributor Author

Okay, the unit tests have been added! Thanks for the help

@tbraun96
Copy link
Contributor Author

tbraun96 commented Jun 30, 2022

Strange how the pipeline failed for that unrelated file

@hawkw did you want me to fix this before merging?

@hawkw
Copy link
Member

hawkw commented Jun 30, 2022

Strange how the pipeline failed for that unrelated file

This occurs sometimes when a new release of clippy adds lints that it did not previously generate warnings for, not your fault at all.

@hawkw did you want me to fix this before merging?

Normally we fix these issues separately and then update any failing branches with the latest version of the base branch. In this case, everything should be fixed on master, so updating your branch ought to be sufficient.

@hawkw hawkw merged commit 63cff14 into tokio-rs:master Jul 1, 2022
hawkw pushed a commit that referenced this pull request Jul 1, 2022
## Motivation

Currently, when an `#[instrument]` attribute has an overridden target,
the events generated by `ret` and `err` arguments do not inherit that
target.

For example, if I write

```rust
#[tracing::instrument(target = "some_target", ret)
fn do_stuff() -> Something {
    // ...
}
```

the `do_stuff` span will have the target "some_target", but the return
value event generated by `ret` will have the current module path as its
target, and there is no way to change the return value event's target.

## Solution

This branch changes the macro expansion for `#[instrument]` with the
`ret` and/or `err` arguments so that an overridden target is propagated
to the events generated by `ret` and `err`.

Fixes #2183
hawkw pushed a commit that referenced this pull request Jul 1, 2022
## Motivation

Currently, when an `#[instrument]` attribute has an overridden target,
the events generated by `ret` and `err` arguments do not inherit that
target.

For example, if I write

```rust
#[tracing::instrument(target = "some_target", ret)
fn do_stuff() -> Something {
    // ...
}
```

the `do_stuff` span will have the target "some_target", but the return
value event generated by `ret` will have the current module path as its
target, and there is no way to change the return value event's target.

## Solution

This branch changes the macro expansion for `#[instrument]` with the
`ret` and/or `err` arguments so that an overridden target is propagated
to the events generated by `ret` and `err`.

Fixes #2183
hawkw added a commit that referenced this pull request Jul 1, 2022
# 0.1.22 (July 1, 2022)

This release fixes an issue where using the `err` or `ret` arguments to
`#[instrument]` along with an overridden target, such as

```rust
#[instrument(target = "...", err, ret)]
```

would not propagate the overridden target to the events generated for
errors/return values.

### Fixed

- Error and return value events generated by `#[instrument(err)]` or
  `#[instrument(ret)]` not inheriting an overridden target (#2184)
- Incorrect default level in documentation (#2119)

Thanks to new contributor @tbraun96 for contributing to this release!
hawkw added a commit that referenced this pull request Jul 1, 2022
# 0.1.22 (July 1, 2022)

This release fixes an issue where using the `err` or `ret` arguments to
`#[instrument]` along with an overridden target, such as

```rust
#[instrument(target = "...", err, ret)]
```

would not propagate the overridden target to the events generated for
errors/return values.

### Fixed

- Error and return value events generated by `#[instrument(err)]` or
  `#[instrument(ret)]` not inheriting an overridden target (#2184)
- Incorrect default level in documentation (#2119)

Thanks to new contributor @tbraun96 for contributing to this release!
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
…2184)

## Motivation

Currently, when an `#[instrument]` attribute has an overridden target,
the events generated by `ret` and `err` arguments do not inherit that
target.

For example, if I write

```rust
#[tracing::instrument(target = "some_target", ret)
fn do_stuff() -> Something {
    // ...
}
```

the `do_stuff` span will have the target "some_target", but the return
value event generated by `ret` will have the current module path as its
target, and there is no way to change the return value event's target.

## Solution

This branch changes the macro expansion for `#[instrument]` with the
`ret` and/or `err` arguments so that an overridden target is propagated
to the events generated by `ret` and `err`.

Fixes tokio-rs#2183
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.1.22 (July 1, 2022)

This release fixes an issue where using the `err` or `ret` arguments to
`#[instrument]` along with an overridden target, such as

```rust
#[instrument(target = "...", err, ret)]
```

would not propagate the overridden target to the events generated for
errors/return values.

### Fixed

- Error and return value events generated by `#[instrument(err)]` or
  `#[instrument(ret)]` not inheriting an overridden target (tokio-rs#2184)
- Incorrect default level in documentation (tokio-rs#2119)

Thanks to new contributor @tbraun96 for contributing to this release!
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.

None yet

2 participants