-
Notifications
You must be signed in to change notification settings - Fork 844
feat(openai-agents): add clear flag to support two instrumentation modes #3489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds a constructor flag Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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. Comment |
There was a problem hiding this 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
45lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft 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 nameclearis 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()). Whilereplace_existingmight 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 nameclearmight be ambiguous - does it clear traces, processors, or something else? The suggested namereplace_existingwould be more self-documenting and reduce the need to read the docstring to understand the parameter's purpose. Whilereplace_existingmight be slightly more explicit, the docstring already provides clear documentation, andclearis a common programming idiom. The parameter is keyword-only (enforced by the*in the signature), so users will seeclear=Trueat 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 by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this 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 Exceptionon line 57 could silently suppress errors beyond import failures (e.g., runtime errors inset_trace_processorsoradd_trace_processor). Consider catchingImportErrorspecifically 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.
📒 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
clearparameter with a default ofFalseensures 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
clearparameter, 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=Trueor appending whenclear=False. This maintains backward compatibility.However, ensure comprehensive tests are added to cover both branches, especially the
clear=Truepath that replaces existing processors.
47-47: Verifyset_trace_processorsavailability in openai-agents >= 0.2.0.The
set_trace_processorsAPI is documented in the OpenAI Agents SDK tracing module, but the exact version when this API was introduced is not publicly documented. Confirm whetherset_trace_processorsis 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.
nirga
left a comment
There was a problem hiding this 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
346cace to
a77793c
Compare
Sure, done. |
There was a problem hiding this 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: Constructorclearflag wiring is correct; consider future‑proofing base initThe
clearflag is cleanly added as a keyword‑only argument and stored onself._clear, matching the later usage in_instrument. SinceBaseInstrumentoris currently an ABC without__init__, not callingsuper().__init__()is safe, but you might still consider adding asuper().__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.
📒 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 goodAdding
.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 forset_trace_processorsAPIThe concern about version compatibility is valid. Web search reveals that
set_trace_processorsis documented in the official Agents SDK tracing API and is compatible with the signatureset_trace_processors([processor]). However, the 0.2.0 release notes make no mention of trace processors functionality — only a typing change forAgentBase. This suggestsset_trace_processorsmay 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_instrumentsaccordingly. Additionally, add test coverage for bothclear=False(append mode) andclear=True(replace mode) to prevent regressions.
a77793c to
8a0fece
Compare
Changes
feat(instrumentation): ...orfix(instrumentation): ....Important
Add
clearflag toOpenAIAgentsInstrumentor._instrumentto support replacing existing trace processors.clearflag to_instrumentmethod inOpenAIAgentsInstrumentorto support two instrumentation modes.clearisTrue, existing trace processors are replaced with the new one usingset_trace_processors.clearisFalse, the new processor is added to existing ones usingadd_trace_processor._instrumentto describe theclearflag and its effect.This description was created by
for 346cace. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.