fix(tracing): add vendor reporting for OTEL tracing#64
Conversation
- Introduced `set_vendor` method in `OtelTracer` to set vendor attributes. - Integrated vendor name retrieval in chat, completion, and embedding functions based on the model provider type.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughA new method to set vendor information was added to the OpenTelemetry tracer, with logic to map provider types to standardized vendor names. This mapping is now used in tracing calls within pipeline functions. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Pipeline
participant Model
participant Provider
participant OtelTracer
Client->>Pipeline: chat_completions/completions/embeddings()
Pipeline->>Model: select model
Model->>Provider: get provider type
Pipeline->>Provider: provider.r#type()
Pipeline->>OtelTracer: set_vendor(get_vendor_name(provider_type))
Pipeline->>Provider: call provider method
Provider-->>Pipeline: response
Pipeline-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 748f0f7 in 1 minute and 5 seconds. Click for details.
- Reviewed
301lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/pipelines/otel.rs:176
- Draft comment:
Ensure GEN_AI_SYSTEM is defined and imported correctly. Document the purpose of set_vendor() since it affects trace attributes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%The comment is asking to ensure that GEN_AI_SYSTEM is defined and imported correctly, which is not allowed as it falls under the category of asking the author to ensure something. However, the second part of the comment is asking to document the purpose of a function, which is a valid request for clarity and documentation purposes.
2. src/pipelines/pipeline.rs:115
- Draft comment:
Consider centralizing the vendor setting logic as it's repeated in each endpoint. If future changes are needed, using a helper function might reduce duplication. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_HpMCzVpheRkYDNZV
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 60f8047 in 32 seconds. Click for details.
- Reviewed
38lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/pipelines/otel.rs:176
- Draft comment:
Formatting change: Chained call in set_vendor is more readable. Ensure consistency with other similar chained calls. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/pipelines/otel.rs:404
- Draft comment:
Minor whitespace removal after asserting vendor name mapping; no change in logic. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/pipelines/otel.rs:421
- Draft comment:
Test set_vendor method: Consider verifying that the span attribute is actually set rather than only asserting true. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_YMjeh2rNy64munNA
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/pipelines/otel.rs (2)
176-178: LGTM: vendor attribute is set on the spanSetting GEN_AI_SYSTEM with the vendor string is correct and keeps concerns encapsulated in OtelTracer. If you adopt Cow<'static, str> for get_vendor_name, you can call with vendor.as_ref() here to stay allocation-free for known mappings.
411-427: Strengthen test: current smoke test doesn’t assert behaviortest_set_vendor_method_exists only asserts true. Prefer a test that verifies the attribute was actually set (e.g., initialize a testing TracerProvider/exporter and assert the span contains GEN_AI_SYSTEM with the expected value).
I can sketch an in-memory exporter setup with opentelemetry_sdk to assert span attributes if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/pipelines/otel.rs(2 hunks)src/pipelines/pipeline.rs(5 hunks)src/providers/provider.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: build
src/pipelines/otel.rs
[error] 174-174: cargo fmt check failed due to formatting differences. Please run 'cargo fmt' to fix code style issues.
[error] 403-403: cargo fmt check failed due to formatting differences. Please run 'cargo fmt' to fix code style issues.
[error] 416-416: cargo fmt check failed due to formatting differences. Please run 'cargo fmt' to fix code style issues.
[error] 423-423: cargo fmt check failed due to formatting differences. Please run 'cargo fmt' to fix code style issues.
🔇 Additional comments (1)
src/pipelines/pipeline.rs (1)
114-116: Ignore outdated vendor‐mapping suggestionI couldn’t find any
Providertrait implementations,r#type()methods, or aget_vendor_namefunction in the current codebase. It looks like the original comments about normalizing provider types and adjusting the call totracer.set_vendor(...)no longer apply.Please disregard that part of the review, or reintroduce/rename those functions if you still intend to support provider-based vendor mapping.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed c67f237 in 1 minute and 40 seconds. Click for details.
- Reviewed
879lines of code in20files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/types/mod.rs:20
- Draft comment:
Good refactor of the provider type: replacing raw strings with a typed enum improves type safety. Consider whether the mapping in Display (which yields lowercase values) should be harmonized with the vendor names returned by get_vendor_name (which mix cases) for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment has two parts: praising the enum refactor (which isn't actionable) and pointing out case inconsistency. The case inconsistency appears to be intentional - Display is used for serialization (hence lowercase) while get_vendor_name is used for display/branding (hence proper case). These serve different purposes, so the inconsistency is likely by design. I might be wrong about the intended purposes of these two methods. There could be places in the code where the inconsistency causes real problems. Without evidence that the case inconsistency causes actual issues, this appears to be a speculative comment about potential problems rather than a clear issue that needs fixing. The comment should be deleted as it combines non-actionable praise with a speculative suggestion about case consistency that appears to be intentionally different for valid reasons.
2. src/providers/provider.rs:38
- Draft comment:
Since the vendor mapping always returns a freshly allocated String, consider returning a &'static str instead to avoid unnecessary allocation if the mapping is static. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
3. src/providers/vertexai/provider.rs:56
- Draft comment:
The get_auth_token implementation uses anexpectto require credentials; ensure that in production scenarios this panics is acceptable or consider graceful error handling. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. tests/config_hash_tests.rs:10
- Draft comment:
Hash implementations sort keys to ensure order independence; this is a solid approach. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
5. General:0
- Draft comment:
Overall, the PR consistently updates provider type usage across modules and tests. The changes bring enhanced type safety and more robust vendor reporting in tracing. Great work on updating tests across the board! - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_Iw6gjXqevWObHmyR
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/providers/provider.rs (1)
37-46: Fix OTEL vendor/system mapping: mixed casing and non-standard valuesThe current values are inconsistent and likely non-compliant with OTEL GenAI conventions. For OpenTelemetry, attribute values such as gen_ai.system are expected to be lowercase, underscore-delimited identifiers (e.g., "openai", "anthropic", "azure_openai", "aws_bedrock", "vertex_ai"). Returning "Azure"/"Anthropic"/"AWS"/"Google" mixes casing and semantics (vendor vs system).
If set_vendor feeds the OTEL gen_ai.system (or a similar standardized attribute), switch to the enumerated, lowercase values. Minimal fix (keeping String):
pub fn get_vendor_name(provider_type: &ProviderType) -> String { match provider_type { - ProviderType::OpenAI => "openai".to_string(), - ProviderType::Azure => "Azure".to_string(), - ProviderType::Anthropic => "Anthropic".to_string(), - ProviderType::Bedrock => "AWS".to_string(), - ProviderType::VertexAI => "Google".to_string(), + ProviderType::OpenAI => "openai".to_string(), + ProviderType::Azure => "azure_openai".to_string(), + ProviderType::Anthropic => "anthropic".to_string(), + ProviderType::Bedrock => "aws_bedrock".to_string(), + ProviderType::VertexAI => "vertex_ai".to_string(), } }Optionally, avoid allocations by returning &'static str or Cow<'static, str>.
Please confirm which OTEL attribute set_vendor writes. If it’s intended as a human-readable “vendor” label instead of gen_ai.system, then normalize consistently (e.g., "OpenAI", "Azure", "Anthropic", "AWS", "Google") and keep casing uniform across all branches.
To verify where the value is used and the expected attribute key, run:
#!/bin/bash # Inspect usage of set_vendor and related OTEL attributes rg -n "set_vendor\(" -A 3 rg -n "gen_ai\.system|llm\.vendor|vendor" -A 3
🧹 Nitpick comments (7)
src/types/mod.rs (1)
47-60: Minor: prefer to_ascii_lowercase for ASCII-only matchingUsing s.to_lowercase() allocates and applies Unicode case folding. For ASCII tokens, to_ascii_lowercase() is cheaper and avoids locale surprises.
- match s.to_lowercase().as_str() { + match s.to_ascii_lowercase().as_str() {src/pipelines/pipeline.rs (6)
7-7: Importing vendor-mapping here is OK; consider reducing cross-module couplingUsing
get_vendor_namein the pipeline is fine, but you could push the mapping concern into the tracer to avoid pipelines depending onproviders::provider. For example, addset_vendor_from_provider(ProviderType)onOtelTracerand keep pipelines unaware of mapping details.Possible follow-up in tracer (outside this file):
impl OtelTracer { pub fn set_vendor_from_provider(&mut self, p: ProviderType) { self.set_vendor(get_vendor_name(&p)); } }Then update call sites in this file (see comments below).
114-116: Set vendor timing is correct; optional: let tracer handle mappingGood placement (right after model match, before invoking provider). To reduce repetition and keep mapping centralized, consider:
- // Set vendor now that we know which model/provider we're using - tracer.set_vendor(&get_vendor_name(&model.provider.r#type())); + // Set vendor now that we know which model/provider we're using + tracer.set_vendor_from_provider(model.provider.r#type());Note: requires adding
set_vendor_from_providertoOtelTraceras shown above.
153-155: Duplicate vendor-setting logic across endpointsSame feedback as chat: this is correct, but consider delegating mapping to the tracer to DRY the code.
- tracer.set_vendor(&get_vendor_name(&model.provider.r#type())); + tracer.set_vendor_from_provider(model.provider.r#type());
180-182: Duplicate vendor-setting logic (embeddings)Also correct here. Same optional refactor applies.
- tracer.set_vendor(&get_vendor_name(&model.provider.r#type())); + tracer.set_vendor_from_provider(model.provider.r#type());
430-577: Reduce duplication across test providers
TestProviderOpenAI,TestProviderAnthropic, andTestProviderAzureare nearly identical aside fromr#type()and the dummy response’s model string. Consider a single parametric test provider holding aProviderType(and maybe akey), to simplify maintenance and addition of new providers.Example:
#[derive(Clone)] struct TestProvider { key: String, kind: ProviderType, model_name: String, } impl Provider for TestProvider { fn new(_config: &ProviderConfig) -> Self { unimplemented!() } fn key(&self) -> String { self.key.clone() } fn r#type(&self) -> ProviderType { self.kind } // chat_completions returns NonStream with model_name; others NOT_IMPLEMENTED }Then instantiate it with different
ProviderTypevalues in tests.
590-606: Provider type methods tests: good coverage; consider adding a tracer integration checkThese assertions validate the provider types and mapping. If feasible, add a small integration test around
OtelTracerto assert the span’s vendor attribute is set (using a test tracer or exposing span attributes in a test build). I can help scaffold a test double forOtelTracerif you want to exercise theset_vendorcall through the pipeline path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/config/validation.rs(3 hunks)src/management/dto.rs(1 hunks)src/management/services/config_provider_service.rs(1 hunks)src/pipelines/otel.rs(2 hunks)src/pipelines/pipeline.rs(9 hunks)src/providers/anthropic/provider.rs(2 hunks)src/providers/azure/provider.rs(2 hunks)src/providers/bedrock/provider.rs(2 hunks)src/providers/bedrock/test.rs(4 hunks)src/providers/openai/provider.rs(2 hunks)src/providers/provider.rs(2 hunks)src/providers/registry.rs(2 hunks)src/providers/vertexai/provider.rs(2 hunks)src/providers/vertexai/tests.rs(7 hunks)src/types/mod.rs(2 hunks)tests/config_hash_tests.rs(6 hunks)tests/pipeline_header_routing_test.rs(1 hunks)tests/router_cache_tests.rs(8 hunks)tests/router_integration_test.rs(6 hunks)tests/unified_openapi_test.rs(2 hunks)
✅ Files skipped from review due to trivial changes (8)
- tests/router_integration_test.rs
- tests/config_hash_tests.rs
- src/config/validation.rs
- src/providers/bedrock/test.rs
- src/providers/vertexai/tests.rs
- src/providers/openai/provider.rs
- tests/router_cache_tests.rs
- src/providers/anthropic/provider.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pipelines/otel.rs
🧰 Additional context used
🧬 Code Graph Analysis (6)
tests/unified_openapi_test.rs (2)
src/providers/provider.rs (1)
r#type(16-16)src/providers/openai/provider.rs (1)
r#type(43-45)
src/providers/bedrock/provider.rs (5)
src/providers/anthropic/provider.rs (1)
r#type(33-35)src/providers/azure/provider.rs (1)
r#type(50-52)src/providers/provider.rs (1)
r#type(16-16)src/providers/openai/provider.rs (1)
r#type(43-45)src/providers/vertexai/provider.rs (1)
r#type(132-134)
src/providers/vertexai/provider.rs (6)
src/providers/anthropic/provider.rs (1)
r#type(33-35)src/providers/bedrock/provider.rs (1)
r#type(132-134)src/providers/azure/provider.rs (1)
r#type(50-52)src/pipelines/pipeline.rs (4)
r#type(235-237)r#type(446-448)r#type(493-495)r#type(540-542)src/providers/provider.rs (1)
r#type(16-16)src/providers/openai/provider.rs (1)
r#type(43-45)
src/providers/azure/provider.rs (6)
src/providers/anthropic/provider.rs (1)
r#type(33-35)src/providers/bedrock/provider.rs (1)
r#type(132-134)src/pipelines/pipeline.rs (4)
r#type(235-237)r#type(446-448)r#type(493-495)r#type(540-542)src/providers/provider.rs (1)
r#type(16-16)src/providers/openai/provider.rs (1)
r#type(43-45)src/providers/vertexai/provider.rs (1)
r#type(132-134)
src/providers/provider.rs (6)
src/providers/anthropic/provider.rs (3)
new(21-27)key(29-31)r#type(33-35)src/providers/bedrock/provider.rs (3)
new(122-126)key(128-130)r#type(132-134)src/providers/azure/provider.rs (3)
new(39-44)key(46-48)r#type(50-52)src/providers/registry.rs (1)
new(17-32)src/providers/openai/provider.rs (3)
new(32-37)key(39-41)r#type(43-45)src/providers/vertexai/provider.rs (3)
new(105-126)key(128-130)r#type(132-134)
src/types/mod.rs (1)
src/management/dto.rs (2)
fmt(434-440)from_str(446-453)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (19)
tests/pipeline_header_routing_test.rs (2)
2-4: Enum import migration looks goodUsing ProviderType in tests aligns with the refactor and keeps type-safety.
10-10: Provider r#type switched to enum variant — LGTMProviderType::OpenAI correctly replaces the previous string literal.
src/management/services/config_provider_service.rs (1)
187-188: Type-safety improvement acknowledged — verify external serialization compatibilityAssigning dto.provider_type directly (enum) improves type safety. Ensure:
- Provider and ProviderResponse use the same enum type.
- Serde derives on ProviderType keep wire format stable (e.g., lowercased strings) for OpenAPI/DB boundaries.
If external JSON previously used string values, confirm the enum uses serde rename or tag to preserve those values.
tests/unified_openapi_test.rs (2)
3-5: Importing ProviderType is consistent with the refactorMatches the trait method r#type() returning ProviderType across providers.
130-130: Provider r#type updated to enum — looks correctProviderType::OpenAI is the right variant here; no further adjustments needed in this test.
src/providers/bedrock/provider.rs (2)
12-12: Imports updated to bring ProviderType into scope — OKKeeps the implementation aligned with the trait signature.
132-134: Return ProviderType::Bedrock — aligned with trait changesThis matches other providers and enables centralized vendor mapping.
If get_vendor_name is used for OTEL attributes, confirm the chosen label for Bedrock (e.g., "aws" vs "bedrock") matches your observability conventions to avoid inconsistent dashboards.
src/providers/vertexai/provider.rs (2)
11-11: Imports updated for ProviderType — goodConsistent with the codebase-wide enum migration.
132-134: Return ProviderType::VertexAI — LGTMConsistent with other provider implementations and supports vendor mapping.
Double-check vendor naming used by get_vendor_name for Vertex AI (e.g., "google" vs "vertexai") to ensure consistent OTEL reporting across vendors.
src/management/dto.rs (1)
6-7: Centralizing ProviderType is correct and improves cohesionRe-exporting ProviderType from crate::types removes duplication and aligns DTOs with the core type. LGTM.
src/providers/azure/provider.rs (1)
50-52: Return strongly-typed ProviderTypeSwitching r#type to return ProviderType::Azure matches the trait change and the registry’s enum-based dispatch. Looks good.
src/providers/registry.rs (1)
21-27: Enum-based provider dispatch is safer and exhaustiveMatching on ProviderType variants removes stringly-typed bugs and makes the match exhaustive (future variants will be compile-time enforced). Good change.
src/providers/provider.rs (1)
16-16: Trait now returns ProviderType — correct migrationThe Provider trait’s r#type signature matches the new enum-based approach. This is consistent across implementations.
src/types/mod.rs (2)
62-72: Provider.r#type moved to enum — good changeUsing ProviderType in the Provider struct strengthens type-safety and simplifies matching in the registry.
35-46: Display implementation aligns with serde-renamed valuesThe
Displayimpl forProviderTypereturns the exact strings defined by your serde renames (azure,openai,anthropic,bedrock,vertexai), so no updates are necessary.src/pipelines/pipeline.rs (4)
235-237: ProviderType enum adoption in MockProvider looks goodReturning
ProviderType::OpenAIaligns with the trait change and keeps tests simple.
269-269: Tests updated to use ProviderType in ProviderConfigSolid update; keeps tests aligned with the strongly-typed provider identification.
343-350: Multiple provider test configs: enum usage is consistentBoth providers updated to
ProviderType::OpenAI. Looks consistent for the test scenario.
579-588: Standardize vendor attribute values and verify intended mappingThe expected values mix cases (“openai” vs “Anthropic”, “Azure”) and map cloud providers as “AWS”/“Google” (vs “bedrock”/“vertexai” as described in the PR objectives). For OTEL attributes, consistent, low-cardinality, normalized strings are preferable. Please confirm intended values across the project and dashboards, then standardize.
Suggestions:
- Option A (normalized slugs): "openai", "anthropic", "azure", "aws", "google"
- Option B (proper names): "OpenAI", "Anthropic", "Azure", "AWS", "Google"
If you choose A, adjust
get_vendor_nameand these assertions accordingly.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 2d0a06b in 49 seconds. Click for details.
- Reviewed
30lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/providers/provider.rs:39
- Draft comment:
Suggestion: Since all vendor names are static string literals, consider returning &'static str instead of Cow<'static, str> to simplify the API, unless dynamic allocation is anticipated in the future. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. src/providers/provider.rs:41
- Draft comment:
Note: Verify consistency in vendor name capitalization (e.g., 'openai' is lowercase while others are not) to ensure it matches OTEL reporting standards. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_6FjmNNyICsJ1Zgfl
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Important
Adds vendor reporting for OTEL tracing and refactors provider types to use a strongly typed enum for improved trace clarity and type safety.
otel.rsandpipeline.rs.provider.rs.ProviderTypeintypes/mod.rs.otel.rsandpipeline.rsto verify vendor name mapping and integration with provider types.ProviderTypeenum.This description was created by
for 2d0a06b. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor