Python: Add OpenTelemetry to Python SK #6914

glahaye · 2024-06-23T23:12:43Z

Motivation and Context

We want observability into usage of SK

Description

Add OpenTelemetry to Python SK

Contribution Checklist

The code builds clean without any errors or warnings
The PR follows the SK Contribution Guidelines and the pre-submission formatting script raises no violations

markwallace-microsoft · 2024-06-23T23:13:17Z

Python 3.10 Test Coverage Report •

File	Stmts	Miss	Cover	Missing
semantic_kernel/connectors/ai/open_ai/services
open_ai_chat_completion_base.py	168	70	58%	100, 104, 124, 149–153, 178, 186, 188, 192, 210–215, 233–261, 264–275, 290–297, 308–316, 332–339, 360, 368, 374–377, 389–392, 423
TOTAL	6681	773	88%

Python 3.10 Unit Test Overview

Tests	Skipped	Failures	Errors	Time
1567	1 💤	0 ❌	0 🔥	26.008s ⏱️

python/poetry.lock

eavanvalkenburg

Some work to do, the basics are there, but most important is to refactor this into a decorator that we can use with all completion services and functions!

python/semantic_kernel/connectors/ai/open_ai/services/open_ai_chat_completion_base.py

python/semantic_kernel/utils/model_diagnostics.py

eavanvalkenburg · 2024-06-24T09:36:21Z

python/semantic_kernel/connectors/ai/open_ai/services/open_ai_chat_completion_base.py

@@ -269,9 +276,25 @@ def _chat_message_content_to_dict(self, message: "ChatMessageContent") -> dict[s

    async def _send_chat_request(self, settings: OpenAIChatPromptExecutionSettings) -> list["ChatMessageContent"]:
        """Send the chat request."""
+        span = model_diagnostics.start_completion_activity(settings.ai_model_id, MODEL_PROVIDER_NAME,


we should put all this and the bottom part into a single decorator so that we can universally apply this across all connectors!

I don't know how feasible or convoluted this can be to achieve as there will be differences between connectors. I am certainly checking right now.

Does this need to be part of this PR, or can it be in a follow-up one?

eavanvalkenburg · 2024-06-24T09:36:53Z

python/semantic_kernel/utils/model_diagnostics.py

+                       {COMPLETION_EVENT_COMPLETION: _messages_to_openai_format(completions)})
+
+
+def _messages_to_openai_format(chat_history: list[ChatMessageContent]) -> str:


ideally this generic function should not have openai specific functions...

This is needed since the OTeL GenAI semantic convention recommends the OpenAI format: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/gen-ai/gen-ai-spans.md#events

then let's name it _messages_to_otel_format!

python/semantic_kernel/utils/model_diagnostics.py

python/semantic_kernel/connectors/ai/open_ai/services/open_ai_chat_completion_base.py

python/semantic_kernel/utils/model_diagnostics.py

python/semantic_kernel/connectors/ai/open_ai/services/open_ai_chat_completion_base.py

…chat_completion_base.py Co-authored-by: Tao Chen <[email protected]>

python/semantic_kernel/connectors/ai/open_ai/services/open_ai_chat_completion_base.py

eavanvalkenburg

I see both naming with diagnostics (env setting and model_diagnostics.py file) and tracing, let's make that more consistent! And we really need to create a decorator out of the code inside openai_chat_completion_base!

eavanvalkenburg · 2024-06-28T09:38:02Z

python/semantic_kernel/connectors/ai/open_ai/services/open_ai_chat_completion_base.py

@@ -279,9 +288,39 @@ def _chat_message_content_to_dict(self, message: "ChatMessageContent") -> dict[s

    async def _send_chat_request(self, settings: OpenAIChatPromptExecutionSettings) -> list["ChatMessageContent"]:
        """Send the chat request."""
-        response = await self._send_request(request_settings=settings)
+        span = start_completion_activity(


this still needs to be put into a decorator, I don't like this code here!

eavanvalkenburg · 2024-06-28T09:39:40Z

python/semantic_kernel/connectors/ai/open_ai/services/open_ai_chat_completion_base.py

@@ -301,9 +340,18 @@ async def _send_chat_stream_request(
    # endregion
    # region content creation

+    def _input_messages_to_prompt(self, messages: list[dict[str, Any]]) -> str:


same for this, we have multiple ways of create a list of dict from messages, and settings have a list of dicts with the messages already here, i would argue that that is the list we want to log!

eavanvalkenburg · 2024-06-28T09:40:51Z

python/semantic_kernel/utils/model_diagnostics.py

+                       {COMPLETION_EVENT_COMPLETION: _messages_to_openai_format(completions)})
+
+
+def _messages_to_openai_format(chat_history: list[ChatMessageContent]) -> str:


then let's name it _messages_to_otel_format!

eavanvalkenburg · 2024-06-28T09:41:44Z

python/semantic_kernel/utils/model_diagnostics.py

@@ -0,0 +1,160 @@
+# Copyright (c) Microsoft. All rights reserved.


this probably should also move into the utils/tracing folder.

loggin

22204aa

glahaye requested a review from a team as a code owner June 23, 2024 23:12

markwallace-microsoft added the python Pull requests for the Python Semantic Kernel label Jun 23, 2024

github-actions bot changed the title ~~Add OpenTelemetry to Python SK~~ Python: Add OpenTelemetry to Python SK Jun 23, 2024

Merge main

1156b46

glahaye commented Jun 24, 2024

View reviewed changes

python/poetry.lock Outdated Show resolved Hide resolved

eavanvalkenburg requested changes Jun 24, 2024

View reviewed changes

Merge branch 'main' into python_telemetry

e5ed784

TaoChenOSU reviewed Jun 24, 2024

View reviewed changes

alliscode reviewed Jun 24, 2024

View reviewed changes

python/semantic_kernel/connectors/ai/open_ai/services/open_ai_chat_completion_base.py Outdated Show resolved Hide resolved

glahaye and others added 3 commits June 24, 2024 11:45

Update python/semantic_kernel/connectors/ai/open_ai/services/open_ai_…

a2c5fd7

…chat_completion_base.py Co-authored-by: Tao Chen <[email protected]>

Addressed PR issues

2848051

Merge main

fe86266

glahaye requested review from TaoChenOSU, eavanvalkenburg and alliscode June 27, 2024 21:46

glahaye added 2 commits June 27, 2024 14:56

Merge main

e9dd311

Update poetry info after merge

4f3783d

alliscode reviewed Jun 27, 2024

View reviewed changes

python/semantic_kernel/connectors/ai/open_ai/services/open_ai_chat_completion_base.py Outdated Show resolved Hide resolved

alliscode reviewed Jun 27, 2024

View reviewed changes

python/semantic_kernel/connectors/ai/open_ai/services/open_ai_chat_completion_base.py Outdated Show resolved Hide resolved

alliscode reviewed Jun 27, 2024

View reviewed changes

python/semantic_kernel/connectors/ai/open_ai/services/open_ai_chat_completion_base.py Show resolved Hide resolved

glahaye added 3 commits June 27, 2024 16:03

Address PR issues

e71fab2

Fix lint warning

6bf6406

Fix false positive security issue

00269b9

eavanvalkenburg requested changes Jun 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python: Add OpenTelemetry to Python SK #6914

Python: Add OpenTelemetry to Python SK #6914

glahaye commented Jun 23, 2024

markwallace-microsoft commented Jun 23, 2024 •

edited

Loading

eavanvalkenburg left a comment

eavanvalkenburg Jun 24, 2024

glahaye Jun 27, 2024

eavanvalkenburg Jun 24, 2024

TaoChenOSU Jun 24, 2024

eavanvalkenburg Jun 28, 2024

eavanvalkenburg left a comment

eavanvalkenburg Jun 28, 2024

eavanvalkenburg Jun 28, 2024

eavanvalkenburg Jun 28, 2024

eavanvalkenburg Jun 28, 2024

		{COMPLETION_EVENT_COMPLETION: _messages_to_openai_format(completions)})


		def _messages_to_openai_format(chat_history: list[ChatMessageContent]) -> str:

		@@ -0,0 +1,160 @@
		# Copyright (c) Microsoft. All rights reserved.

Python: Add OpenTelemetry to Python SK #6914

Are you sure you want to change the base?

Python: Add OpenTelemetry to Python SK #6914

Conversation

glahaye commented Jun 23, 2024

Motivation and Context

Description

Contribution Checklist

markwallace-microsoft commented Jun 23, 2024 • edited Loading

Python 3.10 Unit Test Overview

eavanvalkenburg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eavanvalkenburg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markwallace-microsoft commented Jun 23, 2024 •

edited

Loading