Skip to content

fix(openai): Support report token usage in stream mode #661

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.

Sign up for GitHub

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

Jump to bottom
Merged
nirga merged 9 commits into from
Mar 24, 2024
Merged

fix(openai): Support report token usage in stream mode #661

nirga merged 9 commits into from
Mar 24, 2024

Conversation

@Humbertzhang
Copy link
Contributor

@Humbertzhang Humbertzhang commented Mar 19, 2024 •
edited by nirga
Loading

Fixes #627

  • I have added tests that cover my changes.
  • If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
  • For Traces:
image
  • For Metrics:
image

@Humbertzhang
feat: add token usage in openai stream chat & completion
10b394f
@Humbertzhang
rollback unintended change
aeb69ee
@Humbertzhang
Merge branch 'main' of https://github.com/Humbertzhang/openllmetry in…
a4f4a70 
…to stream_token

# Conflicts:
#	packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py
#	packages/opentelemetry-instrumentation-openai/poetry.lock
#	packages/opentelemetry-instrumentation-openai/pyproject.toml
#	packages/opentelemetry-instrumentation-openai/tests/metrics/test_openai_metrics.py
#	packages/opentelemetry-instrumentation-openai/tests/traces/test_chat.py
#	packages/opentelemetry-instrumentation-openai/tests/traces/test_completions.py
@Humbertzhang
update poetry files
fc1ce27
@Humbertzhang
fix ci error
91ab863
@Humbertzhang
fix ci error
13a3dee
@Humbertzhang
Copy link
Contributor Author

Humbertzhang commented Mar 19, 2024 •
edited
Loading

CI error:

  E               vcr.errors.CannotOverwriteExistingCassetteException: Can't overwrite existing cassette ('/home/runner/work/openllmetry/openllmetry/packages/opentelemetry-instrumentation-openai/tests/traces/cassettes/test_chat/test_chat_streaming.yaml') in your current record mode ('none').
  E               No match for the request (<Request (GET) https://openaipublic.blob.core.windows.net/encodings/cl100k_base.tiktoken>) was found.

tiktoken need to download file at it first run, so vcr's none record mode maybe not suite for it...

@nirga Do u have some thoughts about it?

@nirga
Copy link
Member

nirga commented Mar 19, 2024 •
edited
Loading

@Humbertzhang you can tell VCR to ignore certain hostnames, then it will not try and replay the cassette for those host names, but actually try to make an HTTP call, see this example -

openllmetry/packages/opentelemetry-instrumentation-llamaindex/tests/conftest.py

Line 50 in 373f5c7

"ignore_hosts": ["raw.githubusercontent.com"],

@nirga nirga changed the title feat(openai): Support report token usage in stream mode fix(openai): Support report token usage in stream mode Mar 19, 2024
nirga
nirga reviewed Mar 19, 2024
View reviewed changes
...opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/__init__.py
return model


def should_record_stream_token_usage():
Copy link
Member

@nirga nirga Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put it as an optional parameter to the __init__ of the instrumentation (I'd default it to true)? I think it's cleaner (and I want to move the should_trace_content to be like that as well)

Copy link
Contributor Author

@Humbertzhang Humbertzhang Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean move it to Traceloop.init()'s arg, right?

Copy link
Member

@nirga nirga Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the instrumentation init. Then traceloop sdk can pass it yes

@Humbertzhang
Merge branch 'main' of https://github.com/Humbertzhang/openllmetry in…
cf1eea0 
…to stream_token

# Conflicts:
#	packages/opentelemetry-instrumentation-langchain/poetry.lock
#	packages/opentelemetry-instrumentation-llamaindex/poetry.lock
#	packages/opentelemetry-instrumentation-openai/poetry.lock
#	packages/opentelemetry-instrumentation-pinecone/poetry.lock
#	packages/opentelemetry-instrumentation-weaviate/poetry.lock
@Humbertzhang
Add vcr config
57c6c47
@Humbertzhang
Update poetry.lock files
25a5d57
@Humbertzhang
Copy link
Contributor Author

Humbertzhang commented Mar 24, 2024

Hello @nirga, I have resolved the VCR issue by adding ignore_hosts. However, I'm not entirely sure what you mean by "put it as an optional parameter to the __init__ of the instrumentation."

Could you provide me an example? Alternatively, we could just merge this, and I can make modifications in a later PR after you've updated should_trace_content.

@nirga
Copy link
Member

nirga commented Mar 24, 2024

@Humbertzhang sure, thanks!

@nirga nirga merged commit 17d1d34 into traceloop:main Mar 24, 2024
This was referenced May 7, 2025
Closed
Closed
nina-kollman pushed a commit that referenced this pull request Aug 11, 2025
@Humbertzhang
fix(openai): Support report token usage in stream mode (#661)
62f3ac2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nirga nirga nirga approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

🐛 Bug Report: Number of tokens not reported when streaming OpenAI/AOAI

2 participants

@Humbertzhang @nirga