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

attributes: added missing RecordTypes for instrument #2781

Merged
merged 2 commits into from
Oct 30, 2023

Conversation

kaffarell
Copy link
Contributor

When using a function annotated with #[instrument] it parses the parameters of the function and records them either using Value or using std::fmt::Debug. There were a few types that implement Value but were missing the RecordTypes array. Added them + a unit test for a single one.

Fixed: #2775

When using a function annotated with `#[instrument]` it parses the
parameters of the function and records them either using `Value` or
using `std::fmt::Debug`. There were a few types that implement `Value`
but were missing the RecordTypes array. Added them + a unit test for a
single one.

Fixed: tokio-rs#2775
@safinaskar
Copy link

I don't like this PR. I will write my arguments later (hopefully today), please, wait

@safinaskar
Copy link

safinaskar commented Oct 30, 2023

It seems this code determines whether the type implements Value at macro level. By parsing type. This is fragile. And this doesn't agree with spirit of Rust. This is very ugly. This is easy to broke such code, for example:

use std::string::String as Foo;
#[tracing::instrument]
fn f(x: Foo) {}

In this example Foo is actually String, and hence implements Value, but the macro will not be able to see this.

Also current solution assumes that we need to keep in sync implementors of Value and that list in macro. Which is obvious violation of don't-repeat-yourself principle.

The problem should be solved at Rust language level, not in macro level. How to do this? Well, I know the solution. What we essentially need is writing macro, which will determine does given expression implement given trait or not. If yes, then perform some action, if not, then perform other action. This is possible to do in Rust using one very ugly trick. One of the ugliest of the tricks in the language. Which abuses over-complicated rules for method call expression ( https://doc.rust-lang.org/nightly/reference/expressions/method-call-expr.html ). Here is this trick: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=17babd1d98bb46d1aefa6c879cb31bc3 . It is possible that you can write something similar shorter, but the idea will be the same. Of course, the code possibly should be adapted for your needs. For example, it is possible that in your case T should not be stored inside A, and we need to store &T instead or even PhantomData<T>. I learned this trick from some article, but I lost it. If you know this article, please, let me know.

I think whole existence of such trick in the language proves that Rust is not perfect. I'm nearly sure that real PLT (programming language theory) people (such as people from https://types.pl or http://lambda-the-ultimate.org/ or https://langdev.stackexchange.com ) will completely dislike this trick. Still it works, and it saves our day. It is correct, unlike current macro level implementation, and hence lesser evil.

UPD Oct 30: I found slightly simpler way, see next comment

UPD Nov 2: This method is called autoref specialization. Thanks, @davidbarsky

@safinaskar
Copy link

I found slightly simpler way, based on the same idea. It exploits method similar to well-known a.clone() ambiguity.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=449ecdebc93012d4f32ec007a5b329b8

@davidbarsky
Copy link
Member

@safinaskar I think that you're describing autoref specialization. Generally speaking, exposing type information to macros in a hygienic manner is generally considered to be an open research question in PL, and while it'd be nice for Rust to have a solution to this, I don't really expect one in the near future.

Also current solution assumes that we need to keep in sync implementors of Value and that list in macro. Which is obvious violation of don't-repeat-yourself principle.

My feeling is that if we want to prevent this issue from happening in the future, we can write a CLI that parses the rustdoc JSON output and ensures that the list is not out of date. A rust-analyzer style xtask approach can work here, but that's outside of the scope of this PR.

@kaffarell I think this PR is good and resolves the immediate issue. @safinaskar if you'd like to discuss alternatives, please open an issue.

@davidbarsky davidbarsky merged commit e06201c into tokio-rs:master Oct 30, 2023
54 checks passed
@CAD97
Copy link
Contributor

CAD97 commented Nov 1, 2023

Old PR implementing the autoref specialization approach: #1612

@davidbarsky
Copy link
Member

Old PR implementing the autoref specialization approach: #1612

Man, I forgot about that PR, but we should get it in for tracing 0.2, though. Would be nice to sidestep this issue.

davidbarsky pushed a commit that referenced this pull request Nov 7, 2023
When using a function annotated with `#[instrument]` it parses the
parameters of the function and records them either using `Value` or
using `std::fmt::Debug`. There were a few types that implement `Value`
but were missing the RecordTypes array. Added them + a unit test for a
single one.

Fixed: #2775
hawkw pushed a commit that referenced this pull request Nov 7, 2023
When using a function annotated with `#[instrument]` it parses the
parameters of the function and records them either using `Value` or
using `std::fmt::Debug`. There were a few types that implement `Value`
but were missing the RecordTypes array. Added them + a unit test for a
single one.

Fixed: #2775
kaffarell added a commit to kaffarell/tracing that referenced this pull request Feb 14, 2024
When using a function annotated with `#[instrument]` it parses the
parameters of the function and records them either using `Value` or
using `std::fmt::Debug`. There were a few types that implement `Value`
but were missing the RecordTypes array. Added them + a unit test for a
single one.

Fixed: tokio-rs#2775
davidbarsky pushed a commit that referenced this pull request Oct 2, 2024
When using a function annotated with `#[instrument]` it parses the
parameters of the function and records them either using `Value` or
using `std::fmt::Debug`. There were a few types that implement `Value`
but were missing the RecordTypes array. Added them + a unit test for a
single one.

Fixed: #2775
@fduwjj
Copy link

fduwjj commented Oct 3, 2024

Is there any reason why this change is not included in the release 0.1.27? We need this fix to make our logging works.

@kaffarell
Copy link
Contributor Author

This was merged after the release of 0.1.27. It will be included in the next version.

hds added a commit that referenced this pull request Nov 25, 2024
# 0.1.28 (November 26, 2024)

### Changed

- Bump MSRV to 1.63 ([#2793])

### Fixed

- Added missing RecordTypes for instrument ([#2781])
- Change order of async and unsafe modifier ([#2864])
- Extract match scrutinee ([#2880])
- Allow field path segments to be keywords ([#2925])
- Support const values for `target` and `name` ([#2941])

### Documented

- Fix backporting error in attributes ([#2780])

[#2780]: #2780
[#2781]: #2781
[#2793]: #2793
[#2864]: #2864
[#2880]: #2880
[#2925]: #2925
[#2941]: #2941
hds added a commit that referenced this pull request Nov 26, 2024
# 0.1.28 (November 26, 2024)

### Changed

- Bump MSRV to 1.63 ([#2793])

### Fixed

- Added missing RecordTypes for instrument ([#2781])
- Change order of async and unsafe modifier ([#2864])
- Extract match scrutinee ([#2880])
- Allow field path segments to be keywords ([#2925])
- Support const values for `target` and `name` ([#2941])

### Documented

- Fix backporting error in attributes ([#2780])

[#2780]: #2780
[#2781]: #2781
[#2793]: #2793
[#2864]: #2864
[#2880]: #2880
[#2925]: #2925
[#2941]: #2941
hds added a commit that referenced this pull request Nov 27, 2024
# 0.1.41 (November 25, 2024)

[[[crates.io][crate-0.1.41]]] | [[[docs.rs][docs-0.1.41]]]

This release updates the `tracing-core` dependency to [v0.1.33][core-0.1.33] and
the `tracing-attributes` dependency to [v0.1.28][attrs-0.1.28].

### Added

- **core**: Add index API for `Field` ([#2820])
- **core**: Allow `&[u8]` to be recorded as event/span field ([#2954])

### Changed

- Bump MSRV to 1.63 ([#2793])
- **core**: Use const `thread_local`s when possible ([#2838])

### Fixed

- Removed core imports in macros ([#2762])
- **attributes**: Added missing RecordTypes for instrument ([#2781])
- **attributes**: Change order of async and unsafe modifier ([#2864])
- Fix missing field prefixes ([#2878])
- **attributes**: Extract match scrutinee ([#2880])
- Fix non-simple macro usage without message ([#2879])
- Fix event macros with constant field names in the first position ([#2883])
- Allow field path segments to be keywords ([#2925])
- **core**: Fix missed `register_callsite` error ([#2938])
- **attributes**: Support const values for `target` and `name` ([#2941])
- Prefix macro calls with ::core to avoid clashing with local macros ([#3024])

[#2762]: #2762
[#2781]: #2781
[#2793]: #2793
[#2820]: #2820
[#2838]: #2838
[#2864]: #2864
[#2878]: #2878
[#2879]: #2879
[#2880]: #2880
[#2883]: #2883
[#2925]: #2925
[#2938]: #2938
[#2941]: #2941
[#2954]: #2954
[#3024]: #3024
[attrs-0.1.28]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.28
[core-0.1.33]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.33
[docs-0.1.41]: https://docs.rs/tracing/0.1.41/tracing/
[crate-0.1.41]: https://crates.io/crates/tracing/0.1.41
hds added a commit that referenced this pull request Nov 27, 2024
# 0.1.41 (November 25, 2024)

[[[crates.io][crate-0.1.41]]] | [[[docs.rs][docs-0.1.41]]]

This release updates the `tracing-core` dependency to [v0.1.33][core-0.1.33] and
the `tracing-attributes` dependency to [v0.1.28][attrs-0.1.28].

### Added

- **core**: Add index API for `Field` ([#2820])
- **core**: Allow `&[u8]` to be recorded as event/span field ([#2954])

### Changed

- Bump MSRV to 1.63 ([#2793])
- **core**: Use const `thread_local`s when possible ([#2838])

### Fixed

- Removed core imports in macros ([#2762])
- **attributes**: Added missing RecordTypes for instrument ([#2781])
- **attributes**: Change order of async and unsafe modifier ([#2864])
- Fix missing field prefixes ([#2878])
- **attributes**: Extract match scrutinee ([#2880])
- Fix non-simple macro usage without message ([#2879])
- Fix event macros with constant field names in the first position ([#2883])
- Allow field path segments to be keywords ([#2925])
- **core**: Fix missed `register_callsite` error ([#2938])
- **attributes**: Support const values for `target` and `name` ([#2941])
- Prefix macro calls with ::core to avoid clashing with local macros ([#3024])

[#2762]: #2762
[#2781]: #2781
[#2793]: #2793
[#2820]: #2820
[#2838]: #2838
[#2864]: #2864
[#2878]: #2878
[#2879]: #2879
[#2880]: #2880
[#2883]: #2883
[#2925]: #2925
[#2938]: #2938
[#2941]: #2941
[#2954]: #2954
[#3024]: #3024
[attrs-0.1.28]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.28
[core-0.1.33]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.33
[docs-0.1.41]: https://docs.rs/tracing/0.1.41/tracing/
[crate-0.1.41]: https://crates.io/crates/tracing/0.1.41
hds added a commit that referenced this pull request Nov 27, 2024
# 0.1.41 (November 25, 2024)

[[[crates.io][crate-0.1.41]]] | [[[docs.rs][docs-0.1.41]]]

This release updates the `tracing-core` dependency to [v0.1.33][core-0.1.33] and
the `tracing-attributes` dependency to [v0.1.28][attrs-0.1.28].

### Added

- **core**: Add index API for `Field` ([#2820])
- **core**: Allow `&[u8]` to be recorded as event/span field ([#2954])

### Changed

- Bump MSRV to 1.63 ([#2793])
- **core**: Use const `thread_local`s when possible ([#2838])

### Fixed

- Removed core imports in macros ([#2762])
- **attributes**: Added missing RecordTypes for instrument ([#2781])
- **attributes**: Change order of async and unsafe modifier ([#2864])
- Fix missing field prefixes ([#2878])
- **attributes**: Extract match scrutinee ([#2880])
- Fix non-simple macro usage without message ([#2879])
- Fix event macros with constant field names in the first position ([#2883])
- Allow field path segments to be keywords ([#2925])
- **core**: Fix missed `register_callsite` error ([#2938])
- **attributes**: Support const values for `target` and `name` ([#2941])
- Prefix macro calls with ::core to avoid clashing with local macros ([#3024])

[#2762]: #2762
[#2781]: #2781
[#2793]: #2793
[#2820]: #2820
[#2838]: #2838
[#2864]: #2864
[#2878]: #2878
[#2879]: #2879
[#2880]: #2880
[#2883]: #2883
[#2925]: #2925
[#2938]: #2938
[#2941]: #2941
[#2954]: #2954
[#3024]: #3024
[attrs-0.1.28]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.28
[core-0.1.33]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.33
[docs-0.1.41]: https://docs.rs/tracing/0.1.41/tracing/
[crate-0.1.41]: https://crates.io/crates/tracing/0.1.41
hds added a commit that referenced this pull request Nov 27, 2024
# 0.1.41 (November 25, 2024)

[ [crates.io][crate-0.1.41] ] | [ [docs.rs][docs-0.1.41] ]

This release updates the `tracing-core` dependency to [v0.1.33][core-0.1.33] and
the `tracing-attributes` dependency to [v0.1.28][attrs-0.1.28].

### Added

- **core**: Add index API for `Field` ([#2820])
- **core**: Allow `&[u8]` to be recorded as event/span field ([#2954])

### Changed

- Bump MSRV to 1.63 ([#2793])
- **core**: Use const `thread_local`s when possible ([#2838])

### Fixed

- Removed core imports in macros ([#2762])
- **attributes**: Added missing RecordTypes for instrument ([#2781])
- **attributes**: Change order of async and unsafe modifier ([#2864])
- Fix missing field prefixes ([#2878])
- **attributes**: Extract match scrutinee ([#2880])
- Fix non-simple macro usage without message ([#2879])
- Fix event macros with constant field names in the first position ([#2883])
- Allow field path segments to be keywords ([#2925])
- **core**: Fix missed `register_callsite` error ([#2938])
- **attributes**: Support const values for `target` and `name` ([#2941])
- Prefix macro calls with ::core to avoid clashing with local macros ([#3024])

[#2762]: #2762
[#2781]: #2781
[#2793]: #2793
[#2820]: #2820
[#2838]: #2838
[#2864]: #2864
[#2878]: #2878
[#2879]: #2879
[#2880]: #2880
[#2883]: #2883
[#2925]: #2925
[#2938]: #2938
[#2941]: #2941
[#2954]: #2954
[#3024]: #3024
[attrs-0.1.28]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.28
[core-0.1.33]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.33
[docs-0.1.41]: https://docs.rs/tracing/0.1.41/tracing/
[crate-0.1.41]: https://crates.io/crates/tracing/0.1.41
hds added a commit that referenced this pull request Nov 27, 2024
# 0.1.41 (November 25, 2024)

[ [crates.io][crate-0.1.41] ] | [ [docs.rs][docs-0.1.41] ]

This release updates the `tracing-core` dependency to [v0.1.33][core-0.1.33] and
the `tracing-attributes` dependency to [v0.1.28][attrs-0.1.28].

### Added

- **core**: Add index API for `Field` ([#2820])
- **core**: Allow `&[u8]` to be recorded as event/span field ([#2954])

### Changed

- Bump MSRV to 1.63 ([#2793])
- **core**: Use const `thread_local`s when possible ([#2838])

### Fixed

- Removed core imports in macros ([#2762])
- **attributes**: Added missing RecordTypes for instrument ([#2781])
- **attributes**: Change order of async and unsafe modifier ([#2864])
- Fix missing field prefixes ([#2878])
- **attributes**: Extract match scrutinee ([#2880])
- Fix non-simple macro usage without message ([#2879])
- Fix event macros with constant field names in the first position ([#2883])
- Allow field path segments to be keywords ([#2925])
- **core**: Fix missed `register_callsite` error ([#2938])
- **attributes**: Support const values for `target` and `name` ([#2941])
- Prefix macro calls with ::core to avoid clashing with local macros ([#3024])

[#2762]: #2762
[#2781]: #2781
[#2793]: #2793
[#2820]: #2820
[#2838]: #2838
[#2864]: #2864
[#2878]: #2878
[#2879]: #2879
[#2880]: #2880
[#2883]: #2883
[#2925]: #2925
[#2938]: #2938
[#2941]: #2941
[#2954]: #2954
[#3024]: #3024
[attrs-0.1.28]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.28
[core-0.1.33]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.33
[docs-0.1.41]: https://docs.rs/tracing/0.1.41/tracing/
[crate-0.1.41]: https://crates.io/crates/tracing/0.1.41
hds added a commit that referenced this pull request Nov 27, 2024
# 0.1.41 (November 27, 2024)

[ [crates.io][crate-0.1.41] ] | [ [docs.rs][docs-0.1.41] ]

This release updates the `tracing-core` dependency to [v0.1.33][core-0.1.33] and
the `tracing-attributes` dependency to [v0.1.28][attrs-0.1.28].

### Added

- **core**: Add index API for `Field` ([#2820])
- **core**: Allow `&[u8]` to be recorded as event/span field ([#2954])

### Changed

- Bump MSRV to 1.63 ([#2793])
- **core**: Use const `thread_local`s when possible ([#2838])

### Fixed

- Removed core imports in macros ([#2762])
- **attributes**: Added missing RecordTypes for instrument ([#2781])
- **attributes**: Change order of async and unsafe modifier ([#2864])
- Fix missing field prefixes ([#2878])
- **attributes**: Extract match scrutinee ([#2880])
- Fix non-simple macro usage without message ([#2879])
- Fix event macros with constant field names in the first position ([#2883])
- Allow field path segments to be keywords ([#2925])
- **core**: Fix missed `register_callsite` error ([#2938])
- **attributes**: Support const values for `target` and `name` ([#2941])
- Prefix macro calls with ::core to avoid clashing with local macros ([#3024])

[#2762]: #2762
[#2781]: #2781
[#2793]: #2793
[#2820]: #2820
[#2838]: #2838
[#2864]: #2864
[#2878]: #2878
[#2879]: #2879
[#2880]: #2880
[#2883]: #2883
[#2925]: #2925
[#2938]: #2938
[#2941]: #2941
[#2954]: #2954
[#3024]: #3024
[attrs-0.1.28]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.28
[core-0.1.33]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.33
[docs-0.1.41]: https://docs.rs/tracing/0.1.41/tracing/
[crate-0.1.41]: https://crates.io/crates/tracing/0.1.41
hds added a commit that referenced this pull request Nov 27, 2024
# 0.1.41 (November 27, 2024)

[ [crates.io][crate-0.1.41] ] | [ [docs.rs][docs-0.1.41] ]

This release updates the `tracing-core` dependency to [v0.1.33][core-0.1.33] and
the `tracing-attributes` dependency to [v0.1.28][attrs-0.1.28].

### Added

- **core**: Add index API for `Field` ([#2820])
- **core**: Allow `&[u8]` to be recorded as event/span field ([#2954])

### Changed

- Bump MSRV to 1.63 ([#2793])
- **core**: Use const `thread_local`s when possible ([#2838])

### Fixed

- Removed core imports in macros ([#2762])
- **attributes**: Added missing RecordTypes for instrument ([#2781])
- **attributes**: Change order of async and unsafe modifier ([#2864])
- Fix missing field prefixes ([#2878])
- **attributes**: Extract match scrutinee ([#2880])
- Fix non-simple macro usage without message ([#2879])
- Fix event macros with constant field names in the first position ([#2883])
- Allow field path segments to be keywords ([#2925])
- **core**: Fix missed `register_callsite` error ([#2938])
- **attributes**: Support const values for `target` and `name` ([#2941])
- Prefix macro calls with ::core to avoid clashing with local macros ([#3024])

[#2762]: #2762
[#2781]: #2781
[#2793]: #2793
[#2820]: #2820
[#2838]: #2838
[#2864]: #2864
[#2878]: #2878
[#2879]: #2879
[#2880]: #2880
[#2883]: #2883
[#2925]: #2925
[#2938]: #2938
[#2941]: #2941
[#2954]: #2954
[#3024]: #3024
[attrs-0.1.28]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.28
[core-0.1.33]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.33
[docs-0.1.41]: https://docs.rs/tracing/0.1.41/tracing/
[crate-0.1.41]: https://crates.io/crates/tracing/0.1.41
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.

Docs say that #[instrument] uses Value trait, but experiment disagree
5 participants