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

Allow value_sets of any length #2508

Merged
merged 1 commit into from
Sep 5, 2023
Merged

Conversation

eopb
Copy link
Contributor

@eopb eopb commented Mar 14, 2023

Motivation

fixes: #1566

Solution

This change removes the maximum of 32 fields limitation using const generics.

Having this arbitrary restriction in place to prevent stack overflows feels a little misplaced to me since stack size varies between environments.

@eopb eopb marked this pull request as ready for review March 14, 2023 17:10
@eopb eopb requested review from hawkw, carllerche, davidbarsky and a team as code owners March 14, 2023 17:10
@eopb eopb changed the title Allow value_sets of any lenght Allow value_sets of any length Mar 14, 2023
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.

I'm fine with making this change. If memory serves, I believe the motivation for the array length limit was due to Rust compiler limitations that no longer exist.

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.

oh, before we merge this: there's a docs example stating that a span may only have 32 fields or fewer, which shows creating a span with > 32 fields and asserts that it doesn't compile:

tracing/tracing/src/lib.rs

Lines 389 to 403 in bf05c61

//! Note that a span may have up to 32 fields. The following will not compile:
//!
//! ```rust,compile_fail
//! # use tracing::Level;
//! # fn main() {
//! let bad_span = span!(
//! Level::TRACE,
//! "too many fields!",
//! a = 1, b = 2, c = 3, d = 4, e = 5, f = 6, g = 7, h = 8, i = 9,
//! j = 10, k = 11, l = 12, m = 13, n = 14, o = 15, p = 16, q = 17,
//! r = 18, s = 19, t = 20, u = 21, v = 22, w = 23, x = 24, y = 25,
//! z = 26, aa = 27, bb = 28, cc = 29, dd = 30, ee = 31, ff = 32, gg = 33
//! );
//! # }
//! ```

Let's make sure to remove that example before merging this PR?


As a side note, I would expect the inclusion of that example with compile_fail to be breaking the CI build now that it compiles...which makes me concerned that we're not actually running doctests on CI, or something...

@hawkw
Copy link
Member

hawkw commented Sep 5, 2023

Hmm, actually, it looks like that test did run on this PR, and it did fail to compile: https://github.com/tokio-rs/tracing/actions/runs/6087610980/job/16516577365?pr=2508#step:6:186

This makes me concerned that there's some other reason that test doesn't compile, in addition to the field length limit. Or, we haven't fully removed field length validation. Could we maybe add a new normal test to the macro tests that tries to create a span with > 32 fields, just to make sure that it works?

@eopb
Copy link
Contributor Author

eopb commented Sep 5, 2023

Hmm, actually, it looks like that test did run on this PR, and it did fail to compile: https://github.com/tokio-rs/tracing/actions/runs/6087610980/job/16516577365?pr=2508#step:6:186

This makes me concerned that there's some other reason that test doesn't compile, in addition to the field length limit. Or, we haven't fully removed field length validation. Could we maybe add a new normal test to the macro tests that tries to create a span with > 32 fields, just to make sure that it works?

error: cannot find macro `span` in this scope
 --> tracing/src/lib.rs:395:16
  |
6 | let bad_span = span!(
  |                ^^^^
  |
help: consider importing this macro
  |
4 + use tracing::span;
  |

error: aborting due to previous error

The test wasn't failing for the right reasons :)

@eopb
Copy link
Contributor Author

eopb commented Sep 5, 2023

@hawkw Fixed and I've added a test

@eopb eopb requested a review from hawkw September 5, 2023 17:48
@hawkw hawkw enabled auto-merge (squash) September 5, 2023 17:51
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.

fantastic, thank you! let's merge this as soon as CI passes! :)

@hawkw hawkw merged commit 1c855ae into tokio-rs:master Sep 5, 2023
55 checks passed
@eopb eopb deleted the greater-than-32 branch September 5, 2023 18:18
davidbarsky pushed a commit that referenced this pull request Sep 26, 2023
## Motivation

Fixes: #1566

## Solution

This change removes the maximum of 32 fields limitation using const
generics.

Having this arbitrary restriction in place to prevent stack overflows
feels a little misplaced to me since stack size varies between
environments.
davidbarsky pushed a commit that referenced this pull request Sep 27, 2023
## Motivation

Fixes: #1566

## Solution

This change removes the maximum of 32 fields limitation using const
generics.

Having this arbitrary restriction in place to prevent stack overflows
feels a little misplaced to me since stack size varies between
environments.
davidbarsky pushed a commit that referenced this pull request Sep 27, 2023
## Motivation

Fixes: #1566

## Solution

This change removes the maximum of 32 fields limitation using const
generics.

Having this arbitrary restriction in place to prevent stack overflows
feels a little misplaced to me since stack size varies between
environments.
davidbarsky pushed a commit that referenced this pull request Sep 27, 2023
## Motivation

Fixes: #1566

## Solution

This change removes the maximum of 32 fields limitation using const
generics.

Having this arbitrary restriction in place to prevent stack overflows
feels a little misplaced to me since stack size varies between
environments.
davidbarsky pushed a commit that referenced this pull request Sep 27, 2023
## Motivation

Fixes: #1566

## Solution

This change removes the maximum of 32 fields limitation using const
generics.

Having this arbitrary restriction in place to prevent stack overflows
feels a little misplaced to me since stack size varies between
environments.
davidbarsky pushed a commit that referenced this pull request Sep 29, 2023
## Motivation

Fixes: #1566

## Solution

This change removes the maximum of 32 fields limitation using const
generics.

Having this arbitrary restriction in place to prevent stack overflows
feels a little misplaced to me since stack size varies between
environments.
hawkw pushed a commit that referenced this pull request Oct 1, 2023
## Motivation

Fixes: #1566

## Solution

This change removes the maximum of 32 fields limitation using const
generics.

Having this arbitrary restriction in place to prevent stack overflows
feels a little misplaced to me since stack size varies between
environments.
hawkw pushed a commit that referenced this pull request Oct 13, 2023
# 0.1.32 (October 13, 2023)

### Documented

- Fix typo in `field` docs (#2611)
- Remove duplicate wording (#2674)

### Changed

- Allow `ValueSet`s of any length (#2508)
hawkw pushed a commit that referenced this pull request Oct 13, 2023
# 0.1.39 (October 12, 2023)

This release adds several additional features to the `tracing` macros.
In addition, it updates the `tracing-core` dependency to
[v0.1.32][core-0.1.32] and the `tracing-attributes` dependency to
[v0.1.27][attrs-0.1.27].

### Added

- Allow constant field names in macros ([#2617])
- Allow setting event names in macros ([#2699])
- **core**: Allow `ValueSet`s of any length ([#2508])

### Changed

- `tracing-attributes`: updated to [0.1.27][attrs-0.1.27]
- `tracing-core`: updated to [0.1.32][core-0.1.32]
- **attributes**: Bump minimum version of proc-macro2 to 1.0.60
  ([#2732])
-  **attributes**: Generate less dead code for async block return type
   hint ([#2709])

### Fixed

- Use fully qualified names in macros for items exported from std
  prelude ([#2621], [#2757])
- **attributes**: Allow [`clippy::let_with_type_underscore`] in
  macro-generated code ([#2609])
- **attributes**: Allow `unknown_lints` in macro-generated code
  ([#2626])
- **attributes**: Fix a compilation error in `#[instrument]` when the
  `"log"` feature is enabled ([#2599])

### Documented

- Add `axum-insights` to relevant crates. ([#2713])
- Fix link to RAI pattern crate documentation ([#2612])
- Fix docs typos and warnings ([#2581])
- Add `clippy-tracing` to related crates ([#2628])
- Add `tracing-cloudwatch` to related crates ([#2667])
- Fix deadlink to `tracing-etw` repo ([#2602])

[#2617]: #2617
[#2699]: #2699
[#2508]: #2508
[#2621]: #2621
[#2713]: #2713
[#2581]: #2581
[#2628]: #2628
[#2667]: #2667
[#2602]: #2602
[#2626]: #2626
[#2757]: #2757
[#2732]: #2732
[#2709]: #2709
[#2599]: #2599
[`let_with_type_underscore`]: http://rust-lang.github.io/rust-clippy/rust-1.70.0/index.html#let_with_type_underscore
[attrs-0.1.27]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.27
[core-0.1.32]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.32
maddiemort added a commit to maddiemort/tracing that referenced this pull request Nov 3, 2023
There used to be a length restriction on the arrays usable to construct
`ValueSet`s, but this was due to Rust compiler limitations that no
longer exist. After that restriction was removed, the `ValidLen` trait
was kept around but implemented for statically-sized arrays of all
lengths.

While in theory this allows `ValueSet`s to be constructed from arrays of
arbitrary length, the size of the arrays must still be known at compile
time. Now that there is no longer a restriction on the length of arrays,
slices could in theory be used as well (since they meet the supertrait
requirement of `ValidLen`), but no implementation of the trait exists.

Adding an implementation of `ValidLen` for slices is straightforward.

Refs: tokio-rs#2508
hamchapman pushed a commit to getditto/tracing that referenced this pull request Apr 25, 2024
There used to be a length restriction on the arrays usable to construct
`ValueSet`s, but this was due to Rust compiler limitations that no
longer exist. After that restriction was removed, the `ValidLen` trait
was kept around but implemented for statically-sized arrays of all
lengths.

While in theory this allows `ValueSet`s to be constructed from arrays of
arbitrary length, the size of the arrays must still be known at compile
time. Now that there is no longer a restriction on the length of arrays,
slices could in theory be used as well (since they meet the supertrait
requirement of `ValidLen`), but no implementation of the trait exists.

Adding an implementation of `ValidLen` for slices is straightforward.

Refs: tokio-rs#2508
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.1.32 (October 13, 2023)

### Documented

- Fix typo in `field` docs (tokio-rs#2611)
- Remove duplicate wording (tokio-rs#2674)

### Changed

- Allow `ValueSet`s of any length (tokio-rs#2508)
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.1.39 (October 12, 2023)

This release adds several additional features to the `tracing` macros.
In addition, it updates the `tracing-core` dependency to
[v0.1.32][core-0.1.32] and the `tracing-attributes` dependency to
[v0.1.27][attrs-0.1.27].

### Added

- Allow constant field names in macros ([tokio-rs#2617])
- Allow setting event names in macros ([tokio-rs#2699])
- **core**: Allow `ValueSet`s of any length ([tokio-rs#2508])

### Changed

- `tracing-attributes`: updated to [0.1.27][attrs-0.1.27]
- `tracing-core`: updated to [0.1.32][core-0.1.32]
- **attributes**: Bump minimum version of proc-macro2 to 1.0.60
  ([tokio-rs#2732])
-  **attributes**: Generate less dead code for async block return type
   hint ([tokio-rs#2709])

### Fixed

- Use fully qualified names in macros for items exported from std
  prelude ([tokio-rs#2621], [tokio-rs#2757])
- **attributes**: Allow [`clippy::let_with_type_underscore`] in
  macro-generated code ([tokio-rs#2609])
- **attributes**: Allow `unknown_lints` in macro-generated code
  ([tokio-rs#2626])
- **attributes**: Fix a compilation error in `#[instrument]` when the
  `"log"` feature is enabled ([tokio-rs#2599])

### Documented

- Add `axum-insights` to relevant crates. ([tokio-rs#2713])
- Fix link to RAI pattern crate documentation ([tokio-rs#2612])
- Fix docs typos and warnings ([tokio-rs#2581])
- Add `clippy-tracing` to related crates ([tokio-rs#2628])
- Add `tracing-cloudwatch` to related crates ([tokio-rs#2667])
- Fix deadlink to `tracing-etw` repo ([tokio-rs#2602])

[tokio-rs#2617]: tokio-rs#2617
[tokio-rs#2699]: tokio-rs#2699
[tokio-rs#2508]: tokio-rs#2508
[tokio-rs#2621]: tokio-rs#2621
[tokio-rs#2713]: tokio-rs#2713
[tokio-rs#2581]: tokio-rs#2581
[tokio-rs#2628]: tokio-rs#2628
[tokio-rs#2667]: tokio-rs#2667
[tokio-rs#2602]: tokio-rs#2602
[tokio-rs#2626]: tokio-rs#2626
[tokio-rs#2757]: tokio-rs#2757
[tokio-rs#2732]: tokio-rs#2732
[tokio-rs#2709]: tokio-rs#2709
[tokio-rs#2599]: tokio-rs#2599
[`let_with_type_underscore`]: http://rust-lang.github.io/rust-clippy/rust-1.70.0/index.html#let_with_type_underscore
[attrs-0.1.27]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.27
[core-0.1.32]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.32
maddiemort added a commit to getditto/tracing that referenced this pull request Nov 19, 2024
There used to be a length restriction on the arrays usable to construct
`ValueSet`s, but this was due to Rust compiler limitations that no
longer exist. After that restriction was removed, the `ValidLen` trait
was kept around but implemented for statically-sized arrays of all
lengths.

While in theory this allows `ValueSet`s to be constructed from arrays of
arbitrary length, the size of the arrays must still be known at compile
time. Now that there is no longer a restriction on the length of arrays,
slices could in theory be used as well (since they meet the supertrait
requirement of `ValidLen`), but no implementation of the trait exists.

Adding an implementation of `ValidLen` for slices is straightforward.

Refs: tokio-rs#2508
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.

[Feature request] Raise the number of maximum fields
2 participants