Skip to content

fix(otel-patch): convert incompatible types - #724

Merged
mfocko merged 1 commit into
packit:mainfrom
mfocko:fix/type-issues-with-telemetry
Jul 30, 2026
Merged

fix(otel-patch): convert incompatible types#724
mfocko merged 1 commit into
packit:mainfrom
mfocko:fix/type-issues-with-telemetry

Conversation

@mfocko

@mfocko mfocko commented Jul 30, 2026

Copy link
Copy Markdown
Member

Quite frequent warning from the Sentry logs:

Invalid type OpenInferenceSpanKindValues for attribute 'openinference.span.kind' value. Expected one of ['bool', 'str', 'bytes', 'int', 'float'] or a sequence of those types

Therefore adjust the patch to convert incompatible types via str.

Assisted-by: Claude Opus 4.6

Fixes PACKIT-5261

@mfocko
mfocko requested a review from nforro July 30, 2026 09:48
@mfocko mfocko self-assigned this Jul 30, 2026
@qodo-for-packit

Copy link
Copy Markdown

PR Summary by Qodo

Fix OTEL patch: coerce non-primitive span attributes to strings

🐞 Bug fix 🕐 10-20 Minutes

Grey Divider

AI Description

• Prevent OpenTelemetry warnings by coercing unsupported span attribute value types to strings.
• Keep list/tuple attributes when all elements are OTEL-supported primitives.
• Apply the conversion in the BeeAI instrumentor before setting span attributes.
Diagram

graph TD
  A["BeeAIInstrumentor"] --> B["Node attributes"] --> C{"Value type ok?"} --> E["OTel span attrs"] --> F["Sentry / backend"]
  C --> D["str(value)"] --> E
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Drop invalid attributes instead of coercing
  • ➕ Avoids potentially misleading stringified values in telemetry
  • ➕ Keeps attribute schema cleaner and more query-friendly
  • ➖ Loses diagnostic context when the attribute is still useful in string form
  • ➖ Harder to debug without seeing the original value at all
2. Coerce enums/objects via a dedicated mapping (.value / known serializers)
  • ➕ Preserves more semantic meaning than generic str() for known types
  • ➕ More consistent formatting across producers
  • ➖ Higher maintenance (needs updates as new types appear)
  • ➖ Hard to cover unknown third-party objects without falling back to str() anyway
3. Fix upstream emitter to only produce OTEL-compatible types
  • ➕ Prevents the issue at the source; avoids patch-layer responsibility
  • ➕ May reduce downstream ambiguity in attribute typing
  • ➖ May require upstream dependency changes/release; slower to ship
  • ➖ Not always feasible if values originate in external libraries

Recommendation: The PR’s approach (type-check + str() fallback) is the best short-term mitigation for noisy warnings because it is localized, low-risk, and preserves observability data. If a small set of known non-primitive types (e.g., enums) is common, consider a follow-up to serialize those more semantically (e.g., .value) while keeping str() as a final fallback.

Files changed (1) +8 / -3

Bug fix (1) +8 / -3
openinference-streaming.patchCoerce unsupported OTEL span attribute types to strings +8/-3

Coerce unsupported OTEL span attribute types to strings

• Adds an OTEL-compatible type guard before setting span attributes. If an attribute value is not a primitive OTEL type (or a list/tuple of such primitives), it is converted via str(value) to prevent invalid-type warnings downstream.

openinference-streaming.patch

@qodo-for-packit

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 7 rules

Grey Divider


Action required

1. Sanitization happens too late 🐞 Bug ≡ Correctness
Description
The newly added OTEL type conversion only runs in _end_otel_span, but the OTEL span is created
earlier in _start_otel_span with attributes=dict(processor.span.attributes) and
openinference_span_kind=processor.span.kind, and events/child spans are also created with raw
attributes. As a result, unsupported attribute values can still be passed into OTEL APIs before this
conversion runs, so warnings/dropped attributes can still occur despite the PR.
Code

openinference-streaming.patch[R82-88]

++        _OTEL_TYPES = (bool, str, bytes, int, float)
+        for key, value in node.attributes.items():
++            if not isinstance(value, _OTEL_TYPES) and not (
++                isinstance(value, (list, tuple)) and all(isinstance(v, _OTEL_TYPES) for v in value)
++            ):
++                value = str(value)
+            span.set_attribute(key, value)
Relevance

●● Moderate

Bugfix aligns with goal, but needs earlier sanitization; no close repo precedent on OTEL span
creation paths.

PR-#228
PR-#584

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The conversion is only applied in _end_otel_span when calling span.set_attribute, but earlier
code paths still pass raw attributes/kind into OTEL span/event creation APIs. Since this patch is
applied in the container build and the instrumentor is used by setup_observability, the incomplete
sanitization affects runtime telemetry.

openinference-streaming.patch[53-64]
openinference-streaming.patch[81-96]
openinference-streaming.patch[110-115]
Containerfile.c10s[70-89]
ymir/agents/observability.py[87-96]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The added `_OTEL_TYPES` / `str(value)` fallback is applied only during `_end_otel_span`, after spans/events may already have been created with unsupported attribute types. This makes the PR’s warning reduction incomplete because incompatible values can still be sent to OpenTelemetry at span creation time (root span, inline child spans) and when adding events.

## Issue Context
`openinference-streaming.patch` is applied during container builds to the installed `openinference-instrumentation-beeai` package. The BeeAI instrumentor is used by `ymir/agents/observability.py`, so any remaining invalid OTEL attribute types will continue to affect runtime telemetry.

## Fix Focus Areas
- openinference-streaming.patch[53-69]
- openinference-streaming.patch[81-129]
- openinference-streaming.patch[110-120]

### Concrete fix approach
1. Add a small helper in the patched `__init__.py` (represented inside this patch) to sanitize attribute values *before* passing them to OTEL:
  - Accept OTEL primitives `(bool, str, bytes, int, float)`
  - For list/tuple: sanitize each element; if any element needs coercion, coerce elements to `str` (or drop invalid ones consistently).
  - For other types: coerce to `str`.
2. Apply this helper:
  - When building the `attributes=` dict passed into `_tracer.start_span(...)` in `_start_otel_span`.
  - When calling `span.add_event(..., attributes=...)` for both root and child spans.
  - When creating child spans in `_build_inline_child` (`attributes=node.attributes`).
3. Also normalize `openinference_span_kind` before passing it into `start_span` (e.g., `kind = getattr(kind, "value", kind)` then `str(kind)` if needed), so it cannot be an incompatible object type.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Sequence attributes lose structure 🐞 Bug ⚙ Maintainability
Description
If an attribute value is a list/tuple containing any non-OTEL-primitive element, the new logic
converts the entire sequence to a single string via str(value) rather than preserving an OTEL
array shape. This collapses potentially structured attribute arrays into one string and reduces
queryability/consistency of telemetry attributes.
Code

openinference-streaming.patch[R84-87]

++            if not isinstance(value, _OTEL_TYPES) and not (
++                isinstance(value, (list, tuple)) and all(isinstance(v, _OTEL_TYPES) for v in value)
++            ):
++                value = str(value)
Relevance

●● Moderate

Sequence stringification vs element-wise coercion affects telemetry semantics; team precedent shows
str-coercion, but not for arrays specifically.

PR-#228
PR-#584

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new conditional explicitly falls back to str(value) for any list/tuple that fails the
all(isinstance(v, _OTEL_TYPES) ...) check, which necessarily collapses the entire container into a
single string.

openinference-streaming.patch[82-88]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new sanitizer treats lists/tuples as acceptable only if *all* elements are already OTEL primitives; otherwise it stringifies the entire sequence. This discards list structure even though OTEL supports arrays of primitive values.

## Issue Context
This behavior is introduced by the new `_OTEL_TYPES` check in `_end_otel_span`.

## Fix Focus Areas
- openinference-streaming.patch[82-88]

### Concrete fix approach
Update the sequence handling so that when `value` is a `list`/`tuple`, you sanitize element-by-element and keep the result as a list/tuple of OTEL primitives (commonly `list[str]` when coercion is needed) instead of converting the whole container to a single string.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment thread openinference-streaming.patch
Comment on lines +84 to +87
+ if not isinstance(value, _OTEL_TYPES) and not (
+ isinstance(value, (list, tuple)) and all(isinstance(v, _OTEL_TYPES) for v in value)
+ ):
+ value = str(value)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

2. Sequence attributes lose structure 🐞 Bug ⚙ Maintainability

If an attribute value is a list/tuple containing any non-OTEL-primitive element, the new logic
converts the entire sequence to a single string via str(value) rather than preserving an OTEL
array shape. This collapses potentially structured attribute arrays into one string and reduces
queryability/consistency of telemetry attributes.
Agent Prompt
## Issue description
The new sanitizer treats lists/tuples as acceptable only if *all* elements are already OTEL primitives; otherwise it stringifies the entire sequence. This discards list structure even though OTEL supports arrays of primitive values.

## Issue Context
This behavior is introduced by the new `_OTEL_TYPES` check in `_end_otel_span`.

## Fix Focus Areas
- openinference-streaming.patch[82-88]

### Concrete fix approach
Update the sequence handling so that when `value` is a `list`/`tuple`, you sanitize element-by-element and keep the result as a list/tuple of OTEL primitives (commonly `list[str]` when coercion is needed) instead of converting the whole container to a single string.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Quite frequent warning from the Sentry logs:

    Invalid type OpenInferenceSpanKindValues for attribute 'openinference.span.kind' value. Expected one of ['bool', 'str', 'bytes', 'int', 'float'] or a sequence of those types

Therefore adjust the patch to convert incompatible types via `str`.

Fixes PACKIT-5261

Assisted-by: Claude Opus 4.6
Signed-off-by: Matej Focko <mfocko@packit.dev>
@mfocko
mfocko force-pushed the fix/type-issues-with-telemetry branch from a9b3dc2 to 7689538 Compare July 30, 2026 12:25
@mfocko

mfocko commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

Rebasing and merging.

@mfocko
mfocko merged commit a53037b into packit:main Jul 30, 2026
19 of 20 checks passed
@mfocko
mfocko deleted the fix/type-issues-with-telemetry branch July 30, 2026 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants