feat: expose OTel semantic convention version in trace metadata#166
feat: expose OTel semantic convention version in trace metadata#166AaronProbha18 wants to merge 4 commits into
Conversation
krisztianfekete
left a comment
krisztianfekete
left a comment
There was a problem hiding this comment.
Thanks for the PR, it's going in the correct direction as per our discussion on the issue. Added some comments!
| """ | ||
| try: | ||
| with open(path) as f: | ||
| with open(path, encoding="utf-8") as f: |
There was a problem hiding this comment.
This is not really part of this exercise, and since it's incomplete, I'd rather have this done properly in a follow-up PR.
Can you pull this one out and open another PR with the changes covering all relevant paths?
There was a problem hiding this comment.
makes sense!
I've reverted the changes in this PR and raised a new PR that covers all the relevant UTF-8 encoding updates here: #167
| # Matches a semver-like segment (e.g. "1.37.0") anywhere in a schema URL such | ||
| # as "https://opentelemetry.io/schemas/1.37.0". Deliberately permissive since | ||
| # schema_url values are free-form OTel-defined strings, not something we | ||
| # control the shape of. | ||
| _SEMCONV_VERSION_RE = re.compile(r"(\d+\.\d+\.\d+)") |
There was a problem hiding this comment.
This is incorrect. Can you please follow the definition described here: https://opentelemetry.io/docs/specs/otel/schemas/#schema-url
There was a problem hiding this comment.
We'd also want a test for this.
| if match: | ||
| return match.group(1) | ||
|
|
||
| return "unknown" |
There was a problem hiding this comment.
Since TraceConversionMetadata.semconv_version is typed, this can never be null, but "unknown". Let's return None and serialise to null.
| scope = scope_span.get("scope") or scope_span.get("instrumentationLibrary") or {} | ||
| scope_name = scope.get("name", "") | ||
| scope_version = scope.get("version", "") | ||
| schema_url = scope_span.get("schemaUrl", "") |
There was a problem hiding this comment.
OTLP defines schemaUrl at both ScopeSpans and ResourceSpans. This reads only the scope level, so a trace setting it only at the resource level reports "unknown". resource_span is already in scope from the loop above. Scope stays first as the more specific signal.
| schema_url = scope_span.get("schemaUrl", "") | |
| schema_url = scope_span.get("schemaUrl", "") or resource_span.get("schemaUrl", "") |
| scope = scope_span.get("scope", {}) | ||
| scope_name = scope.get("name", "") | ||
| scope_version = scope.get("version", "") | ||
| schema_url = scope_span.get("schemaUrl", "") |
There was a problem hiding this comment.
This is the same as the loader path so both ingest routes should behave the same. See my other comment in otlp.py.
| model: str | None = None | ||
| response_model: str | None = None | ||
| provider: str | None = None | ||
| semconv_version: str | None = None |
There was a problem hiding this comment.
Per the schema spec, the version number is scoped to a Schema Family (the URL prefix before the version). So basically two URLs with the same version but different hosts are different schemas, and "OpenTelemetry schema version numbers match OpenTelemetry Semantic Conventions version numbers" is only true for the OTel official family. So a bare "1.40.0" from a custom family (e.g. https://randomcompany.internal/schemas/1.40.0) would be recorded as if it were OTel semconv 1.40.0.
Since this field is informational and never drives logic, I wouldn't hard-gate on the host (mirrors exist, and the full otel.schema_url attribute is already preserved for anyone who needs the family). But please make this clear and add note in the field description / the resolve_semconv_version docstring that this is the version segment of the emitting scope's schema URL, comparable only within a schema family and equal to the OTel semconv version for the OTel family.
We could even call it schema_version, since "semconv" asserts the OTel family already.
| metadata["response_model"] = ext["response_model"] | ||
| if ext["provider"]: | ||
| metadata["provider"] = ext["provider"] | ||
| metadata["semconv_version"] = ext["semconv_version"] |
There was a problem hiding this comment.
Here, ext comes from llm_spans[0] only, so a trace mixing instrumentors at different versions will only record the first. It's fine, but please add a comment calling this out.
|
|
||
| def test_extract_trace_metadata_semconv_version_from_valid_schema_url(self): | ||
| trace = _make_tool_trace(["tool_a"]) | ||
| trace.all_spans[1].tags["otel.schema_url"] = "https://opentelemetry.io/schemas/1.39.0" |
There was a problem hiding this comment.
Nit (also on line 271): this tags all_spans[1] by position and relies on that being the span the extractor reads. If we change _make_tool_trace ordering, the test passes for the wrong reason or breaks in a weird way. Let's select the LLM span by the same predicate the extractor uses and tag that.
| assert traces[0]["traceId"] == "dd547580319ab0312cee07f1def50dad" | ||
| assert len(traces[0]["invocations"]) == 1 | ||
|
|
||
| @patch("agentevals.api.routes.extract_trace_metadata") |
There was a problem hiding this comment.
Mocking extract_trace_metadata like this verifies the wiring and camelCase serialization, which is good, but can we please also cover OTLP with scopeSpans.schemaUrl to /api/convert to semconvVersion, so we have something e2e as well?
2 participants
Description
schema_urlto exposesemconv_versionin conversion metadata.extract_*_from_attrs) without rewriting attributes during ingestion.gen_ai.provider.name→gen_ai.system).semconv_versionextraction, metadata propagation, and API serialization.Related Issues
Closes #162
Notes
otel.schema_urlis treated as generic OTel metadata and not as a GenAI detection signal.