Skip to content

[ENH]: add tracing support for tokio console #5239

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

codetheweb
Copy link
Contributor

@codetheweb codetheweb commented Aug 8, 2025

Description of changes

Adds support for tokio-console to all services. This tool is useful when debugging, e.g. it should make it easy to see what futures are "stuck" / waiting on locks. It's disabled by default and can be enabled with an env var.

Test plan

How are these changes tested?

I port-forwarded 6669 from a service in Tilt to my host, then ran the tokio-console application and observed that data was flowing from the service to the tokio console.

Migration plan

Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?

Observability plan

What is the plan to instrument and monitor this change?

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?

Copy link

github-actions bot commented Aug 8, 2025

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@@ -0,0 +1,3 @@
[build]
# Needed to enable tracing support for tokio
rustflags = ["--cfg", "tokio_unstable"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

my understanding of this flag is that it only enables opt-in features that are subject to change without corresponding semver changes

Copy link
Contributor Author

codetheweb commented Aug 8, 2025

2 Jobs Failed:

PR checks / Python tests / test-cluster-rust-frontend (3.9, chromadb/test/property/test_add.py)

No logs available for this step.

PR checks / all-required-pr-checks-passed

No logs available for this step.


Summary: 1 successful workflow, 1 failed workflow

Last updated: 2025-08-12 00:03:05 UTC

@codetheweb codetheweb marked this pull request as ready for review August 8, 2025 20:10
@codetheweb codetheweb requested a review from HammadB August 8, 2025 20:10
Copy link
Contributor

propel-code-bot bot commented Aug 8, 2025

Add Tokio Console Tracing Support and Refactor Tracing Initialization

This PR adds support for the tokio-console tracing backend to all Rust services in the Chroma project. This enables runtime inspection and debugging of asynchronous tasks, such as identifying stuck futures or lock contention, by integrating the console-subscriber crate. The integration is gated by the ENABLE_TOKIO_CONSOLE environment variable, which opts services into exporting tracing data for the tokio-console application. The tracing initialization logic is refactored to bring tokio-console subscriber support under a single init_otel_tracing function, and dependency management is updated to ensure console-subscriber is available. Associated documentation, Docker build, and workspace build configurations are updated to support and build with these new features.

Key Changes

• Adds console-subscriber as a dependency in the workspace and to the chroma-tracing crate
• Introduces a new tracing initialization approach via init_otel_tracing that conditionally registers the tokio-console subscriber if ENABLE_TOKIO_CONSOLE is set
• Refactors all services to use the unified init_otel_tracing initialization (removing old function layering logic)
• Updates .cargo/config.toml to set rustflags = [--cfg, ``tokio_unstable``] globally, required for tokio-console support
• Updates Dockerfile and Tiltfile to include .cargo/ configs in build contexts for container builds
• Updates Cargo.lock to reflect new dependency versions and workspace-wide changes

Affected Areas

• Rust workspace dependency management (Cargo.toml, Cargo.lock)
• Tracing initialization in all Rust services (frontend, garbage_collector, chroma-tracing, etc.)
• Docker build process, Tiltfile local dev workflow, and Dockerfile dependency resolution
• Chroma-tracing crate API and usage
• .cargo/config.toml (Rust compiler flags)

This summary was automatically generated by @propel-code-bot


let subscriber = tracing_subscriber::registry()
.with(layers.with_filter(get_env_filter(custom_filters)))
.with(
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be config? we don't really use env vars elsewhere but i could see it maybe being fine here. is this a standardized env var?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a strong preference
it's only standardized in the sense it's the same across all our services
common debug config loosely makes sense to me to have in env vars, but maybe we should just define a new common tracing config struct

Copy link
Collaborator

@HammadB HammadB Aug 12, 2025

Choose a reason for hiding this comment

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

ok, in that case i would ask that it is config since our config system supports env var overrides, and having a single source of truth is preferable.

fmt::layer().pretty().with_target(false).boxed()
}

pub fn init_tracing(layers: Vec<Box<dyn Layer<Registry> + Send + Sync>>) {
Copy link
Collaborator

@HammadB HammadB Aug 12, 2025

Choose a reason for hiding this comment

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

does hosted/ depend on this code? It should be checked for dep issues, with a fix PR being up before this is merged


tracing::subscriber::set_global_default(subscriber)
.expect("Should be able to set global tracing subscriber");
tracing::info!("Global tracing subscriber set");
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there any parent span that will actually capture this info?

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