fix(otel): share one AnyValue decoder across all OTLP paths - #187
Conversation
Three AnyValue decoders had drifted apart. extraction's
flatten_otlp_attributes silently dropped array/kvlist/bytes attributes,
and the OTLP JSON loader json.dumps()'d the raw proto wrapper instead of
decoding it. gen_ai.response.finish_reasons therefore surfaced as
'{"values": [{"stringValue": "stop"}]}' on one path and vanished on
another, where both should yield ["stop"].
Move the already-correct recursive decoder out of api/otlp_processing.py
into a dependency-free module and route all three call sites through it.
The new module imports only the standard library, so extraction,
loader.otlp and api.otlp_processing can all share it without creating an
import cycle.
bytesValue is returned unchanged: MessageToDict base64-encodes protobuf
bytes fields, so callers already receive a str today. Decoding it here
would change existing behaviour, which is out of scope for this fix.
Adds coverage for array/kvlist/bytes attributes, which previously had
none on either path.
Fixes agentevals-dev#173
e007c14 to
c65f82d
Compare
|
Rebased onto latest main. CI hasn't run on this PR — looks like it's waiting on the first-time-contributor workflow approval. Could a maintainer kick it off? Happy to address anything it turns up. |
krisztianfekete
left a comment
krisztianfekete
left a comment
There was a problem hiding this comment.
Thanks, added two comments, can you please take a look at them?
There was a problem hiding this comment.
Here we still hand roll stringValue only handling. Strands puts gen_ai.input.messages in span events, and newer GenAI semconv makes messages a complex array, so that promotion still silently drops anything that isn't a plain string. Can you please fix this as well?
|
|
||
| def _extract_agentevals_metadata(resource_attrs: list[dict]) -> dict: | ||
| """Extract agentevals-specific metadata from OTLP resource attributes.""" | ||
| flat = flatten_otlp_attributes(resource_attrs) |
There was a problem hiding this comment.
flatten_otlp_attributes used to guarantee scalar or absent. After this change can return a list or dict, and _extract_agentevals_metadatafeeds agentevals.session_name straight into self._active_session_for_name.get(session_name) in ws_server.py and otlp_processing.py for logs.
Can we read session_name and eval_set_id with a string only accessor, the way _extract_conversation_id already does in otlp_processing.py?
Strands stores gen_ai.input.messages in span events, and newer GenAI semconv makes messages a complex array, so the stringValue-only promotion in the OTLP loader silently dropped anything that was not a plain string. Route it through the shared decoder like the other paths.
flatten_otlp_attributes used to guarantee a scalar or nothing. Now that it delegates to the shared AnyValue decoder it can also return lists and dicts, which are unhashable and raise TypeError where these values are used as dict keys (_active_session_for_name in ws_server.py and otlp_processing.py). Add a string-only accessor and use it for both fields, matching what _extract_conversation_id already did; that function now shares the same implementation. The decoded value stays available in resource_attrs.
|
Both addressed, thanks — the second one was a real regression I missed. Event attribute promotion ( I checked what that widens downstream: the promoted values are consumed via
I left Six tests added; the three regression ones fail if I revert the two source CI still hasn't run on this branch — no checks reported. Happy to address |
| Delegates to the shared ``AnyValue`` decoder so array/kvlist/bytes | ||
| attributes survive instead of being dropped. | ||
| """ | ||
| return decode_attributes(attrs_list) |
There was a problem hiding this comment.
This is the widening. span.tags now carries lists into every consumer, and the same unhashable crash as session_name survives at other places as well.
| return span | ||
|
|
||
|
|
||
| def _extract_string_attribute(attrs_list: list[dict], key: str) -> str | None: |
There was a problem hiding this comment.
Right fix, but it's per field and the hazard is per context. Can we coerce once for attrs the spec types as strings instead of adding an accessor per call site as we find them?
| }, | ||
| ] | ||
| ) | ||
| assert result == {"gen_ai.response.finish_reasons": ["stop"]} |
There was a problem hiding this comment.
Stops at the decoder. Nothing asserts finish_reasons == ["stop"] out of extract_extended_model_info_from_attrs, which is the symptom #173 calls out.
| "value": {"arrayValue": {"values": [{"stringValue": "stop"}]}}, | ||
| } | ||
| ) | ||
| assert span.tags["gen_ai.response.finish_reasons"] == ["stop"] |
There was a problem hiding this comment.
Same, stops at span.tags
The per-field accessor only covered the two dict-key sites we had found; flatten_otlp_attributes still handed containers to every other consumer via span.tags, so the same unhashable hazard survived elsewhere. Move the guard into decode_attributes, keyed by SPEC_STRING_ATTRS in trace_attrs. Attributes the spec types as strings come back str-or-absent; a container in one of those slots is dropped with a warning rather than JSON-dumped, since dumping would put the literal blob back into user-visible output. Attributes the spec types as arrays or structured values keep their decoded containers, which is what agentevals-dev#173 fixes. The registry is hand-maintained: opentelemetry-semantic-conventions exposes constants and prose docstrings only, so attribute value types are not machine-readable and the set cannot be derived from the package. _extract_string_attribute is gone and _extract_conversation_id is back to its original form. Tests now assert through extract_extended_model_info_from_attrs and extract_user_text_from_attrs, the layers agentevals-dev#173's symptom actually names, instead of stopping at the decoder and span.tags.
|
Reworked along the lines you suggested — the per-field accessor is gone. Coercion moved to the decode boundary. It's a split rather than a blanket coercion: attributes the spec types as The registry is hand-maintained, with a note in the code saying why. I — so the value type isn't machine-readable. Happy to revisit if you know of a One judgement call you didn't specify: when a string-typed attribute Behaviour change worth flagging: non-string values in string-typed slots no Tests moved up a layer, both places you pointed at:
Two of the new tests fail if I revert this round's source changes; the 768 passed, 6 skipped locally, ruff clean. One thing I left alone: the nested-dict path in the loader |
| # Attributes the spec types as arrays or structured values are deliberately | ||
| # absent - gen_ai.response.finish_reasons, gen_ai.input/output.messages, | ||
| # gen_ai.tool.definitions and gen_ai.system_instructions must keep their | ||
| # decoded containers, which is the whole point of #173. Numeric attributes are | ||
| # likewise absent; the decoder already returns them as int/float. | ||
| SPEC_STRING_ATTRS: frozenset[str] = frozenset( |
There was a problem hiding this comment.
The set covers gen_ai.* and agentevals.* but no gcp.vertex.agent.*. ADK_INVOCATION_ID is a dict key, so an arrayValue still gives an error. Confirmed.
|
You're right, and it's worse than one entry: I haven't pushed a fix for it. I think the list itself is the problem, and What I got wrong. #173 asks for one shared decoder so the lost values Here's what the two paths actually did before this PR: # extraction.py - containers dropped entirely
if "stringValue" ... elif "intValue" ... elif "doubleValue" ... elif "boolValue" ...
return result
# loader/otlp.py - containers JSON-dumped as raw proto
elif "arrayValue" in value_obj:
result[key] = json.dumps(value_obj["arrayValue"])So What I'd like to do instead: invert the list to opt-in. SPEC_CONTAINER_ATTRS = frozenset({
OTEL_GENAI_RESPONSE_FINISH_REASONS,
OTEL_GENAI_INPUT_MESSAGES,
OTEL_GENAI_OUTPUT_MESSAGES,
OTEL_GENAI_TOOL_DEFINITIONS,
OTEL_GENAI_SYSTEM_INSTRUCTIONS,
})Scalars (including bytesValue, which decodes to a hashable str) always decoded; The Costs, so you can weigh them:
And a question I think is separate from #173: what should the shared Which way do you want it? |
Thanks for checking, let's do the inversion! A missing denylist entry crashes ingestion on an unauthenticated port; a missing allowlist entry drops one value, which is what the extraction layer does already. I checked your proposal and none reach a dict key or set, so the crash goes away structurally, that has been my intention. Yes to both Yes to separate issues for the arbitrary container question and One thing the inversion won't fix though, is that |
…list SPEC_STRING_ATTRS listed the attributes to narrow, so a missing entry let an unhashable value reach a dict key on an unauthenticated receiver port. It missed the whole gcp.vertex.agent.* namespace. Replace it with SPEC_CONTAINER_ATTRS: the seven keys the GenAI semconv types as arrays or structured values. Those decode to native containers; every other key decodes to a scalar or is dropped, which is what extraction.py did before the decoder was shared. A missing entry now drops one value rather than crashing ingestion, and attributes neither of us has thought about are safe by construction rather than by enumeration. decode_attribute() carries the rule so the OTLP-array path and the span-event promotion path share one implementation. bytesValue now survives as the base64 string it already was, which agentevals-dev#173 asks for and is hashable. The default for unlisted container values is tracked in agentevals-dev#208; the nested-dict loader path that bypasses this decoder entirely is agentevals-dev#207.
|
Inverted, and I checked the seven independently rather than take it on trust: every reference One detail I'd add: genai_model = span.get_tag(OTEL_GENAI_REQUEST_MODEL)
if genai_model:
models_used.add(genai_model)It's safe, because Two things to flag:
Issues filed as agreed: #208 for the default behaviour on unlisted CI still hasn't run here, so I reproduced the workflow locally, including
|
2 participants
Fixes #173
Problem
Three places decoded the OTLP
AnyValueunion, and only one did it fully:arrayValue/kvlistValuebytesValueextraction.flatten_otlp_attributesloader.otlp.OtlpJsonLoader._extract_attributesjson.dumpsof the raw proto wrapperapi.otlp_processing._parse_otlp_any_valueFeeding one span attribute —
gen_ai.response.finish_reasonscarryingarrayValue: ["stop"]— through the receiver paths produced three different answers before this change:OtlpJsonLoader'{"values": [{"stringValue": "stop"}]}'["stop"]OtlpJsonLoader'{"values": [{"stringValue": "stop"}]}'["stop"]["stop"]["stop"]["stop"]Note that even after a
json.loads, the loader's value is still the proto wrapper — not the decoded list.Approach
Moved the already-correct recursive decoder into a new
agentevals/otlp_anyvalue.pyand pointed all three call sites at it. The gRPC receiver needs no change: it handsMessageToDictoutput to the sameprocess_traces.The new module imports only the standard library. That is deliberate:
extractionimportsloader.base, which eagerly initialises theloaderpackage (and thereforeloader.otlp), so havingloader.otlpimport fromextractionwould create a real import cycle. A leaf module has no edge back into the package and cannot participate in one.Two behaviours are intentionally preserved:
bytesValueis returned unchanged.MessageToDictbase64-encodes protobuf bytes fields and OTLP/JSON does the same, so call sites already receive astr. Decoding to real bytes would be a behaviour change beyond this fix.{}—is_any_value()keeps the prior semantics of both flatteners.The loader's dict-shaped attribute branch (
_flatten_nested_dict, for ClickHouse-style nested JSON) is untouched; only the OTLP array branch now shares the decoder.Testing
array/kvlist/bytesattributes had no test coverage on either path — which is how the mismatch survived. Added 7 tests acrosstests/test_extraction.pyandtests/test_otlp_loader.py, including the nestedarrayValue-of-kvlistValueshape used for tool calls.758 passed, 6 skipped (unit suite)
ruff check . / ruff format --check . clean