Conversation
Contributor
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
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.
Why
This PR does not upload images from
input.value. It adds a recording gate so a later provider callback can do that work only after the sampler keeps the span.Provider instrumentors serialize
input.valueas a JSON copy of the request. That copy is a different attribute from the flattenedllm.input_messages.*.image.urlkeys that #3409 already made recording-safe.TraceConfig.mask()never walks JSON, so callingBlobUploaderwhile buildinginput.valuewould upload before the sampler decides whether the span is kept. This PR addsOITracer.start_span(..., input_value=callback)so providers finishinput.valueonly after the span is recording.TraceConfig.externalize_blob()is the shared upload-or-redact helper for already-decoded bytes. The existing data-URI mask path now uses it too.#3717 is the first consumer. It passes
input_value=lambda: serialize_request_input(...). That JSON walk is not in this PR.The two copies of the same image
This PR only opens a safe seam for the
input.valuecopy. It does not walk provider JSON and it does not upload frominput.valueby itself. Follow-up PRs (OpenAI #3717, then Anthropic and Google GenAI) pass the callback.Before
Naive upload inside the provider serializer, which is the pattern this seam exists to stop. On
maintoday OpenAI redacts oversized images in that serializer and does not upload. There is still no recording-safe place to upload frominput.value.Flattened image keys still upload later at
finish_tracingwhen the span is recording.After
The callback is where a provider may walk JSON, honor
hide_input_images, and callTraceConfig.externalize_blob(). Core does not inspect image leaves.hide_input_imagesis a provider-callback concern. Core only skips the callback whenhide_inputsis true or the span is not recording.externalize_blob()Blob.modalityis"image". Failures never raise into the instrumented SDK call.Scope
input_valuecallbacks toOITracer.start_span()andOITracer.start_as_current_span().TraceConfig.externalize_blob()and reuse it for existing data URI masking.Tradeoffs
The sampler receives
__REDACTED__instead of serialized provider input. This avoids provider traversal and upload work before the recording decision.The callback is specific to
input.value. This keeps ordinary OpenTelemetry attribute values and masking contracts unchanged.Blast radius
The new callback and upload helper are opt-in. Existing spans and flattened image attributes keep their current behavior.
Verification
uvx tox run -e ruff-mypy-test-instrumentationpassed formatting, Ruff, mypy, and 8,524 tests.Closes #3537