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

feat(log): unhide tracing::instrument from behind feature = "otel" #3873

Merged
merged 3 commits into from
Jul 10, 2024

Conversation

rami3l
Copy link
Member

@rami3l rami3l commented Jun 14, 2024

Cherry-picked from #3803 as discussed in #3803 (comment).

Note

This is done by simply replacing cfg_attr\(feature = \"otel\", tracing::instrument(\((.*)\))?\)\] with tracing::instrument(level = "trace", $2)], then replacing , \)\] with )].

@rami3l rami3l added this to the 1.28.0 milestone Jun 14, 2024
@rami3l
Copy link
Member Author

rami3l commented Jun 14, 2024

The Windows CI is still red, but only a small number of tests are failing due to stack overflow compared to #3803's CI.

@roife has pointed me to the stack size restriction on Windows, saying that maybe it's not a stack overflow caused by infinite recursion, instead we might simply need a bigger stack. rust-analyzer has some code related to this restriction: https://github.com/rust-lang/rust-analyzer/blob/6b8b8ff4c56118ddee6c531cde06add1aad4a6af/crates/rust-analyzer/src/bin/main.rs#L147-L163

@ChrisDenton
Copy link
Member

On Windows, applications choose their stack size rather than being defined by the system. So another option is to increase the default stack size by using a linker argument (for msvc that's /stack). I don't know what's easier.

@rami3l
Copy link
Member Author

rami3l commented Jun 15, 2024

@ChrisDenton Thanks a lot for your pointers!

Looks like unoptimized Rust is indeed hungry on stack space:

The Rust compiler uses a 6MiB stack and then increase than to 20MiB (!!!) when optimizations are disabled. The default stack size on Linux is 8MiB, so that's likely how much is available for the main thread. Rust tends to need a ridiculous amount of stack space, especially without optimizations.
rust-lang/rust#17044 (comment)

I'll try your suggestion in https://users.rust-lang.org/t/stack-overflow-when-compiling-on-windows-10/50818/8 and rust-lang/rust#85303 first.

@ChrisDenton
Copy link
Member

ChrisDenton commented Jun 15, 2024

Oh, hm actually I think the test runner sets the stack size itself. I'm not sure if that is directly configurable. You might need to set the RUST_MIN_STACK environment variable.

@rami3l rami3l force-pushed the feat/unhide-tracing-instrument branch 2 times, most recently from 2a7f0ec to 8730a22 Compare July 10, 2024 02:32
@rami3l rami3l force-pushed the feat/unhide-tracing-instrument branch from 8730a22 to 2ede7f7 Compare July 10, 2024 02:33
@rami3l
Copy link
Member Author

rami3l commented Jul 10, 2024

Oh, hm actually I think the test runner sets the stack size itself. I'm not sure if that is directly configurable. You might need to set the RUST_MIN_STACK environment variable.

@ChrisDenton Thanks a lot! That seems to work :)

PS: Sorry I got a bit sidetracked with other issues in the past few weeks 🙈

@rami3l rami3l force-pushed the feat/unhide-tracing-instrument branch from d4a5997 to c678487 Compare July 10, 2024 03:24
@rami3l rami3l marked this pull request as ready for review July 10, 2024 03:38
@rami3l rami3l requested a review from djc July 10, 2024 03:42
@rami3l rami3l added this pull request to the merge queue Jul 10, 2024
Merged via the queue into master with commit 79ba44c Jul 10, 2024
27 checks passed
@rami3l rami3l deleted the feat/unhide-tracing-instrument branch July 10, 2024 10:30
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.

3 participants