Record content events regardless of span sampling decision #3226
Conversation
github-actions
bot
assigned alizenhom, codefromthecrypt, gyliu513, karthikscale3, lmolkova, lzchen and nirga
github-actions
bot
requested review from
alizenhom,
codefromthecrypt,
gyliu513,
karthikscale3,
lzchen and
nirga
There was a problem hiding this comment.
I think the owners know after this change we'll get the conversion overhead always, and that can be quite large if there's a large context.
Follow-up notes:
There's little to do about this while we still depend on event_logger. Singe event_logger is deprecated, there's no sense in adding a sampling function to it. Since it could be wrapped, testing if it is noop may have limited benefit.
When things move to a logger base api, it could be possible to optimize this to check if
- active span is sampled
- a corresponding log scope is enabled
in either case, do the overhead and when neither, don't.
| @@ -633,6 +633,55 @@ async def test_async_chat_completion_multiple_tools_streaming_no_content( | |||
| ) | |||
|
|
|||
|
|
|||
| @pytest.mark.vcr() | |||
| @pytest.mark.asyncio() | |||
| async def test_async_chat_completion_streaming_sample_out( | |||
There was a problem hiding this comment.
note for later (so feel free to resolve), we can re-use cassettes when the server's response is uninteresting vs another code path. This often exists between "async" and not.
I've made a fixture like this to handle it, which makes the diffs a lot shorter
@pytest.fixture
def vcr_cassette_name(request):
"""
Strips `_async` from the test function name as they use the same data.
"""
# Get the name of the test function
test_name = request.node.name
# Remove '_async' from the test name
cassette_name = test_name.replace("_async", "")
return cassette_name| @@ -633,6 +633,55 @@ async def test_async_chat_completion_multiple_tools_streaming_no_content( | |||
| ) | |||
|
|
|||
|
|
|||
| @pytest.mark.vcr() | |||
| @pytest.mark.asyncio() | |||
| async def test_async_chat_completion_streaming_sample_out( | |||
There was a problem hiding this comment.
dismiss if "sampled out" is a common phrase
| async def test_async_chat_completion_streaming_sample_out( | |
| async def test_async_chat_completion_streaming_unsampled( |
| @@ -659,6 +659,48 @@ def test_chat_completion_multiple_tools_streaming_no_content( | |||
| ) | |||
|
|
|||
|
|
|||
| @pytest.mark.vcr() | |||
| def test_chat_completion_with_content_span_sampled_out( | |||
There was a problem hiding this comment.
again, close out if "sampled out" is common
| def test_chat_completion_with_content_span_sampled_out( | |
| def test_chat_completion_with_content_span_unsampled( |
Fixes #3217
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.