-
Notifications
You must be signed in to change notification settings - Fork 2k
[CHORE]: hide private service names in otel/tracing init #5075
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
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
Refactor Tracing/Otel Initialization with Configurable Crate Filters and Hidden Private Services This PR refactors tracing and OpenTelemetry (Otel) initialization logic across the Rust codebase to support hiding private service names, modularize crate-level filter configuration, and provide per-service or per-crate filter overrides at runtime. The core change introduces a typed filter configuration (OtelFilter, OtelFilterLevel) allowing each service to specify a set of crate+filter rules, which are combined with global defaults, thereby separating OSS configuration concerns from private code and enabling per-deployment visibility policies. This PR is the first step in a two-part rollout and requires coordinated deployment with a corresponding change in hosted-chroma to fully ensure private service names are hidden from OSS. Key Changes• Introduces OtelFilter and OtelFilterLevel types to specify tracing filter rules per crate. Affected Areas• rust/tracing (init_tracer.rs, lib.rs) This summary was automatically generated by @propel-code-bot |
…per/chore-hide-private-services
| let config = GarbageCollectorConfig { | ||
| service_name: "gc".to_string(), | ||
| otel_endpoint: "none".to_string(), | ||
| otel_filters: vec![], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clean nit - should this be the default? You can do ...default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GC config currently doesn't implement Default. IMO that's a separate concern from this PR. Alternative options are to leave as an empty array, hard-code the same default filters as are hard-coded in the default fn, or make the default fn public. Of these, I think hard-coding the same value makes sense since it's only one element.
| #[serde(default = "CompactionServiceConfig::default_otel_endpoint")] | ||
| pub otel_endpoint: String, | ||
| #[serde(default = "CompactionServiceConfig::default_otel_filters")] | ||
| pub otel_filters: Vec<OtelFilter>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this look in yaml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this:
otel_filters:
- crate_name: some_crate
filter_level: debug
- crate_name: other_crate
filter_level: trace| "query_service", | ||
| "wal3", | ||
| "worker", | ||
| "continuous_verification", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will have to update this in hosted/ with this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HammadB
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, did you test forwards /backwards compatibility of config parsing to avoid rollout issues? Can you add reasoning / test plan followed for why its safe?
…per/chore-hide-private-services
…per/chore-hide-private-services
## Description of changes Adds the new tracing init pattern introduced in #5075 to sample YAML files to demonstrate how these values should be supplied in config. ## Test plan - [x] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust ## Migration plan N/A ## Observability plan Will monitor honeycomb in staging to make sure we are still getting traces for all services. ## Documentation Changes N/A
…#5075) ## Description of changes Currently, the OSS code is responsible for initializing tracing/otel for all crates. A global filter is applied in which for all crates that initialize tracing, every filter is applied for that crate. This PR slightly refactors this pattern to define a set of "global default crate filters" and allow initializers to pass in additional crate filters (and even overrides) at their call sites. ## Test plan This PR is the first in a stack with https://github.com/chroma-core/hosted-chroma/pull/3345. Together, these refactor the tracing and otel initialization to move private service names into our private code. Forward/backward compatibility would require a dance in which we doubly-initialize filters for the same crates, which feels like more complexity than it's worth given the scope of these changes. Therefore, these two PRs should be rolled out together. - [x] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust ## Documentation Changes N/A
## Description of changes Adds the new tracing init pattern introduced in chroma-core#5075 to sample YAML files to demonstrate how these values should be supplied in config. ## Test plan - [x] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust ## Migration plan N/A ## Observability plan Will monitor honeycomb in staging to make sure we are still getting traces for all services. ## Documentation Changes N/A
Description of changes
Currently, the OSS code is responsible for initializing tracing/otel for all crates. A global filter is applied in which for all crates that initialize tracing, every filter is applied for that crate. This PR slightly refactors this pattern to define a set of "global default crate filters" and allow initializers to pass in additional crate filters (and even overrides) at their call sites.
Test plan
This PR is the first in a stack with https://github.com/chroma-core/hosted-chroma/pull/3345. Together, these refactor the tracing and otel initialization to move private service names into our private code. Forward/backward compatibility would require a dance in which we doubly-initialize filters for the same crates, which feels like more complexity than it's worth given the scope of these changes. Therefore, these two PRs should be rolled out together.
pytestfor python,yarn testfor js,cargo testfor rustDocumentation Changes
N/A