-
Notifications
You must be signed in to change notification settings - Fork 159
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.
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
feat: basic Support for OpenTelemetry Metrics and Token Usage Metrics in OpenAI V1 #369
Conversation
…i.resources.chat.completions
Nice work @Humbertzhang! Let's connect it to some observability platform to see if it works? |
Sharing some work that's being done now with the OpenTelemetry community that we should align with - traceloop/semantic-conventions#2 |
Hi @nirga! And I think it reports metrics as expect! |
ok @nirga , I will look into this pr align with it ! |
Update to Align with traceloop/semantic-conventions#2Hello @nirga, I have updated my code to align with the changes proposed of You can see the results under normal conditions in the attached image. I have appropriately added attributes such as You can observe the results under exceptional conditions in below images. However, I encountered challenges in retrieving the |
Looks good @Humbertzhang! Before I fully review it, can you:
Reg. your question - can you give an example of when it didn't work for you? |
Hi @nirga !
and I have manually tested all metrics in prometheus. Additionally, I've completed the rebase and addressed all linting issues. Regarding For the metric I look forward to your review and any feedback you may have! |
Hi @nirga , I have updated tests for metrics using VCR, and I hope it meets the format. |
Thanks @Humbertzhang ! Hey @nirga , can we get this merged? I was planning to create a PR to enable watsonx as well, and it will depend on this PR, thanks! |
@gyliu513 yeah probably today / tomorrow. I need to see why the tests are failing, and I want to move this to a common semantic conventions. |
@Humbertzhang looks like there's regression in the streaming test of openai? 🤔 |
Nice work @Humbertzhang! It's a significant milestone for OpenLLMetry :) |
@Humbertzhang thanks for the work here, could you resolve the conflicts so we can merge? :) |
@@ -129,6 +131,13 @@ def _set_response_attributes(span, response): | |||
) | |||
|
|||
|
|||
def _get_openai_base_url(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Is this function still always returning empty string?
I can confirm this behavior if no initialization is done.
In [2]: def _get_openai_base_url():
...: base_url = openai.base_url if hasattr(openai, "base_url") else openai.api_base
...: if not base_url:
...: return ""
...: return base_url
...:
In [3]: import openai
In [4]: _get_openai_base_url()
Out[4]: ''
In [5]:
On the other hand, if you inspect the client instance, you should get the base url:
In [6]: client = openai.OpenAI()
In [7]: client.base_url
Out[7]: URL('https://api.openai.com/v1/')
Does this help you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed your code, sadly I don't see an easy way to extract this URL from an instance in the current instrumentation code.
My only idea was to instrument the client constructor so you can inject a listener which retrieves the URL for you. If you store it in a Singleton, you could retrieve it while calling this function:
def _handle_response(response, span, token_counter=None, choice_counter=None, duration_histogram=None, duration=None):
if is_openai_v1():
response_dict = model_as_dict(response)
else:
response_dict = response
# metrics record
_set_chat_metrics(token_counter, choice_counter, duration_histogram, response_dict, duration)
# span attributes
_set_response_attributes(span, response_dict)
if should_send_prompts():
_set_completions(span, response_dict.get("choices"))
return response
Or maybe even directly inside the _set_chat_metrics
.
But again, quite some effort for just one URL :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nirga what do you think? Should we track this URL as a separate issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paolorechia not sure I follow - I think this function does return the right base URL, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, @Humbertzhang mentioned in a comment, he couldn’t get this URL to work correctly, which why I looked a bit on that
edit: no, I think it’s not working if I understood it correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Humbertzhang I think this is the logic we should use to get it: https://github.com/traceloop/openllmetry/blob/main/packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/__init__.py
(depending on the SDK version, it will be in different attributes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For v1, it should be instance._client.base_url
, and for v0 it should be the code you wrote below
Hi @paolorechia, I resolved conflicts in my local env, but when I run tests of traceloop-sdk and openai, I got
and
errors and exceptions. Have you or @nirga encountered similar exceptions when using VCR? |
@Humbertzhang I'd try to re-generate the cassettes. Comment out the OpenAI mock key from |
Still got connection errors even i can curl request openai... Maybe u can help me with that @nirga ? |
@nirga not pushed successfully just now😅, now it is pushed |
@Humbertzhang All tests pass now 🤩 |
Hi @nirga and @paolorechia, I have fixed the |
Hello all, this is my first PR for issue#251
This PR introduces fundamental components for OpenTelemetry Metrics support, and it add counters for token usage data of
openai.resources.chat.completions
in OpenAI V1.My goal with this PR is to present my approach to addressing the issue at hand and to seek your feedback on the implementation. Your insights and suggestions will be extremely valuable for refining this solution. Moving forward, based on your feedback, I plan to enhance this implementation further and extend support to other instrumentors.
Looking for your reviews and constructive criticism to help improve this integration.