Skip to content

Add in opentelemetry tracing as a feature #3287

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

Merged
merged 1 commit into from
May 3, 2023

Conversation

rbtcollins
Copy link
Contributor

This is an evolution on the previous tracing support I added a few years
back. See CONTRIBUTING for details.

The rs_tracing crate is functionally fine but very limited in features
compared to the tracing crate that provides the developer interface for
opentelemetry instrumentation. As we start looking at performance again
I think this tooling will be useful.

@rbtcollins
Copy link
Contributor Author

rbtcollins commented Mar 26, 2023

Example trace showing the slow spots in rustup show.
image

@rbtcollins rbtcollins force-pushed the tracing branch 3 times, most recently from 5337e36 to f2afa2a Compare March 26, 2023 11:53
@rbtcollins
Copy link
Contributor Author

And trace of a test
image

@rbtcollins rbtcollins force-pushed the tracing branch 3 times, most recently from bc14b7c to 8f4c55b Compare April 9, 2023 14:19
@0xPoe
Copy link
Member

0xPoe commented Apr 9, 2023

I'll try it and review it recently. Thanks!

Copy link
Member

@0xPoe 0xPoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just go through it. The PR is conflicting now.

I will take a deep look tomorrow.

Thanks for adding it!

@rbtcollins rbtcollins force-pushed the tracing branch 2 times, most recently from 1fa56a8 to baae569 Compare April 30, 2023 08:40
@rbtcollins
Copy link
Contributor Author

Update to resolve conflicts

@rbtcollins
Copy link
Contributor Author

Random MacOSX test failure with no visible cause :/

Copy link
Member

@0xPoe 0xPoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally. Looks good to me! Thanks for adding it!

Sorry, I merged some PRs and make your pull request conflicts again.

them temporarily while figuring out the shape of a problem)
- Be way of debug build timing - release optimisations make a huge difference,
though debug is a lot faster to iterate on. If something isn't a problem in
release don't pay it too much heed in debug.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
release don't pay it too much heed in debug.
release don't pay it too much heed in debug

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you want the trailing period ('.')?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because other sentences don't have it.

This is an evolution on the previous tracing support I added a few years
back. See CONTRIBUTING for details.

The rs_tracing crate is functionally fine but very limited in features
compared to the tracing crate that provides the developer interface for
opentelemetry instrumentation. As we start looking at performance again
I think this tooling will be useful.

Included is support for both tracing rustup-init and individual tests
@rbtcollins rbtcollins merged commit 4b29b79 into rust-lang:master May 3, 2023
@rbtcollins rbtcollins deleted the tracing branch May 3, 2023 20:22
@rami3l rami3l mentioned this pull request Jan 29, 2024
1 task
bors added a commit to rust-lang/cargo that referenced this pull request Mar 5, 2024
feat(cli): Allow logging to chrome traces

### What does this PR try to resolve?

> The time from executing cargo to executing rustc is 230 ms. I wonder if there’s scope for caching whatever expensive computations cargo is doing here.

*Source: https://davidlattimore.github.io/working-on-rust-iteration-time.html*

This made me curious where the time was going.  I've been meaning to try out `tracing-chrome` for a while and this gave me the opportunity.

This adds `CARGO_LOG_PROFILE=<bool>` and `CARGO_LOG_PROFILE_CAPTURE_ARGS=<bool>` for enabling and controlling these trace files.  These are perma-unstable env variables.

Traces can be viewed at https://ui.perfetto.dev or `chrome://tracing`.

Example: no-op `cargo check` run on `cargo-nextest`:

![image](https://github.com/rust-lang/cargo/assets/60961/f21e9c2d-86f6-41b9-8887-562d4fedb4e8)

TODO
- [ ] Find a place to document this, see https://doc.crates.io/contrib/tests/profiling.html#internal-profiler

### How should we test and review this PR?

I looked in `config/` and didn't see a env variable parser to reuse (`get_cv_with_env` is too specialized).

### Additional information

In past projects, I've been able to use features like this to better understand code bases, bugs, etc.  Hopefully this can evolve to help on-board people one day.

Most of the implementation was inspired by https://github.com/arxanas/git-branchless/blob/2923924dfbff07faa61d50771b6604cb18e64bc8/git-branchless-invoke/src/lib.rs#L55

`tracing-chrome` hasn't received updates recently, but it is also fairly quiet on Issues and PRs, so this might just be "maturity".  It does have over a million downloads.  It also is only enabled if explicitly opted into.

rustup added trace visualization in rust-lang/rustup#3287 using [open telemetry](https://crates.io/crates/tracing-opentelemetry) but it is behind a feature flag and and requires running a server to receive the traces.
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.

2 participants