[opentelemetry-instrumentation-genai-dspy] Add instrumentation for inference invocations - #683
Open
DylanRussell wants to merge 3 commits into
Open
[opentelemetry-instrumentation-genai-dspy] Add instrumentation for inference invocations#683DylanRussell wants to merge 3 commits into
opentelemetry-instrumentation-genai-dspy] Add instrumentation for inference invocations#683DylanRussell wants to merge 3 commits into
Conversation
Pull request dashboard statusWaiting on the author · refreshed 2026-09-11 20:19 UTC Respond to 4 review items (e.g. link a commit, explain why not, ask a follow-up): Status above doesn't look right?
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Multiple moderate issues remain in operation naming, duplicate-span suppression, concurrent response attribution, choice handling, tool parsing, and numeric conversion.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds DSPy LM sync/async inference instrumentation with structured request, response, usage, multimodal, and tool-call extraction.
Changes:
- Patches
dspy.LM.__call__andacall. - Adds unit and conformance tests.
- Documents LM instrumentation support.
File summaries
| File | Reviewed changes |
|---|---|
instrumentation/opentelemetry-instrumentation-genai-dspy/tests/test_lm.py |
LM instrumentation and parsing tests |
instrumentation/opentelemetry-instrumentation-genai-dspy/tests/test_conformance.py |
Registers LM conformance coverage |
instrumentation/opentelemetry-instrumentation-genai-dspy/tests/conformance/lm.py |
Defines the LM conformance scenario |
instrumentation/opentelemetry-instrumentation-genai-dspy/src/opentelemetry/instrumentation/genai/dspy/utils.py |
Parses providers, parameters, usage, messages, multimodal content, and tool parts; moderate parsing and numeric-conversion issues remain |
instrumentation/opentelemetry-instrumentation-genai-dspy/src/opentelemetry/instrumentation/genai/dspy/patch.py |
Instruments LM calls; moderate operation naming, deduplication, concurrency, and multi-choice handling issues remain |
instrumentation/opentelemetry-instrumentation-genai-dspy/README.rst |
Documents LM instrumentation |
Review details
Suppressed comments (5)
instrumentation/opentelemetry-instrumentation-genai-dspy/src/opentelemetry/instrumentation/genai/dspy/patch.py:353
historyis shared by the LM instance, sohistory[-1]is not guaranteed to belong to this invocation when the same LM is used concurrently from threads or async tasks. A later call can overwrite the model, usage, response ID, and finish reason read here, causing telemetry to be attributed to the wrong request; use call-specific response metadata or another per-invocation association instead of the global last entry.
# DSPy 3.x LM calls return a legacy list by default unless experimental=True
# or an LMRequest is used. DSPy appends each call's metadata to instance.history,
# so history[-1] corresponds to the invocation that just finished.
finish_reason: str | None = None
history: Sequence[Mapping[str, Any]] | None = getattr(
instance, "history", None
)
if isinstance(history, Sequence) and history:
instrumentation/opentelemetry-instrumentation-genai-dspy/src/opentelemetry/instrumentation/genai/dspy/utils.py:172
- Using
orhere treats an empty argument object as absent. A valid tool call withargs={}is therefore recorded witharguments=None, losing the model-supplied empty arguments; use an explicitis Nonefallback when selectingargsversusarguments.
name = str(tc.get("name", ""))
args_raw = tc.get("args") or tc.get("arguments")
instrumentation/opentelemetry-instrumentation-genai-dspy/src/opentelemetry/instrumentation/genai/dspy/utils.py:192
- The object path has the same empty-arguments loss:
LMToolCallPart(args={})is converted toNoneby the truthiness fallback. Preserve an explicitly supplied empty mapping by checking whetherargsisNonebefore falling back toarguments.
args = None
if capture_content:
args = getattr(tc, "args", None) or getattr(tc, "arguments", None)
instrumentation/opentelemetry-instrumentation-genai-dspy/src/opentelemetry/instrumentation/genai/dspy/utils.py:75
safe_intis used while starting every inference for values such asmax_tokens,seed, andn, butint(float("inf"))raisesOverflowError, which is not caught here. A non-finite configuration value therefore makes the instrumentation raise before DSPy is called instead of honoring this helper's safe-conversion contract.
if isinstance(val, (int, float, str, bytes)):
try:
return int(val)
except (ValueError, TypeError):
return None
instrumentation/opentelemetry-instrumentation-genai-dspy/src/opentelemetry/instrumentation/genai/dspy/utils.py:85
safe_floathas the same unhandledOverflowErrorcase when a very large integer is converted to a float. Since this helper runs in telemetry setup before the wrapped call, such a value can change the user's DSPy call into an instrumentation exception.
if isinstance(val, (int, float, str, bytes)):
try:
return float(val)
except (ValueError, TypeError):
return None
- Files reviewed: 7/7 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+231
to
+243
| if hasattr(dspy, "LM"): | ||
| lm_module = dspy.LM.__module__ | ||
| lm_name = dspy.LM.__name__ | ||
| _wrap_function( | ||
| lm_module, | ||
| f"{lm_name}.__call__", | ||
| _lm_call(handler), | ||
| ) | ||
| _wrap_function( | ||
| lm_module, | ||
| f"{lm_name}.acall", | ||
| _lm_acall(handler), | ||
| ) |
Comment on lines
+280
to
+283
| invocation = handler.inference( | ||
| provider=provider, | ||
| request_model=request_model, | ||
| ) |
Comment on lines
+373
to
+378
| choices = _get_field(resp_obj, "choices") | ||
| if isinstance(choices, Sequence) and choices: | ||
| fr = _get_field(choices[0], "finish_reason") | ||
| if fr: | ||
| finish_reason = str(fr) | ||
| invocation.finish_reasons = [finish_reason] |
Comment on lines
+330
to
+336
| elif p_type_str == "tool_result" or hasattr(p, "call_id"): | ||
| call_id = getattr(p, "call_id", None) | ||
| content = getattr(p, "content", None) | ||
| return ToolCallResponsePart( | ||
| id=str(call_id) if call_id else None, | ||
| response=content, | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Monkey patches
dspy.LM.__call__/acallso that they emit inference spans. Most of the logic is in parsing the input/output messages.We probably want to wait until #663 lands to avoid duplicate inference spans/events, but that will not impact the implementation here so we should get this PR ready at least.
I think this is the last big gap between our instrumentation and open inferences except that Open inference also monkey patches
PredictandModulemethods, but we don't have sem convs matching those and i don't think it makes sense to add them, as they are building blocks that seem very specific todspyand don't seem generalizable..We could add non-sem-conv compliant spans for those methods. That is worth discussing.. Maybe I'll propose a PR and see what people think.
Type of change
How has this been tested?
Unit tests
Checklist
See CONTRIBUTING.md
for the style guide, changelog guidance, and more.