fix(tracing): do not cache a missing OPENAI_API_KEY - #5017
Open
hyeonsang010716 wants to merge 1 commit into
Open
hyeonsang010716 wants to merge 1 commit into
hyeonsang010716 wants to merge 1 commit into
Conversation
`BackendSpanExporter.api_key` was a `cached_property` over
`self._api_key or os.environ.get("OPENAI_API_KEY")`. Resolving it before
the variable existed cached `None`, so a key set afterwards, for example
by `load_dotenv()`, never took effect and every export kept logging
"OPENAI_API_KEY is not set, skipping trace export".
`api_key` is now a property that reads the explicit key once and falls
back to the environment only when it is missing. A key found in the
environment is kept, so a later change to the variable does not reroute
exports. A lookup that finds nothing stores nothing, so it cannot
discard a key that `set_api_key()` sets on another thread meanwhile.
A setter keeps `exporter.api_key = "..."` working, since public code
relies on assigning the attribute.
Contributor
|
Reviewed the complete diff against I found no actionable correctness issue. The property preserves the explicit-key precedence and existing environment fallback, avoids caching a missing key, and the setter keeps direct Focused verification on the PR snapshot passed: |
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.
Summary
BackendSpanExporter.api_keywas acached_propertyoverself._api_key or os.environ.get("OPENAI_API_KEY"). If the exporter resolved it before the variableexisted, it cached
Nonefor the life of the process. SettingOPENAI_API_KEYafterwards never tookeffect, and every export kept logging "OPENAI_API_KEY is not set, skipping trace export" even though the
key was now set.
This happens whenever a trace is exported before the key is loaded, for example a notebook run that
fails for lack of a key and is retried after setting it, or an app that calls
load_dotenv()or fetchessecrets after its first traced run. The default model client recovers in the same situation, because a
failed client creation is not cached, so model calls start working while tracing stays off. That
contradicts
docs/config.md, which says tracing uses the same key as model requests.The cache was added in #289 so the key is read when the exporter needs it rather than at import time,
and #1339 later made
set_api_key()clear it. This change keeps both intents without caching a missingkey:
api_keyis now a property that reads the explicit key once and consults the environment only whenit is missing.
not reroute exports that did not call
set_api_key().writing the missing result back could otherwise discard a key that
set_api_key()sets meanwhile, onevery keyless export.
exporter.api_key = "..."working. It was never documented, but public code relies onit: the maokangkun/SigmaFlow setup page tells users to run
default_exporter().api_key = "...". Thesetter writes the key directly, as the old assignment did, and accepts
str | None.Behavior changes worth knowing about, none of which I found documented, tested, or used in public code:
del exporter.api_keyand instance-levelmock.patch.object(exporter, "api_key", ...)now raiseAttributeError. Class-level patching still works.Noneor""now falls back to the environment, matchingset_api_key(""), instead ofpinning a missing key.
set_tracing_disabled(True)remains the way to stop exports.self.api_keybefore callingsuper().__init__()loses that value, andpyright reports a
cached_propertyoverride ofapi_keyas incompatible.organizationandprojectare unchanged. They are read only after a key is available, so thescenario above does not leave them stale. The race in which the one lookup that finds
OPENAI_API_KEYoverlaps a concurrentset_api_key()also behaves exactly as before.Test plan
tests/tracing/test_processor_api_key.py, each exercisingexporter.export()or the public attribute:test_exporter_uses_env_api_key_set_after_an_export_without_oneexports with no key, sets thevariable, and asserts the next export sends it. It fails on
main.test_exporter_keeps_env_api_key_once_resolvedchanges the variable after a key is resolved andasserts the original key is still sent. It passes on
mainand fails on a property that does notcache at all.
test_exporter_uses_an_assigned_api_keypasses onmainand fails without the setter.test_keyless_lookup_does_not_discard_a_key_set_while_it_runscallsset_api_key()from inside theenvironment lookup and asserts the key survives. It fails on
mainand on a getter that writes backwhatever the lookup returned.
tests/tracing/test_set_api_key_fix.pyreset its explicit key by assigning the private_api_keyanddeleting the cache entry, which no longer exists. It now calls
set_api_key(""), which covers the samefallback through the public API and passes on
mainas well.ruff format --check,ruff check,check_optional_truthiness.py src/agents,mypy src(312 files)and
pyrightare clean.are the pre-existing symlink-privilege ones (Sandbox tar extraction and 12 sandbox tests fail on Windows without symlink privilege #4852); a pristine tree on this host produces the same 12.
makeis not installed here, so theMakefiletargets were run directly in the script's order:format, lint, typecheck, then the parallel and serial suites.
Issue number
None.
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR