Skip to content

Conversation

@duanyutong
Copy link

@duanyutong duanyutong commented Dec 2, 2025

Changes

  • add clear flag to support two instrumentation modes
  • This is useful for replacing the default OpenAI instrumentation (enabled by default) with this one
  • I have added tests that cover my changes.
  • If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
  • PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
  • (If applicable) I have updated the documentation accordingly.

Important

Add clear flag to OpenAIAgentsInstrumentor._instrument to support replacing existing trace processors.

  • Behavior:
    • Add clear flag to _instrument method in OpenAIAgentsInstrumentor to support two instrumentation modes.
    • If clear is True, existing trace processors are replaced with the new one using set_trace_processors.
    • If clear is False, the new processor is added to existing ones using add_trace_processor.
  • Documentation:
    • Update docstring for _instrument to describe the clear flag and its effect.

This description was created by Ellipsis for 346cace. You can customize this summary. It will automatically update as commits are pushed.

Summary by CodeRabbit

  • New Features
    • Add an opt-in "clear" configuration to instrumentation that, when enabled, replaces existing trace processors instead of appending; default (append) behavior remains unchanged.
  • Chores
    • Update repository ignore list to exclude the Yarn cache directory (.yarn).

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 2, 2025

Walkthrough

Adds a constructor flag clear: bool and updates _instrument(self, **kwargs) -> None to optionally replace the SDK trace processor set using set_trace_processors([otel_processor]) when clear is true; otherwise retains add_trace_processor(otel_processor). Also adds .yarn/ to .gitignore.

Changes

Cohort / File(s) Summary
Instrumentation processor control
packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py
Adds __init__(*, clear: bool = False) storing self._clear. Updates _instrument(self, **kwargs) -> None signature and docstring. Imports set_trace_processors and conditionally calls set_trace_processors([otel_processor]) when self._clear is True, otherwise calls add_trace_processor(otel_processor). Tracer/meter-provider handling retained.
Repository ignore list
.gitignore
Adds .yarn/ to the repository ignore list.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Areas to check:
    • Constructor initialization and use of self._clear.
    • Correct import and compatibility of set_trace_processors.
    • _instrument docstring accuracy and preserved public API behavior.

Poem

🐇 I nudged a flag to clear or keep,
Trace processors waking from their sleep,
A hop, a swap, or gentle add,
Small change, but made this rabbit glad,
Carrot code and footprints neat. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and specifically summarizes the main change: adding a clear flag to support two instrumentation modes for OpenAI agents instrumentation.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a77793c and 8a0fece.

📒 Files selected for processing (2)
  • .gitignore (1 hunks)
  • packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • .gitignore
  • packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/init.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to 346cace in 1 minute and 45 seconds. Click for details.
  • Reviewed 45 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py:21
  • Draft comment:
    Consider renaming the 'clear' parameter to something more descriptive (e.g., 'replace_existing') to clarify that it resets existing trace processors.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a subjective naming suggestion. The parameter name clear is short and reasonably descriptive, especially given the comprehensive docstring. The term "clear" is commonly used in programming contexts to mean "remove existing items" (e.g., list.clear(), dict.clear()). While replace_existing might be slightly more descriptive, the current name isn't wrong or confusing. The docstring clearly explains the behavior. This type of comment falls into the category of "code quality refactor" but it's not clearly actionable - it's more of a subjective preference. The rules state to avoid comments that aren't clearly necessary, and naming preferences without strong justification typically fall into this category. The parameter name clear might be ambiguous - does it clear traces, processors, or something else? The suggested name replace_existing would be more self-documenting and reduce the need to read the docstring to understand the parameter's purpose. While replace_existing might be slightly more explicit, the docstring already provides clear documentation, and clear is a common programming idiom. The parameter is keyword-only (enforced by the * in the signature), so users will see clear=True at call sites, which combined with IDE hints showing the docstring, makes the intent clear. This is more of a stylistic preference than a necessary change. This comment is a subjective naming suggestion without strong evidence that the current name is problematic. The parameter has clear documentation and follows common programming conventions. This type of stylistic suggestion doesn't meet the bar for a necessary code change.
2. packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py:57
  • Draft comment:
    The try/except block catches all Exceptions. Consider narrowing it to ImportError or adding logging to avoid masking unexpected errors.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_Yfq0N4uMlr0xmk69

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py (1)

46-59: Consider more specific exception handling.

The broad except Exception on line 57 could silently suppress errors beyond import failures (e.g., runtime errors in set_trace_processors or add_trace_processor). Consider catching ImportError specifically or logging suppressed exceptions for debugging.

Example refactor:

         # Use hook-based approach with OpenAI Agents SDK callbacks
         try:
             from agents import add_trace_processor, set_trace_processors
             from ._hooks import OpenTelemetryTracingProcessor
 
             # Create and add our OpenTelemetry processor
             otel_processor = OpenTelemetryTracingProcessor(tracer)
             if clear:
                 set_trace_processors([otel_processor])
             else:
                 add_trace_processor(otel_processor)
 
-        except Exception:
-            # Silently handle import errors - OpenAI Agents SDK may not be available
+        except ImportError:
+            # OpenAI Agents SDK may not be available
             pass
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between c8c1553 and 346cace.

📒 Files selected for processing (1)
  • packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules

Files:

  • packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py
🧬 Code graph analysis (1)
packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py (1)
packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/_hooks.py (1)
  • OpenTelemetryTracingProcessor (19-641)
🔇 Additional comments (4)
packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py (4)

21-21: Well-designed method signature.

The keyword-only clear parameter with a default of False ensures backward compatibility while preventing accidental positional argument usage. The added return type annotation improves type safety.


22-35: Clear and comprehensive docstring.

The documentation effectively explains the purpose of the clear parameter, its use case for replacing default instrumentation, and the accepted kwargs. Well done.


52-55: Logic correctly implements the two instrumentation modes.

The conditional properly handles both modes: replacing all processors when clear=True or appending when clear=False. This maintains backward compatibility.

However, ensure comprehensive tests are added to cover both branches, especially the clear=True path that replaces existing processors.


47-47: Verify set_trace_processors availability in openai-agents >= 0.2.0.

The set_trace_processors API is documented in the OpenAI Agents SDK tracing module, but the exact version when this API was introduced is not publicly documented. Confirm whether set_trace_processors is available in openai-agents 0.2.0 (the minimum version specified in _instruments). If this API was introduced in a later version, update the minimum dependency version accordingly.

Copy link
Member

@nirga nirga left a comment

Choose a reason for hiding this comment

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

@duanyutong I think you should pass it in the instrumentation initialization

@duanyutong duanyutong force-pushed the feat-openai-agents-clear branch from 346cace to a77793c Compare December 3, 2025 18:45
@duanyutong
Copy link
Author

@duanyutong I think you should pass it in the instrumentation initialization

Sure, done.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py (1)

18-29: Constructor clear flag wiring is correct; consider future‑proofing base init

The clear flag is cleanly added as a keyword‑only argument and stored on self._clear, matching the later usage in _instrument. Since BaseInstrumentor is currently an ABC without __init__, not calling super().__init__() is safe, but you might still consider adding a super().__init__() call for future‑proofing if the base class ever grows initialization logic.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 346cace and a77793c.

📒 Files selected for processing (2)
  • .gitignore (1 hunks)
  • packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules

Files:

  • packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py
🧠 Learnings (1)
📚 Learning: 2025-12-02T21:09:48.690Z
Learnt from: duanyutong
Repo: traceloop/openllmetry PR: 3487
File: packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/utils.py:177-178
Timestamp: 2025-12-02T21:09:48.690Z
Learning: The opentelemetry-instrumentation-openai and opentelemetry-instrumentation-openai-agents packages must remain independent and not share code, so code duplication between them is acceptable.

Applied to files:

  • packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py
🧬 Code graph analysis (1)
packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py (1)
packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/_hooks.py (1)
  • OpenTelemetryTracingProcessor (19-641)
🔇 Additional comments (2)
.gitignore (1)

34-34: Ignore .yarn/ cache directory – looks good

Adding .yarn/ is a standard, low‑risk ignore entry and has no impact on runtime behavior.

packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py (1)

34-65: Verify minimum version requirement for set_trace_processors API

The concern about version compatibility is valid. Web search reveals that set_trace_processors is documented in the official Agents SDK tracing API and is compatible with the signature set_trace_processors([processor]). However, the 0.2.0 release notes make no mention of trace processors functionality — only a typing change for AgentBase. This suggests set_trace_processors may have been introduced after 0.2.0, which means the declared minimum version requirement of "openai-agents >= 0.2.0" is likely too permissive and should be updated to reflect when this API actually became available.

Confirm the exact version that introduced set_trace_processors (likely > 0.2.0) and update _instruments accordingly. Additionally, add test coverage for both clear=False (append mode) and clear=True (replace mode) to prevent regressions.

@duanyutong duanyutong force-pushed the feat-openai-agents-clear branch from a77793c to 8a0fece Compare December 4, 2025 16:59
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