[OPIK-7717] [BE] fix: map OTel semconv provider names to Opik canonical providers - #7909
[OPIK-7717] [BE] fix: map OTel semconv provider names to Opik canonical providers#7909awkoy wants to merge 5 commits into
Conversation
…al providers
Custom OTel instrumentation reports the provider via `gen_ai.system` using the
semantic-convention vocabulary, which spells several providers differently from
Opik's price-table vocabulary. OpenTelemetryMapper copied the value verbatim into
the span's `provider`, so `vertex_ai` never reached the `google_vertexai` price
row and the span silently cost 0 — even though `gemini-3.1-flash-lite` is priced
correctly under the canonical name. Only the generic `google` value was special
cased (GoogleProviderResolver); CostService aliased the hyphenated `vertex-ai`
but not the OTel-standard underscore form.
Add GenAiProviderAliasResolver for the unambiguous 1:1 renames, each confirmed to
price at 0 before and correctly after:
vertex_ai, gcp.vertex_ai -> google_vertexai
gcp.gemini -> google_ai
aws.bedrock -> bedrock
az.ai.openai,
azure.ai.openai -> azure
mistral_ai -> mistral
x_ai -> xai
Both spellings are carried where the semconv renamed a value but instrumentation
still emits the old one (`vertex_ai` -> `gcp.vertex_ai`, `az.ai.openai` ->
`azure.ai.openai`). The rewrite happens at ingestion rather than at cost lookup
because the raw value is also persisted to the `provider` column, so fixing only
the lookup would leave filtering and grouping split across two names for
identical traffic.
Values naming more than one backend are deliberately excluded. `gcp.gen_ai`
("specific backend is unknown" per the semconv) joins the generic `google` in
GoogleProviderResolver's server.address disambiguation. `azure.ai.inference` /
`az.ai.inference` front either Azure OpenAI or Foundry models such as Claude and
Llama, which LiteLLM prices under a separate `azure_ai` provider absent from
CostService.PROVIDERS_MAPPING; aliasing them to `azure` would price a Foundry
model against the OpenAI table.
Also map `gen_ai.provider.name`, which replaced the now-deprecated
`gen_ai.system` and was not read at all, leaving newer instrumentation with no
provider. Instrumentations mid-migration emit both attributes with differing
vocabularies (`xai` vs `x_ai`), so `gen_ai.system` stays authoritative and the
new attribute only fills in when absent — keeping this strictly additive for
spans that already resolve a provider.
Note this is forward-only: cost is computed once at ingestion and stored, so
spans already persisted with cost 0 are not re-priced.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
java
⏱️ pre-commit per-hook timing
⏭️ 42 skipped (no matching files changed)
|
|
No test needed here. This PR already ships the test that would catch it: OpenTelemetryResourceTest.testProviderVocabularyIsAliasedAndPriced POSTs real OTLP to /v1/private/otel/v1/traces and asserts both the persisted provider and a non-zero total_estimated_cost for all eight aliases, which is exactly the user-visible outcome (a Vertex span showing $0 in Logs). The UI side is a pass-through of that number, and trace-explore/trace-spans-depth.spec.ts (@cap:traces.span-model-cost-tokens) already asserts the span panel renders model, tokens and cost. Nothing for a Playwright spec to add here. Worth noting separately: the e2e estate has no OTLP ingestion coverage at all and taxonomy.yaml has no capability for it — a gap for the coverage radar, not something to hang on this PR. Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. Re-checked after a push on 20 Aug 10:53 UTC. |
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
…hosts on a domain boundary Addresses review findings on the OTel provider aliasing change. A blank or non-string `gen_ai.provider.name` yields "" from getStringValue(), and the fallback assigned it unconditionally — persisting an empty provider on a span that previously carried none at all. Require both sides to be non-blank. GoogleProviderResolver matched its host markers with `contains`, so an address such as `evil-aiplatform.googleapis.com.attacker.test` classified as Vertex AI. Reduce `server.address` to its bare host and match on a suffix instead. Scheme, path, numeric port and a trailing FQDN dot are stripped first so well-formed addresses are not rejected by the stricter match, and an IPv6 literal is not truncated at its last colon. Also collapse the two DEBUG records the unrecognized-host path emitted into one. Tests: 266 pass (13 new), covering embedded-marker hosts that must not classify as Vertex, well-formed addresses with port/scheme/path/FQDN dot that must still resolve, and blank provider-name values that must neither persist nor override. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
…ank-provider tests Addresses follow-up review findings. GenAiProviderAliasResolver matched aliases on the trimmed value but returned the original on a miss, so a padded " openai " persisted verbatim and missed the price-table lookup, which keys on the exact provider string. Return the trimmed value either way so both paths are consistent. Case is deliberately left alone: lowercasing would rewrite stored providers that are usable for filtering today. Also convert the blank-provider regression test to a @ParameterizedTest, matching the surrounding provider tests, and cover padded values in both directions. Tests: 269 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🌙 Nightly cleanup: The test environment for this PR ( |
The existing alias coverage stops at the mapper and CostService in-process. Nothing posted OTLP over HTTP and then read the persisted span back, so a regression in the resource/ingestion wiring could land with every test green. Parameterized over the alias table plus the gen_ai.provider.name fallback and one ambiguous google + server.address case. 7 of the 8 fail against main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Backend Tests - Integration Group 12222 tests 221 ✅ 4m 3s ⏱️ For more details on these failures, see this check. Results for commit c0931dc. ♻️ This comment has been updated with latest results. |
| Stream<Arguments> testProviderVocabularyIsAliasedAndPriced() { | ||
| return Stream.of( | ||
| // The OPIK-7717 report: stored verbatim, 'vertex_ai' matched no price row and cost 0. | ||
| arguments("vertex_ai", "gen_ai.system", "vertex_ai", "gemini-3.1-flash-lite", null, |
There was a problem hiding this comment.
Backend test violates naming convention
testProviderVocabularyIsAliasedAndPriced uses a test... prefix, and its implicit @MethodSource resolves by that name, so renaming only the test breaks discovery before any cases run. Should we rename the test and factory together (or use @MethodSource("...")), as apps/opik-backend/AGENTS.md and .agents/skills/opik-backend/testing.md require?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/OpenTelemetryResourceTest.java`
around lines 431-434, rename the `testProviderVocabularyIsAliasedAndPriced` parameter
source and its corresponding parameterized test to descriptive method-style names
without the `test` prefix. Keep both names identical, or add an explicit `@MethodSource`
name, because the current implicit factory lookup depends on matching method names.
| // Names no backend on its own, so it is resolved from the endpoint host instead. | ||
| arguments("google + vertex host", "gen_ai.system", "google", "gemini-2.5-flash-lite", | ||
| "us-east1-aiplatform.googleapis.com", "google_vertexai")); |
There was a problem hiding this comment.
Fallback rationale is unclear
Endpoints without a backend name resolve from the host instead, so let's verify that path works end-to-end.
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/OpenTelemetryResourceTest.java
around lines 445-447, update the comment in the testProviderVocabularyIsAliasedAndPriced
method. Replace “Names no backend on its own” with “Names no backend on their
own” so the sentence is grammatically correct and clearly explains that the provider
is resolved from the endpoint host.
| assertThat(persistedSpan.totalEstimatedCost()) | ||
| .as("cost stored for model %s under provider %s", model, expectedProvider) | ||
| .isNotNull() | ||
| .isGreaterThan(BigDecimal.ZERO); |
There was a problem hiding this comment.
totalEstimatedCost() only checks that the value is greater than zero, so an incorrect pricing row or gemini-3.1-flash-lite fallback under google_vertexai can pass — should we add an expected cost (e.g. $0.001000 for the ticket case) to each Arguments entry and assert exact equality with the existing scale-aware BigDecimal comparison?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/OpenTelemetryResourceTest.java
around lines 494-497, update `testProviderVocabularyIsAliasedAndPriced` and its
method-source cases so the pricing regression test validates the exact expected cost
instead of merely requiring a positive value. Add an expected `BigDecimal` cost argument
for every provider/model case (calculated deterministically for 1,000 input and 500
output tokens from the loaded pricing table, e.g. `$0.001000` for the documented ticket
case), and assert `persistedSpan.totalEstimatedCost()` equals that value exactly, using
the same scale-aware BigDecimal comparison convention as the existing cost tests.
Backend Tests - Integration Group 14 30 files 30 suites 5m 27s ⏱️ Results for commit c0931dc. ♻️ This comment has been updated with latest results. |
| */ | ||
| @UtilityClass | ||
| @Slf4j | ||
| public class GenAiProviderAliasResolver { |
There was a problem hiding this comment.
We should probably abstract all those resolvers and find a way select the ones that must be applied
| private static String extractHost(String serverAddress) { | ||
| String host = serverAddress; | ||
|
|
||
| int scheme = host.indexOf("://"); | ||
| if (scheme >= 0) { | ||
| host = host.substring(scheme + 3); | ||
| } | ||
| int path = host.indexOf('/'); | ||
| if (path >= 0) { | ||
| host = host.substring(0, path); | ||
| } | ||
| // Only strip a genuine numeric port, so an IPv6 literal isn't truncated at its last colon. | ||
| int port = host.lastIndexOf(':'); | ||
| if (port >= 0 && port < host.length() - 1 | ||
| && host.substring(port + 1).chars().allMatch(Character::isDigit)) { | ||
| host = host.substring(0, port); | ||
| } | ||
|
|
||
| return StringUtils.removeEnd(host, "."); | ||
| } |
There was a problem hiding this comment.
Better to use a URLParser
thiagohora
left a comment
thiagohora
left a comment
There was a problem hiding this comment.
Automated review of the provider-alias changes. Two findings worth acting on (span mistyping, Anthropic-on-Vertex pricing), plus a doc gap and a test that doesn't exercise what it claims.
Checked and clean: rule precedence/placement (gen_ai.system is the only other PROVIDER rule; no earlier ruleset matches the new key), the blank guards avoiding persisted "", the fallback ordering that keeps gen_ai.provider.name=elastic routing through ElasticInferenceServiceResolver, extractHost scheme/path/port/IPv6/trailing-dot handling and the contains→endsWith tightening, the alias set against the OTel GenAI registry, every model asserted in the new tests against model_prices_and_context_window.json, and the coverage lost by deleting GoogleProviderOtelPipelineTest (carried by OtelProviderCostPipelineTest).
| private static final Map<String, String> ALIASES = Map.ofEntries( | ||
| // Legacy spellings the semconv renamed but instrumentation still emits (OPIK-7717): | ||
| // `vertex_ai` -> `gcp.vertex_ai`, `az.ai.openai` -> `azure.ai.openai`. | ||
| Map.entry("vertex_ai", GoogleProviderResolver.GOOGLE_VERTEX_AI), |
There was a problem hiding this comment.
correctness — vertex_ai → google_vertexai isn't the unambiguous 1:1 rename the javadoc describes.
Vertex fronts Anthropic models, which Opik prices under a different canonical provider: anthropic_vertexai, keyed vertex_ai-anthropic_models in CostService.PROVIDERS_MAPPING (asserted in CostServiceTest:143).
Failure: gen_ai.system=vertex_ai + gen_ai.request.model=claude-haiku-4-5 now stores google_vertexai. findModelPrice("claude-haiku-4-5", "google_vertexai") misses every fallback — the prefix fallback needs a / in the model name, and PROVIDERS_MAPPING has no vertex_ai key — so the span still costs $0, and is now additionally mislabeled and grouped/filtered as a Google provider.
This is precisely the multi-backend ambiguity for which azure.ai.inference was deliberately excluded, so the exclusion rationale is applied inconsistently here. Consider disambiguating by model family (claude* → anthropic_vertexai), or at minimum documenting the gap. Same applies to the gcp.vertex_ai entry below.
| resolved = GOOGLE_AI; | ||
| log.debug("Provider 'google' with unrecognized server.address '{}', defaulting to '{}'", | ||
| serverAddress, resolved); | ||
| recognized = false; |
There was a problem hiding this comment.
low — with gcp.gen_ai added to AMBIGUOUS_PROVIDERS, this default branch now affirmatively mislabels rather than leaving the value alone.
Previously a gcp.gen_ai span with no (or an unrecognized) server.address was stored verbatim as gcp.gen_ai — cost 0, but obviously unmapped. It now stores google_ai, so a Vertex call from an instrumentation that omits server.address is persisted as Gemini Developer API traffic: provider filtering and grouping silently attribute it to the wrong backend, and if the two price tables ever diverge the cost is wrong rather than merely absent.
The class javadoc carries this caveat for google; it should state it for gcp.gen_ai too.
| } | ||
|
|
||
| @ParameterizedTest(name = "[{index}] {0} -> google_vertexai") | ||
| @CsvSource({"VERTEX_AI", "Vertex_Ai", " vertex_ai "}) |
There was a problem hiding this comment.
test-coverage — the whitespace half of this test is never exercised.
JUnit's @CsvSource defaults to ignoreLeadingAndTrailingWhitespace = true for unquoted columns, so " vertex_ai " reaches the mapper as vertex_ai and merely duplicates the plain lowercase case. A regression in whitespace handling would still pass.
Quote it — "' vertex_ai '" — as paddedProviderIsStoredTrimmed correctly does.
| // `gen_ai.system` authoritative when present; see PROVIDER_NAME_ATTR there. | ||
| OpenTelemetryMappingRule.builder() | ||
| .rule(PROVIDER_NAME_ATTR).source(SOURCE).outcome(OpenTelemetryMappingRule.Outcome.PROVIDER) | ||
| .spanType(SpanType.llm).build(), |
There was a problem hiding this comment.
correctness — .spanType(SpanType.llm) on this rule will mistype execute_tool spans.
Unlike gen_ai.system, gen_ai.provider.name is defined in current semconv on execute_tool and invoke_agent spans too, not only inference spans. enrichSpanWithAttributes applies rule.getSpanType() unconditionally while walking the attribute list, so the last spanType-bearing rule wins.
Failure: an execute_tool span from an instrumentation that has fully migrated to the new attribute (so it emits no gen_ai.system) carrying only gen_ai.tool.name (METADATA, no spanType) is typed llm instead of tool — and the type also flips whenever gen_ai.provider.name is iterated after gen_ai.tool.call.arguments. Before this PR the key had no rule at all, so such a span kept tool/general deterministically.
invoke_agent is protected by the explicit operation-name guard in OpenTelemetryMapper (~L281); execute_tool has no equivalent. Either drop spanType from this rule, or extend that guard to cover execute_tool.
4 participants
Details
OTel instrumentation reports the provider via
gen_ai.systemusing the semantic-convention vocabulary, which spells several providers differently from Opik's price table.OpenTelemetryMappercopied the value verbatim, sovertex_ainever matched thegoogle_vertexaiprice row and the span silently cost $0 — even though the same model prices correctly under the canonical name.Same model, same usage, only the wire value differs:
gen_ai.systemgoogle+ aiplatform hostgoogle_vertexaivertex_aivertex_aiNew
GenAiProviderAliasResolvernormalizes the semconv vocabulary at ingestion:It also starts reading
gen_ai.provider.name, which replaced the deprecatedgen_ai.systemand wasn't read at all. When a span carries both,gen_ai.systemwins — so no span that resolves a provider today can change behaviour.Why at ingestion, not at cost lookup: the raw value is also persisted to the
providercolumn, so fixing only the lookup would leave filtering and grouping split across two names for identical traffic.Deliberately not aliased:
gcp.gen_ai— semconv defines it as "specific backend is unknown", so it's disambiguated byserver.addresslike the existing genericgoogle.azure.ai.inference— fronts both Azure OpenAI and Foundry models (Claude, Llama), which LiteLLM prices under a separateazure_aiprovider; aliasing it toazurewould price a Foundry model against the OpenAI table.gemini,vertex-ai— already price correctly today; rewriting them would change stored data on a working path and silently break savedproviderfilters.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
13 of the 18 pipeline cases failed before the fix, so that suite is a real regression test rather than a restatement of current behaviour.
Covered: every alias; both
gcp.gen_aibranches; padded values; hosts merely embedding a marker (must not classify as Vertex); hosts with port / scheme / path (must still resolve); the precedence guard (gen_ai.system=xai+gen_ai.provider.name=x_ai→xai); and the two deliberate exclusions.Ingestion-path regression test
All of the above stops at the mapper and
CostServicein-process. Nothing posted OTLP over HTTP and then read the persisted span back, so a regression in the resource/ingestion wiring could have landed with every test still green.OpenTelemetryResourceTest#testProviderVocabularyIsAliasedAndPricedcloses that: real OTLP protobuf to/v1/private/otel/v1/traces, then the persisted span read back, asserting the storedproviderand a non-zerototal_estimated_cost.7 of the 8 cases fail against
main, so this is a real regression test — includinggen_ai.provider.name, which resolved to anullprovider before this PR. The eighth (google+ Vertex host) already worked and is carried as a guard against regressing it.This also fills a gap the QA test radar flagged on this PR: nothing in the tagged e2e estate posts to the OTel endpoint, and the one spec tagged
traces.span-model-cost-tokensseedsproviderandtotal_costthrough the SDK, so it would pass even if every alias here were wrong. Adding the coverage in the backend resource test asserts the exact derived values rather than the UI's rounded<$0.01, and needs no new QA-owned taxonomy capability.End-to-end, manually, against a backend built from this branch: real OTLP spans posted to
/v1/private/otel/v1/traces, then persisted spans read back through the public API — 22/22 pass. The ticket's exact case (vertex_ai+gemini-3.1-flash-lite) storesgoogle_vertexaiand costs$0.001000, confirmed at both span and trace level.Documentation
None needed — internal ingestion fix with no API, configuration or SDK surface change. Spans that showed $0 simply start showing the correct cost.