feat(openai): instrument Audio API (speech, transcriptions, translations) - #232

Merged
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation
Apr 9, 2026
Merged

feat(openai): instrument Audio API (speech, transcriptions, translations)#232
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation

Conversation

@AbhiPrasad

Copy link
Copy Markdown
Member

Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).

Closes#174

…ons)
Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).
Closes#174
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_AudioFileWrapper._parse_params was popping 'file' from the original
kwargs before prettify_params made a copy, causing the actual API call
to lose the required file argument.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace 3 copy-pasted wrapper callbacks with _make_base_wrapper_callback factory
- Extract shared text extraction logic into _AudioFileWrapper._extract_text
- Use context managers for file handles in audio tests
- Remove unnecessary comment in SpeechWrapper.process_output
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@starfolkai

starfolkaiBot commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Drop into this review session: sfk devbox session 68

PR #232 — Audio API Instrumentation: Code Review

Verdict: Approve with minor suggestions

All 438 tests pass (nox -s "test_openai(latest)"), including the 6 new audio tests. The implementation is clean and consistent with the repo's established patterns.


What's good

  • Follows existing conventions exactly: VCR cassettes, sync/async patcher pairs, registration in both integration.py (for setup()) and _WRAP_TARGETS (for wrap_openai()). The test_wrap_openai_and_setup_use_same_wrappers enforcement is satisfied.
  • _AudioFileWrapper._parse_params correctly prevents logging binary audio data by popping file from the prettified copy of params, so the original kwargs passed to the API call are unaffected.
  • _make_base_wrapper_callback is a reasonable factory that reduces the repetitive sync/async branching seen in the inline wrappers for chat/embeddings/moderation.
  • SpeechWrapper logging {"type": "audio"} for output is the right call — streaming binary audio into a span would be impractical.
  • TranscriptionWrapper correctly handles the {"type":"duration","seconds":1} usage shape from Whisper — _parse_metrics_from_usage returns an empty dict gracefully when token fields are absent.

Issues

1. _extract_text fallback can silently produce wrong output (minor bug)

tracing.py, _AudioFileWrapper._extract_text:

@staticmethoddef_extract_text(response: Any) ->str|None:
ifisinstance(response, dict):
returnresponse.get("text")
ifisinstance(response, str):
returnresponsereturnstr(response) # ← problem here

When _try_to_dict can't convert the response to a dict (e.g., an object type it doesn't recognize), and the response isn't a plain string, the fallback str(response) will return something like Transcription(text='hello world', logprobs=None, usage=None) — Pydantic's repr — instead of the actual transcribed text. This won't crash, but the span output would be garbage.

A safer fallback:

returngetattr(response, "text", None)

This handles OpenAI Pydantic model types (Transcription, TranscriptionVerbose, Translation, etc.) that have a .text attribute directly, without relying on dict conversion.


2. Weak test assertions for transcription/translation

assertspan["output"] isnotNone

For transcription and translation tests, the output is never checked beyond being non-None. Given that the cassette response is {"text":"you","usage":{"type":"duration","seconds":1}}, you can assert the actual value:

assertspan["output"] =="you"

This would have caught the _extract_text fallback issue at test-recording time if anything went wrong.


Nits / observations (non-blocking)

_make_base_wrapper_callback is placed ~600 lines before BaseWrapper — the string annotation wrapper_cls: type["BaseWrapper"] is correct, but it's a bit of a readability surprise. The existing per-method wrapper functions (_embedding_create_wrapper, etc.) aren't converted to use the factory, which leaves a small inconsistency. Neither is a blocker, but if the factory pattern proves useful it might be worth a follow-up to unify the three inline wrappers.

No coverage for response_format="text" — when the caller passes response_format="text", OpenAI returns a plain str rather than a JSON object. The isinstance(response, str) branch in _extract_text handles this, but there's no test for it. A cassette for this path would be a good addition.


Summary

The core logic is correct and all existing + new tests pass. I'd address the _extract_text fallback (issue 1) and strengthen the transcription/translation output assertions (issue 2) before merge. Everything else is minor.

- Replace str(response) fallback with getattr(response, 'text', None) so
Pydantic model responses (Transcription, Translation, etc.) are handled
correctly instead of logging repr strings
- Tighten transcription/translation span output assertions from
`is not None` to the actual fixture value "you"
- Add test_openai_audio_transcription_text_format covering the
response_format="text" path (plain-string response from Whisper)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Whisper's response_format="text" response ends with \n. Strip it so the
logged span output is consistent with the json format (which returns the
text field without trailing whitespace).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) merged commit 45e4de1 into mainApr 9, 2026
59 checks passed
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) deleted the feat/audio-api-instrumentation branch April 9, 2026 18:11
Abhijeet Prasad (AbhiPrasad) added a commit that referenced this pull request Apr 10, 2026
Sign up for freeto 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.

OpenAI: Audio API (client.audio.speech, transcriptions, translations) not instrumented

2 participants

@AbhiPrasad@Qard
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

feat(openai): instrument Audio API (speech, transcriptions, translations) - #232

Merged
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation
Apr 9, 2026
Merged

feat(openai): instrument Audio API (speech, transcriptions, translations)#232
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation

Conversation

@AbhiPrasad

Copy link
Copy Markdown
Member

Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).

Closes#174

…ons)
Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).
Closes#174
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_AudioFileWrapper._parse_params was popping 'file' from the original
kwargs before prettify_params made a copy, causing the actual API call
to lose the required file argument.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace 3 copy-pasted wrapper callbacks with _make_base_wrapper_callback factory
- Extract shared text extraction logic into _AudioFileWrapper._extract_text
- Use context managers for file handles in audio tests
- Remove unnecessary comment in SpeechWrapper.process_output
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@starfolkai

starfolkaiBot commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Drop into this review session: sfk devbox session 68

PR #232 — Audio API Instrumentation: Code Review

Verdict: Approve with minor suggestions

All 438 tests pass (nox -s "test_openai(latest)"), including the 6 new audio tests. The implementation is clean and consistent with the repo's established patterns.


What's good

  • Follows existing conventions exactly: VCR cassettes, sync/async patcher pairs, registration in both integration.py (for setup()) and _WRAP_TARGETS (for wrap_openai()). The test_wrap_openai_and_setup_use_same_wrappers enforcement is satisfied.
  • _AudioFileWrapper._parse_params correctly prevents logging binary audio data by popping file from the prettified copy of params, so the original kwargs passed to the API call are unaffected.
  • _make_base_wrapper_callback is a reasonable factory that reduces the repetitive sync/async branching seen in the inline wrappers for chat/embeddings/moderation.
  • SpeechWrapper logging {"type": "audio"} for output is the right call — streaming binary audio into a span would be impractical.
  • TranscriptionWrapper correctly handles the {"type":"duration","seconds":1} usage shape from Whisper — _parse_metrics_from_usage returns an empty dict gracefully when token fields are absent.

Issues

1. _extract_text fallback can silently produce wrong output (minor bug)

tracing.py, _AudioFileWrapper._extract_text:

@staticmethoddef_extract_text(response: Any) ->str|None:
ifisinstance(response, dict):
returnresponse.get("text")
ifisinstance(response, str):
returnresponsereturnstr(response) # ← problem here

When _try_to_dict can't convert the response to a dict (e.g., an object type it doesn't recognize), and the response isn't a plain string, the fallback str(response) will return something like Transcription(text='hello world', logprobs=None, usage=None) — Pydantic's repr — instead of the actual transcribed text. This won't crash, but the span output would be garbage.

A safer fallback:

returngetattr(response, "text", None)

This handles OpenAI Pydantic model types (Transcription, TranscriptionVerbose, Translation, etc.) that have a .text attribute directly, without relying on dict conversion.


2. Weak test assertions for transcription/translation

assertspan["output"] isnotNone

For transcription and translation tests, the output is never checked beyond being non-None. Given that the cassette response is {"text":"you","usage":{"type":"duration","seconds":1}}, you can assert the actual value:

assertspan["output"] =="you"

This would have caught the _extract_text fallback issue at test-recording time if anything went wrong.


Nits / observations (non-blocking)

_make_base_wrapper_callback is placed ~600 lines before BaseWrapper — the string annotation wrapper_cls: type["BaseWrapper"] is correct, but it's a bit of a readability surprise. The existing per-method wrapper functions (_embedding_create_wrapper, etc.) aren't converted to use the factory, which leaves a small inconsistency. Neither is a blocker, but if the factory pattern proves useful it might be worth a follow-up to unify the three inline wrappers.

No coverage for response_format="text" — when the caller passes response_format="text", OpenAI returns a plain str rather than a JSON object. The isinstance(response, str) branch in _extract_text handles this, but there's no test for it. A cassette for this path would be a good addition.


Summary

The core logic is correct and all existing + new tests pass. I'd address the _extract_text fallback (issue 1) and strengthen the transcription/translation output assertions (issue 2) before merge. Everything else is minor.

- Replace str(response) fallback with getattr(response, 'text', None) so
Pydantic model responses (Transcription, Translation, etc.) are handled
correctly instead of logging repr strings
- Tighten transcription/translation span output assertions from
`is not None` to the actual fixture value "you"
- Add test_openai_audio_transcription_text_format covering the
response_format="text" path (plain-string response from Whisper)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Whisper's response_format="text" response ends with \n. Strip it so the
logged span output is consistent with the json format (which returns the
text field without trailing whitespace).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) merged commit 45e4de1 into mainApr 9, 2026
59 checks passed
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) deleted the feat/audio-api-instrumentation branch April 9, 2026 18:11
Abhijeet Prasad (AbhiPrasad) added a commit that referenced this pull request Apr 10, 2026
Sign up for freeto 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.

OpenAI: Audio API (client.audio.speech, transcriptions, translations) not instrumented

2 participants

@AbhiPrasad@Qard
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(openai): instrument Audio API (speech, transcriptions, translations) - #232

Merged
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation
Apr 9, 2026
Merged

feat(openai): instrument Audio API (speech, transcriptions, translations)#232
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation

Conversation

@AbhiPrasad

Copy link
Copy Markdown
Member

Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).

Closes#174

…ons)
Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).
Closes#174
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_AudioFileWrapper._parse_params was popping 'file' from the original
kwargs before prettify_params made a copy, causing the actual API call
to lose the required file argument.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace 3 copy-pasted wrapper callbacks with _make_base_wrapper_callback factory
- Extract shared text extraction logic into _AudioFileWrapper._extract_text
- Use context managers for file handles in audio tests
- Remove unnecessary comment in SpeechWrapper.process_output
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@starfolkai

starfolkaiBot commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Drop into this review session: sfk devbox session 68

PR #232 — Audio API Instrumentation: Code Review

Verdict: Approve with minor suggestions

All 438 tests pass (nox -s "test_openai(latest)"), including the 6 new audio tests. The implementation is clean and consistent with the repo's established patterns.


What's good

  • Follows existing conventions exactly: VCR cassettes, sync/async patcher pairs, registration in both integration.py (for setup()) and _WRAP_TARGETS (for wrap_openai()). The test_wrap_openai_and_setup_use_same_wrappers enforcement is satisfied.
  • _AudioFileWrapper._parse_params correctly prevents logging binary audio data by popping file from the prettified copy of params, so the original kwargs passed to the API call are unaffected.
  • _make_base_wrapper_callback is a reasonable factory that reduces the repetitive sync/async branching seen in the inline wrappers for chat/embeddings/moderation.
  • SpeechWrapper logging {"type": "audio"} for output is the right call — streaming binary audio into a span would be impractical.
  • TranscriptionWrapper correctly handles the {"type":"duration","seconds":1} usage shape from Whisper — _parse_metrics_from_usage returns an empty dict gracefully when token fields are absent.

Issues

1. _extract_text fallback can silently produce wrong output (minor bug)

tracing.py, _AudioFileWrapper._extract_text:

@staticmethoddef_extract_text(response: Any) ->str|None:
ifisinstance(response, dict):
returnresponse.get("text")
ifisinstance(response, str):
returnresponsereturnstr(response) # ← problem here

When _try_to_dict can't convert the response to a dict (e.g., an object type it doesn't recognize), and the response isn't a plain string, the fallback str(response) will return something like Transcription(text='hello world', logprobs=None, usage=None) — Pydantic's repr — instead of the actual transcribed text. This won't crash, but the span output would be garbage.

A safer fallback:

returngetattr(response, "text", None)

This handles OpenAI Pydantic model types (Transcription, TranscriptionVerbose, Translation, etc.) that have a .text attribute directly, without relying on dict conversion.


2. Weak test assertions for transcription/translation

assertspan["output"] isnotNone

For transcription and translation tests, the output is never checked beyond being non-None. Given that the cassette response is {"text":"you","usage":{"type":"duration","seconds":1}}, you can assert the actual value:

assertspan["output"] =="you"

This would have caught the _extract_text fallback issue at test-recording time if anything went wrong.


Nits / observations (non-blocking)

_make_base_wrapper_callback is placed ~600 lines before BaseWrapper — the string annotation wrapper_cls: type["BaseWrapper"] is correct, but it's a bit of a readability surprise. The existing per-method wrapper functions (_embedding_create_wrapper, etc.) aren't converted to use the factory, which leaves a small inconsistency. Neither is a blocker, but if the factory pattern proves useful it might be worth a follow-up to unify the three inline wrappers.

No coverage for response_format="text" — when the caller passes response_format="text", OpenAI returns a plain str rather than a JSON object. The isinstance(response, str) branch in _extract_text handles this, but there's no test for it. A cassette for this path would be a good addition.


Summary

The core logic is correct and all existing + new tests pass. I'd address the _extract_text fallback (issue 1) and strengthen the transcription/translation output assertions (issue 2) before merge. Everything else is minor.

- Replace str(response) fallback with getattr(response, 'text', None) so
Pydantic model responses (Transcription, Translation, etc.) are handled
correctly instead of logging repr strings
- Tighten transcription/translation span output assertions from
`is not None` to the actual fixture value "you"
- Add test_openai_audio_transcription_text_format covering the
response_format="text" path (plain-string response from Whisper)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Whisper's response_format="text" response ends with \n. Strip it so the
logged span output is consistent with the json format (which returns the
text field without trailing whitespace).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) merged commit 45e4de1 into mainApr 9, 2026
59 checks passed
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) deleted the feat/audio-api-instrumentation branch April 9, 2026 18:11
Abhijeet Prasad (AbhiPrasad) added a commit that referenced this pull request Apr 10, 2026
Sign up for freeto 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.

OpenAI: Audio API (client.audio.speech, transcriptions, translations) not instrumented

2 participants

@AbhiPrasad@Qard
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(openai): instrument Audio API (speech, transcriptions, translations) - #232

Merged
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation
Apr 9, 2026
Merged

feat(openai): instrument Audio API (speech, transcriptions, translations)#232
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation

Conversation

@AbhiPrasad

Copy link
Copy Markdown
Member

Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).

Closes#174

…ons)
Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).
Closes#174
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_AudioFileWrapper._parse_params was popping 'file' from the original
kwargs before prettify_params made a copy, causing the actual API call
to lose the required file argument.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace 3 copy-pasted wrapper callbacks with _make_base_wrapper_callback factory
- Extract shared text extraction logic into _AudioFileWrapper._extract_text
- Use context managers for file handles in audio tests
- Remove unnecessary comment in SpeechWrapper.process_output
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@starfolkai

starfolkaiBot commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Drop into this review session: sfk devbox session 68

PR #232 — Audio API Instrumentation: Code Review

Verdict: Approve with minor suggestions

All 438 tests pass (nox -s "test_openai(latest)"), including the 6 new audio tests. The implementation is clean and consistent with the repo's established patterns.


What's good

  • Follows existing conventions exactly: VCR cassettes, sync/async patcher pairs, registration in both integration.py (for setup()) and _WRAP_TARGETS (for wrap_openai()). The test_wrap_openai_and_setup_use_same_wrappers enforcement is satisfied.
  • _AudioFileWrapper._parse_params correctly prevents logging binary audio data by popping file from the prettified copy of params, so the original kwargs passed to the API call are unaffected.
  • _make_base_wrapper_callback is a reasonable factory that reduces the repetitive sync/async branching seen in the inline wrappers for chat/embeddings/moderation.
  • SpeechWrapper logging {"type": "audio"} for output is the right call — streaming binary audio into a span would be impractical.
  • TranscriptionWrapper correctly handles the {"type":"duration","seconds":1} usage shape from Whisper — _parse_metrics_from_usage returns an empty dict gracefully when token fields are absent.

Issues

1. _extract_text fallback can silently produce wrong output (minor bug)

tracing.py, _AudioFileWrapper._extract_text:

@staticmethoddef_extract_text(response: Any) ->str|None:
ifisinstance(response, dict):
returnresponse.get("text")
ifisinstance(response, str):
returnresponsereturnstr(response) # ← problem here

When _try_to_dict can't convert the response to a dict (e.g., an object type it doesn't recognize), and the response isn't a plain string, the fallback str(response) will return something like Transcription(text='hello world', logprobs=None, usage=None) — Pydantic's repr — instead of the actual transcribed text. This won't crash, but the span output would be garbage.

A safer fallback:

returngetattr(response, "text", None)

This handles OpenAI Pydantic model types (Transcription, TranscriptionVerbose, Translation, etc.) that have a .text attribute directly, without relying on dict conversion.


2. Weak test assertions for transcription/translation

assertspan["output"] isnotNone

For transcription and translation tests, the output is never checked beyond being non-None. Given that the cassette response is {"text":"you","usage":{"type":"duration","seconds":1}}, you can assert the actual value:

assertspan["output"] =="you"

This would have caught the _extract_text fallback issue at test-recording time if anything went wrong.


Nits / observations (non-blocking)

_make_base_wrapper_callback is placed ~600 lines before BaseWrapper — the string annotation wrapper_cls: type["BaseWrapper"] is correct, but it's a bit of a readability surprise. The existing per-method wrapper functions (_embedding_create_wrapper, etc.) aren't converted to use the factory, which leaves a small inconsistency. Neither is a blocker, but if the factory pattern proves useful it might be worth a follow-up to unify the three inline wrappers.

No coverage for response_format="text" — when the caller passes response_format="text", OpenAI returns a plain str rather than a JSON object. The isinstance(response, str) branch in _extract_text handles this, but there's no test for it. A cassette for this path would be a good addition.


Summary

The core logic is correct and all existing + new tests pass. I'd address the _extract_text fallback (issue 1) and strengthen the transcription/translation output assertions (issue 2) before merge. Everything else is minor.

- Replace str(response) fallback with getattr(response, 'text', None) so
Pydantic model responses (Transcription, Translation, etc.) are handled
correctly instead of logging repr strings
- Tighten transcription/translation span output assertions from
`is not None` to the actual fixture value "you"
- Add test_openai_audio_transcription_text_format covering the
response_format="text" path (plain-string response from Whisper)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Whisper's response_format="text" response ends with \n. Strip it so the
logged span output is consistent with the json format (which returns the
text field without trailing whitespace).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) merged commit 45e4de1 into mainApr 9, 2026
59 checks passed
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) deleted the feat/audio-api-instrumentation branch April 9, 2026 18:11
Abhijeet Prasad (AbhiPrasad) added a commit that referenced this pull request Apr 10, 2026
Sign up for freeto 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.

OpenAI: Audio API (client.audio.speech, transcriptions, translations) not instrumented

2 participants

@AbhiPrasad@Qard
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

feat(openai): instrument Audio API (speech, transcriptions, translations) - #232

Merged
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation
Apr 9, 2026
Merged

feat(openai): instrument Audio API (speech, transcriptions, translations)#232
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation

Conversation

@AbhiPrasad

Copy link
Copy Markdown
Member

Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).

Closes#174

…ons)
Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).
Closes#174
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_AudioFileWrapper._parse_params was popping 'file' from the original
kwargs before prettify_params made a copy, causing the actual API call
to lose the required file argument.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace 3 copy-pasted wrapper callbacks with _make_base_wrapper_callback factory
- Extract shared text extraction logic into _AudioFileWrapper._extract_text
- Use context managers for file handles in audio tests
- Remove unnecessary comment in SpeechWrapper.process_output
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@starfolkai

starfolkaiBot commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Drop into this review session: sfk devbox session 68

PR #232 — Audio API Instrumentation: Code Review

Verdict: Approve with minor suggestions

All 438 tests pass (nox -s "test_openai(latest)"), including the 6 new audio tests. The implementation is clean and consistent with the repo's established patterns.


What's good

  • Follows existing conventions exactly: VCR cassettes, sync/async patcher pairs, registration in both integration.py (for setup()) and _WRAP_TARGETS (for wrap_openai()). The test_wrap_openai_and_setup_use_same_wrappers enforcement is satisfied.
  • _AudioFileWrapper._parse_params correctly prevents logging binary audio data by popping file from the prettified copy of params, so the original kwargs passed to the API call are unaffected.
  • _make_base_wrapper_callback is a reasonable factory that reduces the repetitive sync/async branching seen in the inline wrappers for chat/embeddings/moderation.
  • SpeechWrapper logging {"type": "audio"} for output is the right call — streaming binary audio into a span would be impractical.
  • TranscriptionWrapper correctly handles the {"type":"duration","seconds":1} usage shape from Whisper — _parse_metrics_from_usage returns an empty dict gracefully when token fields are absent.

Issues

1. _extract_text fallback can silently produce wrong output (minor bug)

tracing.py, _AudioFileWrapper._extract_text:

@staticmethoddef_extract_text(response: Any) ->str|None:
ifisinstance(response, dict):
returnresponse.get("text")
ifisinstance(response, str):
returnresponsereturnstr(response) # ← problem here

When _try_to_dict can't convert the response to a dict (e.g., an object type it doesn't recognize), and the response isn't a plain string, the fallback str(response) will return something like Transcription(text='hello world', logprobs=None, usage=None) — Pydantic's repr — instead of the actual transcribed text. This won't crash, but the span output would be garbage.

A safer fallback:

returngetattr(response, "text", None)

This handles OpenAI Pydantic model types (Transcription, TranscriptionVerbose, Translation, etc.) that have a .text attribute directly, without relying on dict conversion.


2. Weak test assertions for transcription/translation

assertspan["output"] isnotNone

For transcription and translation tests, the output is never checked beyond being non-None. Given that the cassette response is {"text":"you","usage":{"type":"duration","seconds":1}}, you can assert the actual value:

assertspan["output"] =="you"

This would have caught the _extract_text fallback issue at test-recording time if anything went wrong.


Nits / observations (non-blocking)

_make_base_wrapper_callback is placed ~600 lines before BaseWrapper — the string annotation wrapper_cls: type["BaseWrapper"] is correct, but it's a bit of a readability surprise. The existing per-method wrapper functions (_embedding_create_wrapper, etc.) aren't converted to use the factory, which leaves a small inconsistency. Neither is a blocker, but if the factory pattern proves useful it might be worth a follow-up to unify the three inline wrappers.

No coverage for response_format="text" — when the caller passes response_format="text", OpenAI returns a plain str rather than a JSON object. The isinstance(response, str) branch in _extract_text handles this, but there's no test for it. A cassette for this path would be a good addition.


Summary

The core logic is correct and all existing + new tests pass. I'd address the _extract_text fallback (issue 1) and strengthen the transcription/translation output assertions (issue 2) before merge. Everything else is minor.

- Replace str(response) fallback with getattr(response, 'text', None) so
Pydantic model responses (Transcription, Translation, etc.) are handled
correctly instead of logging repr strings
- Tighten transcription/translation span output assertions from
`is not None` to the actual fixture value "you"
- Add test_openai_audio_transcription_text_format covering the
response_format="text" path (plain-string response from Whisper)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Whisper's response_format="text" response ends with \n. Strip it so the
logged span output is consistent with the json format (which returns the
text field without trailing whitespace).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) merged commit 45e4de1 into mainApr 9, 2026
59 checks passed
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) deleted the feat/audio-api-instrumentation branch April 9, 2026 18:11
Abhijeet Prasad (AbhiPrasad) added a commit that referenced this pull request Apr 10, 2026
Sign up for freeto 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.

OpenAI: Audio API (client.audio.speech, transcriptions, translations) not instrumented

2 participants

@AbhiPrasad@Qard
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(openai): instrument Audio API (speech, transcriptions, translations) - #232

Merged
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation
Apr 9, 2026
Merged

feat(openai): instrument Audio API (speech, transcriptions, translations)#232
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation

Conversation

@AbhiPrasad

Copy link
Copy Markdown
Member

Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).

Closes#174

…ons)
Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).
Closes#174
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_AudioFileWrapper._parse_params was popping 'file' from the original
kwargs before prettify_params made a copy, causing the actual API call
to lose the required file argument.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace 3 copy-pasted wrapper callbacks with _make_base_wrapper_callback factory
- Extract shared text extraction logic into _AudioFileWrapper._extract_text
- Use context managers for file handles in audio tests
- Remove unnecessary comment in SpeechWrapper.process_output
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@starfolkai

starfolkaiBot commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Drop into this review session: sfk devbox session 68

PR #232 — Audio API Instrumentation: Code Review

Verdict: Approve with minor suggestions

All 438 tests pass (nox -s "test_openai(latest)"), including the 6 new audio tests. The implementation is clean and consistent with the repo's established patterns.


What's good

  • Follows existing conventions exactly: VCR cassettes, sync/async patcher pairs, registration in both integration.py (for setup()) and _WRAP_TARGETS (for wrap_openai()). The test_wrap_openai_and_setup_use_same_wrappers enforcement is satisfied.
  • _AudioFileWrapper._parse_params correctly prevents logging binary audio data by popping file from the prettified copy of params, so the original kwargs passed to the API call are unaffected.
  • _make_base_wrapper_callback is a reasonable factory that reduces the repetitive sync/async branching seen in the inline wrappers for chat/embeddings/moderation.
  • SpeechWrapper logging {"type": "audio"} for output is the right call — streaming binary audio into a span would be impractical.
  • TranscriptionWrapper correctly handles the {"type":"duration","seconds":1} usage shape from Whisper — _parse_metrics_from_usage returns an empty dict gracefully when token fields are absent.

Issues

1. _extract_text fallback can silently produce wrong output (minor bug)

tracing.py, _AudioFileWrapper._extract_text:

@staticmethoddef_extract_text(response: Any) ->str|None:
ifisinstance(response, dict):
returnresponse.get("text")
ifisinstance(response, str):
returnresponsereturnstr(response) # ← problem here

When _try_to_dict can't convert the response to a dict (e.g., an object type it doesn't recognize), and the response isn't a plain string, the fallback str(response) will return something like Transcription(text='hello world', logprobs=None, usage=None) — Pydantic's repr — instead of the actual transcribed text. This won't crash, but the span output would be garbage.

A safer fallback:

returngetattr(response, "text", None)

This handles OpenAI Pydantic model types (Transcription, TranscriptionVerbose, Translation, etc.) that have a .text attribute directly, without relying on dict conversion.


2. Weak test assertions for transcription/translation

assertspan["output"] isnotNone

For transcription and translation tests, the output is never checked beyond being non-None. Given that the cassette response is {"text":"you","usage":{"type":"duration","seconds":1}}, you can assert the actual value:

assertspan["output"] =="you"

This would have caught the _extract_text fallback issue at test-recording time if anything went wrong.


Nits / observations (non-blocking)

_make_base_wrapper_callback is placed ~600 lines before BaseWrapper — the string annotation wrapper_cls: type["BaseWrapper"] is correct, but it's a bit of a readability surprise. The existing per-method wrapper functions (_embedding_create_wrapper, etc.) aren't converted to use the factory, which leaves a small inconsistency. Neither is a blocker, but if the factory pattern proves useful it might be worth a follow-up to unify the three inline wrappers.

No coverage for response_format="text" — when the caller passes response_format="text", OpenAI returns a plain str rather than a JSON object. The isinstance(response, str) branch in _extract_text handles this, but there's no test for it. A cassette for this path would be a good addition.


Summary

The core logic is correct and all existing + new tests pass. I'd address the _extract_text fallback (issue 1) and strengthen the transcription/translation output assertions (issue 2) before merge. Everything else is minor.

- Replace str(response) fallback with getattr(response, 'text', None) so
Pydantic model responses (Transcription, Translation, etc.) are handled
correctly instead of logging repr strings
- Tighten transcription/translation span output assertions from
`is not None` to the actual fixture value "you"
- Add test_openai_audio_transcription_text_format covering the
response_format="text" path (plain-string response from Whisper)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Whisper's response_format="text" response ends with \n. Strip it so the
logged span output is consistent with the json format (which returns the
text field without trailing whitespace).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) merged commit 45e4de1 into mainApr 9, 2026
59 checks passed
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) deleted the feat/audio-api-instrumentation branch April 9, 2026 18:11
Abhijeet Prasad (AbhiPrasad) added a commit that referenced this pull request Apr 10, 2026
Sign up for freeto 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.

OpenAI: Audio API (client.audio.speech, transcriptions, translations) not instrumented

2 participants

@AbhiPrasad@Qard
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(openai): instrument Audio API (speech, transcriptions, translations) - #232

Merged
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation
Apr 9, 2026
Merged

feat(openai): instrument Audio API (speech, transcriptions, translations)#232
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation

Conversation

@AbhiPrasad

Copy link
Copy Markdown
Member

Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).

Closes#174

…ons)
Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).
Closes#174
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_AudioFileWrapper._parse_params was popping 'file' from the original
kwargs before prettify_params made a copy, causing the actual API call
to lose the required file argument.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace 3 copy-pasted wrapper callbacks with _make_base_wrapper_callback factory
- Extract shared text extraction logic into _AudioFileWrapper._extract_text
- Use context managers for file handles in audio tests
- Remove unnecessary comment in SpeechWrapper.process_output
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@starfolkai

starfolkaiBot commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Drop into this review session: sfk devbox session 68

PR #232 — Audio API Instrumentation: Code Review

Verdict: Approve with minor suggestions

All 438 tests pass (nox -s "test_openai(latest)"), including the 6 new audio tests. The implementation is clean and consistent with the repo's established patterns.


What's good

  • Follows existing conventions exactly: VCR cassettes, sync/async patcher pairs, registration in both integration.py (for setup()) and _WRAP_TARGETS (for wrap_openai()). The test_wrap_openai_and_setup_use_same_wrappers enforcement is satisfied.
  • _AudioFileWrapper._parse_params correctly prevents logging binary audio data by popping file from the prettified copy of params, so the original kwargs passed to the API call are unaffected.
  • _make_base_wrapper_callback is a reasonable factory that reduces the repetitive sync/async branching seen in the inline wrappers for chat/embeddings/moderation.
  • SpeechWrapper logging {"type": "audio"} for output is the right call — streaming binary audio into a span would be impractical.
  • TranscriptionWrapper correctly handles the {"type":"duration","seconds":1} usage shape from Whisper — _parse_metrics_from_usage returns an empty dict gracefully when token fields are absent.

Issues

1. _extract_text fallback can silently produce wrong output (minor bug)

tracing.py, _AudioFileWrapper._extract_text:

@staticmethoddef_extract_text(response: Any) ->str|None:
ifisinstance(response, dict):
returnresponse.get("text")
ifisinstance(response, str):
returnresponsereturnstr(response) # ← problem here

When _try_to_dict can't convert the response to a dict (e.g., an object type it doesn't recognize), and the response isn't a plain string, the fallback str(response) will return something like Transcription(text='hello world', logprobs=None, usage=None) — Pydantic's repr — instead of the actual transcribed text. This won't crash, but the span output would be garbage.

A safer fallback:

returngetattr(response, "text", None)

This handles OpenAI Pydantic model types (Transcription, TranscriptionVerbose, Translation, etc.) that have a .text attribute directly, without relying on dict conversion.


2. Weak test assertions for transcription/translation

assertspan["output"] isnotNone

For transcription and translation tests, the output is never checked beyond being non-None. Given that the cassette response is {"text":"you","usage":{"type":"duration","seconds":1}}, you can assert the actual value:

assertspan["output"] =="you"

This would have caught the _extract_text fallback issue at test-recording time if anything went wrong.


Nits / observations (non-blocking)

_make_base_wrapper_callback is placed ~600 lines before BaseWrapper — the string annotation wrapper_cls: type["BaseWrapper"] is correct, but it's a bit of a readability surprise. The existing per-method wrapper functions (_embedding_create_wrapper, etc.) aren't converted to use the factory, which leaves a small inconsistency. Neither is a blocker, but if the factory pattern proves useful it might be worth a follow-up to unify the three inline wrappers.

No coverage for response_format="text" — when the caller passes response_format="text", OpenAI returns a plain str rather than a JSON object. The isinstance(response, str) branch in _extract_text handles this, but there's no test for it. A cassette for this path would be a good addition.


Summary

The core logic is correct and all existing + new tests pass. I'd address the _extract_text fallback (issue 1) and strengthen the transcription/translation output assertions (issue 2) before merge. Everything else is minor.

- Replace str(response) fallback with getattr(response, 'text', None) so
Pydantic model responses (Transcription, Translation, etc.) are handled
correctly instead of logging repr strings
- Tighten transcription/translation span output assertions from
`is not None` to the actual fixture value "you"
- Add test_openai_audio_transcription_text_format covering the
response_format="text" path (plain-string response from Whisper)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Whisper's response_format="text" response ends with \n. Strip it so the
logged span output is consistent with the json format (which returns the
text field without trailing whitespace).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) merged commit 45e4de1 into mainApr 9, 2026
59 checks passed
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) deleted the feat/audio-api-instrumentation branch April 9, 2026 18:11
Abhijeet Prasad (AbhiPrasad) added a commit that referenced this pull request Apr 10, 2026
Sign up for freeto 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.

OpenAI: Audio API (client.audio.speech, transcriptions, translations) not instrumented

2 participants

@AbhiPrasad@Qard
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

feat(openai): instrument Audio API (speech, transcriptions, translations) - #232

Merged
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation
Apr 9, 2026
Merged

feat(openai): instrument Audio API (speech, transcriptions, translations)#232
Abhijeet Prasad (AbhiPrasad) merged 12 commits into
mainfrom
feat/audio-api-instrumentation

Conversation

@AbhiPrasad

Copy link
Copy Markdown
Member

Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).

Closes#174

…ons)
Add tracing wrappers for OpenAI's three Audio API methods so that
wrap_openai() and OpenAIIntegration.setup() produce Braintrust spans
for audio.speech.create, audio.transcriptions.create, and
audio.translations.create (sync + async).
Closes#174
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_AudioFileWrapper._parse_params was popping 'file' from the original
kwargs before prettify_params made a copy, causing the actual API call
to lose the required file argument.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace 3 copy-pasted wrapper callbacks with _make_base_wrapper_callback factory
- Extract shared text extraction logic into _AudioFileWrapper._extract_text
- Use context managers for file handles in audio tests
- Remove unnecessary comment in SpeechWrapper.process_output
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@starfolkai

starfolkaiBot commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Drop into this review session: sfk devbox session 68

PR #232 — Audio API Instrumentation: Code Review

Verdict: Approve with minor suggestions

All 438 tests pass (nox -s "test_openai(latest)"), including the 6 new audio tests. The implementation is clean and consistent with the repo's established patterns.


What's good

  • Follows existing conventions exactly: VCR cassettes, sync/async patcher pairs, registration in both integration.py (for setup()) and _WRAP_TARGETS (for wrap_openai()). The test_wrap_openai_and_setup_use_same_wrappers enforcement is satisfied.
  • _AudioFileWrapper._parse_params correctly prevents logging binary audio data by popping file from the prettified copy of params, so the original kwargs passed to the API call are unaffected.
  • _make_base_wrapper_callback is a reasonable factory that reduces the repetitive sync/async branching seen in the inline wrappers for chat/embeddings/moderation.
  • SpeechWrapper logging {"type": "audio"} for output is the right call — streaming binary audio into a span would be impractical.
  • TranscriptionWrapper correctly handles the {"type":"duration","seconds":1} usage shape from Whisper — _parse_metrics_from_usage returns an empty dict gracefully when token fields are absent.

Issues

1. _extract_text fallback can silently produce wrong output (minor bug)

tracing.py, _AudioFileWrapper._extract_text:

@staticmethoddef_extract_text(response: Any) ->str|None:
ifisinstance(response, dict):
returnresponse.get("text")
ifisinstance(response, str):
returnresponsereturnstr(response) # ← problem here

When _try_to_dict can't convert the response to a dict (e.g., an object type it doesn't recognize), and the response isn't a plain string, the fallback str(response) will return something like Transcription(text='hello world', logprobs=None, usage=None) — Pydantic's repr — instead of the actual transcribed text. This won't crash, but the span output would be garbage.

A safer fallback:

returngetattr(response, "text", None)

This handles OpenAI Pydantic model types (Transcription, TranscriptionVerbose, Translation, etc.) that have a .text attribute directly, without relying on dict conversion.


2. Weak test assertions for transcription/translation

assertspan["output"] isnotNone

For transcription and translation tests, the output is never checked beyond being non-None. Given that the cassette response is {"text":"you","usage":{"type":"duration","seconds":1}}, you can assert the actual value:

assertspan["output"] =="you"

This would have caught the _extract_text fallback issue at test-recording time if anything went wrong.


Nits / observations (non-blocking)

_make_base_wrapper_callback is placed ~600 lines before BaseWrapper — the string annotation wrapper_cls: type["BaseWrapper"] is correct, but it's a bit of a readability surprise. The existing per-method wrapper functions (_embedding_create_wrapper, etc.) aren't converted to use the factory, which leaves a small inconsistency. Neither is a blocker, but if the factory pattern proves useful it might be worth a follow-up to unify the three inline wrappers.

No coverage for response_format="text" — when the caller passes response_format="text", OpenAI returns a plain str rather than a JSON object. The isinstance(response, str) branch in _extract_text handles this, but there's no test for it. A cassette for this path would be a good addition.


Summary

The core logic is correct and all existing + new tests pass. I'd address the _extract_text fallback (issue 1) and strengthen the transcription/translation output assertions (issue 2) before merge. Everything else is minor.

- Replace str(response) fallback with getattr(response, 'text', None) so
Pydantic model responses (Transcription, Translation, etc.) are handled
correctly instead of logging repr strings
- Tighten transcription/translation span output assertions from
`is not None` to the actual fixture value "you"
- Add test_openai_audio_transcription_text_format covering the
response_format="text" path (plain-string response from Whisper)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Whisper's response_format="text" response ends with \n. Strip it so the
logged span output is consistent with the json format (which returns the
text field without trailing whitespace).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) merged commit 45e4de1 into mainApr 9, 2026
59 checks passed
@AbhiPrasad
Abhijeet Prasad (AbhiPrasad) deleted the feat/audio-api-instrumentation branch April 9, 2026 18:11
Abhijeet Prasad (AbhiPrasad) added a commit that referenced this pull request Apr 10, 2026
Sign up for freeto 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.

OpenAI: Audio API (client.audio.speech, transcriptions, translations) not instrumented

2 participants

@AbhiPrasad@Qard