address review comments

openai · Jul 26, 2024 · 044cb44 · 044cb44
1 parent 872738a
commit 044cb44
Show file tree

Hide file tree

Showing 10 changed files with 110 additions and 102 deletions.
diff --git a/README.md b/README.md
@@ -27,9 +27,7 @@ It is generated from our [OpenAPI specification](https://github.com/openai/opena
 - [Advanced scenarios](#advanced-scenarios)
   - [Using protocol methods](#using-protocol-methods)
   - [Automatically retrying errors](#automatically-retrying-errors)
-- [Observability with OpenTelemetry](#observability-with-opentelemetry)
-  - [How to enable](#how-to-enable)
-  - [Available sources and meters](#available-sources-and-meters)
+- [Observability](#observability)
 
 ## Getting started
 
@@ -732,56 +730,6 @@ By default, the client classes will automatically retry the following errors up
 - 503 Service Unavailable
 - 504 Gateway Timeout
 
-## Observability with OpenTelemetry
+## Observability
 
-> Note:
-> OpenAI .NET SDK instrumentation is in development and is not complete. See [Available sources and meters](#available-sources-and-meters) section for the list of covered operations.
-
-OpenAI .NET library is instrumented with distributed tracing and metrics using .NET [tracing](https://learn.microsoft.com/dotnet/core/diagnostics/distributed-tracing)
-and [metrics](https://learn.microsoft.com/dotnet/core/diagnostics/metrics-instrumentation) API and supports [OpenTelemetry](https://learn.microsoft.com/dotnet/core/diagnostics/observability-with-otel).
-
-OpenAI .NET library follows [OpenTelemetry Semantic Conventions for Generative AI systems](https://github.com/open-telemetry/semantic-conventions/tree/main/docs/gen-ai).
-
-### How to enable
-
-The instrumentation is **experimental** - names of activity sources and meters, volume and semantics of the telemetry items may change.
-
-To enable the instrumentation:
-
-1. Set instrumentation feature-flag using one of the following options:
-
-   - set the `AZURE_EXPERIMENTAL_ENABLE_ACTIVITY_SOURCE` environment variable to `"true"`
-   - set the `Azure.Experimental.EnableActivitySource` context switch to true in your application code when application
-     is starting and before initializing any OpenAI clients. For example:
-
-     ```csharp
-     AppContext.SetSwitch("Azure.Experimental.EnableActivitySource", true);
-     ```
-
-2. Configuring OpenTelemetry to record telemetry from OpenAI sources and meters:
-
-   ```csharp
-   builder.Services.AddOpenTelemetry()
-       .WithTracing(b =>
-       {
-           b.AddSource("OpenAI.*")
-             ...
-            .AddOtlpExporter();
-       })
-       .WithMetrics(b =>
-       {
-           b.AddMeter("OpenAI.*")
-            ...
-            .AddOtlpExporter();
-       });
-   ```
-
-Consider enabling [HTTP client instrumentation](https://www.nuget.org/packages/OpenTelemetry.Instrumentation.Http) to see all HTTP client
-calls made by your application including those done by the OpenAI SDK.
-Check out [OpenTelemetry documentation](https://opentelemetry.io/docs/languages/net/getting-started/) for more details.
-
-### Available sources and meters
-
-The following sources and meters are available:
-
-- `OpenAI.ChatClient` - records traces and metrics for `ChatClient` operations (except streaming and protocol methods which are not instrumented yet)
+OpenAI .NET library supports experimental distributed tracing and metrics with OpenTelemetry. Check out [Observability with OpenTelemetry](./docs/observability.md) for more details.
diff --git a/docs/observability.md b/docs/observability.md
@@ -0,0 +1,57 @@
+## Observability with OpenTelemetry
+
+> Note:
+> OpenAI .NET SDK instrumentation is in development and is not complete. See [Available sources and meters](#available-sources-and-meters) section for the list of covered operations.
+
+OpenAI .NET library is instrumented with distributed tracing and metrics using .NET [tracing](https://learn.microsoft.com/dotnet/core/diagnostics/distributed-tracing)
+and [metrics](https://learn.microsoft.com/dotnet/core/diagnostics/metrics-instrumentation) API and supports [OpenTelemetry](https://learn.microsoft.com/dotnet/core/diagnostics/observability-with-otel).
+
+OpenAI .NET instrumentation follows [OpenTelemetry Semantic Conventions for Generative AI systems](https://github.com/open-telemetry/semantic-conventions/tree/main/docs/gen-ai).
+
+### How to enable
+
+The instrumentation is **experimental** - volume and semantics of the telemetry items may change.
+
+To enable the instrumentation:
+
+1. Set instrumentation feature-flag using one of the following options:
+
+   - set the `OPENAI_EXPERIMENTAL_ENABLE_INSTRUMENTATION` environment variable to `"true"`
+   - set the `OpenAI.Experimental.EnableInstrumentation` context switch to true in your application code when application
+     is starting and before initializing any OpenAI clients. For example:
+
+     ```csharp
+     AppContext.SetSwitch("OpenAI.Experimental.EnableInstrumentation", true);
+     ```
+
+2. Enable OpenAI telemetry:
+
+   ```csharp
+   builder.Services.AddOpenTelemetry()
+       .WithTracing(b =>
+       {
+           b.AddSource("OpenAI.*")
+             ...
+            .AddOtlpExporter();
+       })
+       .WithMetrics(b =>
+       {
+           b.AddMeter("OpenAI.*")
+            ...
+            .AddOtlpExporter();
+       });
+   ```
+
+   Distributed tracing is enabled with `AddSource("OpenAI.*")` which tells OpenTelemetry to listen to all [ActivitySources](https://learn.microsoft.com/dotnet/api/system.diagnostics.activitysource) with names starting with `OpenAI.*`.
+
+   Similarly, metrics are configured with `AddMeter("OpenAI.*")` which enables all OpenAI-related [Meters](https://learn.microsoft.com/dotnet/api/system.diagnostics.metrics.meter).
+
+Consider enabling [HTTP client instrumentation](https://www.nuget.org/packages/OpenTelemetry.Instrumentation.Http) to see all HTTP client
+calls made by your application including those done by the OpenAI SDK.
+Check out [OpenTelemetry documentation](https://opentelemetry.io/docs/languages/net/getting-started/) for more details.
+
+### Available sources and meters
+
+The following sources and meters are available:
+
+- `OpenAI.ChatClient` - records traces and metrics for `ChatClient` operations (except streaming and protocol methods which are not instrumented yet)
diff --git a/src/Custom/Chat/ChatClient.cs b/src/Custom/Chat/ChatClient.cs
@@ -1,4 +1,4 @@
-using OpenAI.Custom.Common.Instrumentation;
+using OpenAI.Instrumentation;
 using System;
 using System.ClientModel;
 using System.ClientModel.Primitives;
@@ -225,7 +225,7 @@ private void CreateChatCompletionOptions(IEnumerable<ChatMessage> messages, ref
     {
         options.Messages = messages.ToList();
         options.Model = _model;
-        options.Stream = stream 
+        options.Stream = stream
             ? true
             : null;
         options.StreamOptions = stream ? options.StreamOptions : null;

diff --git a/...ustom/Common/Instrumentation/Constants.cs → src/Utility/Instrumentation/Constants.cs b/...ustom/Common/Instrumentation/Constants.cs → src/Utility/Instrumentation/Constants.cs
@@ -1,4 +1,4 @@
-namespace OpenAI.Custom.Common.Instrumentation;
+namespace OpenAI.Instrumentation;
 
 internal class Constants
 {
@@ -20,7 +20,7 @@ internal class Constants
     public const string GenAiRequestTopPKey = "gen_ai.request.top_p";
 
     public const string GenAiResponseIdKey = "gen_ai.response.id";
-    public const string GenAiResponseFinishReasonKey = "gen_ai.response.finish_reason";
+    public const string GenAiResponseFinishReasonKey = "gen_ai.response.finish_reasons";
     public const string GenAiResponseModelKey = "gen_ai.response.model";
 
     public const string GenAiSystemKey = "gen_ai.system";

diff --git a/...Instrumentation/InstrumentationFactory.cs → ...Instrumentation/InstrumentationFactory.cs b/...Instrumentation/InstrumentationFactory.cs → ...Instrumentation/InstrumentationFactory.cs
@@ -1,7 +1,7 @@
 using OpenAI.Chat;
 using System;
 
-namespace OpenAI.Custom.Common.Instrumentation;
+namespace OpenAI.Instrumentation;
 
 internal class InstrumentationFactory
 {
@@ -22,7 +22,7 @@ public InstrumentationFactory(string model, Uri endpoint)
 
     public InstrumentationScope StartChatScope(ChatCompletionOptions completionsOptions)
     {
-        return IsInstrumentationEnabled 
+        return IsInstrumentationEnabled
             ? InstrumentationScope.StartChat(_model, ChatOperationName, _serverAddress, _serverPort, completionsOptions)
             : null;
     }

diff --git a/...n/Instrumentation/InstrumentationScope.cs → ...y/Instrumentation/InstrumentationScope.cs b/...n/Instrumentation/InstrumentationScope.cs → ...y/Instrumentation/InstrumentationScope.cs
@@ -1,15 +1,18 @@
 using OpenAI.Chat;
 using System;
+using System.Buffers;
 using System.ClientModel;
 using System.Diagnostics;
 using System.Diagnostics.Metrics;
 
-namespace OpenAI.Custom.Common.Instrumentation;
+namespace OpenAI.Instrumentation;
 
 internal class InstrumentationScope : IDisposable
 {
     private static readonly ActivitySource s_chatSource = new ActivitySource("OpenAI.ChatClient");
     private static readonly Meter s_chatMeter = new Meter("OpenAI.ChatClient");
+
+    // TODO: add explicit histogram buckets once System.Diagnostics.DiagnosticSource 9.0 is used
     private static readonly Histogram<double> s_duration = s_chatMeter.CreateHistogram<double>(Constants.GenAiClientOperationDurationMetricName, "s", "Measures GenAI operation duration.");
     private static readonly Histogram<long> s_tokens = s_chatMeter.CreateHistogram<long>(Constants.GenAiClientTokenUsageMetricName, "{token}", "Measures the number of input and output token used.");
 
@@ -83,7 +86,11 @@ public void RecordException(Exception ex)
     {
         var errorType = GetErrorType(ex);
         RecordMetrics(null, errorType, null, null);
-        SetActivityError(ex, errorType);
+        if (_activity?.IsAllDataRequested == true)
+        {
+            _activity?.SetTag(Constants.ErrorTypeKey, errorType);
+            _activity?.SetStatus(ActivityStatusCode.Error, ex?.Message ?? errorType);
+        }
     }
 
     public void Dispose()
@@ -102,64 +109,67 @@ private void RecordCommonAttributes()
 
     private void RecordMetrics(string responseModel, string errorType, int? inputTokensUsage, int? outputTokensUsage)
     {
-        TagList tags = ResponseTagsWithError(responseModel, errorType);
-        s_duration.Record(_duration.Elapsed.TotalSeconds, tags);
+        // tags is a struct, let's copy and modify them
+        var tags = _commonTags;
+
+        if (responseModel != null)
+        {
+            tags.Add(Constants.GenAiResponseModelKey, responseModel);
+        }
 
         if (inputTokensUsage != null)
         {
-            // tags is a struct, let's copy them
-            TagList inputUsageTags = tags;
+            var inputUsageTags = tags;
             inputUsageTags.Add(Constants.GenAiTokenTypeKey, "input");
             s_tokens.Record(inputTokensUsage.Value, inputUsageTags);
         }
 
         if (outputTokensUsage != null)
-        { 
-            TagList outputUsageTags = tags;
+        {
+            var outputUsageTags = tags;
             outputUsageTags.Add(Constants.GenAiTokenTypeKey, "output");
-
             s_tokens.Record(outputTokensUsage.Value, outputUsageTags);
         }
-    }
-
-    private TagList ResponseTagsWithError(string responseModel, string errorType)
-    {
-        // tags is a struct, let's copy them
-        var tags = _commonTags;
-
-        if (responseModel != null)
-        {
-            tags.Add(Constants.GenAiResponseModelKey, responseModel);
-        }
 
         if (errorType != null)
         {
             tags.Add(Constants.ErrorTypeKey, errorType);
         }
 
-        return tags;
+        s_duration.Record(_duration.Elapsed.TotalSeconds, tags);
     }
 
     private void RecordResponseAttributes(string responseId, string model, ChatFinishReason? finishReason, ChatTokenUsage usage)
     {
         SetActivityTagIfNotNull(Constants.GenAiResponseIdKey, responseId);
         SetActivityTagIfNotNull(Constants.GenAiResponseModelKey, model);
-        SetActivityTagIfNotNull(Constants.GenAiResponseFinishReasonKey, GetFinishReason(finishReason));
         SetActivityTagIfNotNull(Constants.GenAiUsageInputTokensKey, usage?.InputTokens);
         SetActivityTagIfNotNull(Constants.GenAiUsageOutputTokensKey, usage?.OutputTokens);
+        SetFinishReasonAttribute(finishReason);
     }
 
-    private string GetFinishReason(ChatFinishReason? reason) =>
-        reason switch
+    private void SetFinishReasonAttribute(ChatFinishReason? finishReason)
+    {
+        if (finishReason == null)
+        {
+            return;
+        }
+
+        var reasonStr = finishReason switch
         {
             ChatFinishReason.ContentFilter => "content_filter",
             ChatFinishReason.FunctionCall => "function_call",
             ChatFinishReason.Length => "length",
             ChatFinishReason.Stop => "stop",
             ChatFinishReason.ToolCalls => "tool_calls",
-            _ => reason?.ToString(),
+            _ => finishReason.ToString(),
         };
 
+        // There could be multiple finish reasons, so semantic conventions use array type for the corrresponding attribute.
+        // It's likely to change, but for now let's report it as array.
+        _activity.SetTag(Constants.GenAiResponseFinishReasonKey, new[] { reasonStr });
+    }
+
     private string GetChatMessageRole(ChatMessageRole? role) =>
         role switch
         {
@@ -175,23 +185,14 @@ private string GetErrorType(Exception exception)
     {
         if (exception is ClientResultException requestFailedException)
         {
-            // TODO (limolkova) when we start targeting .NET 8 we should put
+            // TODO (lmolkova) when we start targeting .NET 8 we should put
             // requestFailedException.InnerException.HttpRequestError into error.type
             return requestFailedException.Status.ToString();
         }
 
         return exception?.GetType()?.FullName;
     }
 
-    private void SetActivityError(Exception exception, string errorType)
-    {
-        if (exception != null || errorType != null)
-        {
-            _activity?.SetTag(Constants.ErrorTypeKey, errorType);
-            _activity?.SetStatus(ActivityStatusCode.Error, exception?.Message ?? errorType);
-        }
-    }
-
     private void SetActivityTagIfNotNull(string name, object value)
     {
         if (value != null)

diff --git a/tests/Chat/ChatSmokeTests.cs b/tests/Chat/ChatSmokeTests.cs
@@ -6,7 +6,9 @@
 using System;
 using System.ClientModel;
 using System.ClientModel.Primitives;
+using System.Collections.Generic;
 using System.IO;
+using System.Linq;
 using System.Net;
 using System.Text.Json;
 using System.Threading.Tasks;

diff --git a/tests/Instrumentation/ChatInstrumentationTests.cs b/tests/Instrumentation/ChatInstrumentationTests.cs
@@ -1,6 +1,6 @@
 using NUnit.Framework;
 using OpenAI.Chat;
-using OpenAI.Custom.Common.Instrumentation;
+using OpenAI.Instrumentation;
 using System;
 using System.ClientModel.Primitives;
 using System.Collections.Generic;
@@ -62,7 +62,7 @@ public void MetricsOnTracingOff()
         var elapsedMax = Stopwatch.StartNew();
         using var scope = factory.StartChatScope(new ChatCompletionOptions());
         var elapsedMin = Stopwatch.StartNew();
-        
+
         Assert.Null(Activity.Current);
         Assert.NotNull(scope);
 
@@ -149,7 +149,7 @@ public void ChatTracingAllAttributes()
             scope.RecordChatCompletion(chatCompletion);
         }
         Assert.Null(Activity.Current);
-        
+
         ValidateChatActivity(listener.Activities.Single(), chatCompletion, RequestModel, Host, Port);
     }
 
@@ -214,7 +214,7 @@ public async Task ChatTracingAndMetricsMultiple()
         await Task.WhenAll(tasks);
 
         Assert.AreEqual(tasks.Length, activityListener.Activities.Count);
-        
+
         var durations = meterListener.GetMeasurements("gen_ai.client.operation.duration");
         Assert.AreEqual(tasks.Length, durations.Count);
         Assert.AreEqual(numberOfSuccessfulResponses, durations.Count(d => !d.tags.ContainsKey("error.type")));

diff --git a/tests/Instrumentation/TestActivityListener.cs b/tests/Instrumentation/TestActivityListener.cs
@@ -50,7 +50,7 @@ public static void ValidateChatActivity(Activity activity, ChatCompletion respon
         {
             Assert.AreEqual(response.Model, activity.GetTagItem("gen_ai.response.model"));
             Assert.AreEqual(response.Id, activity.GetTagItem("gen_ai.response.id"));
-            Assert.AreEqual(response.FinishReason.ToString().ToLower(), activity.GetTagItem("gen_ai.response.finish_reason"));
+            Assert.AreEqual(new[] { response.FinishReason.ToString().ToLower() }, activity.GetTagItem("gen_ai.response.finish_reasons"));
             Assert.AreEqual(response.Usage.OutputTokens, activity.GetTagItem("gen_ai.usage.output_tokens"));
             Assert.AreEqual(response.Usage.InputTokens, activity.GetTagItem("gen_ai.usage.input_tokens"));
             Assert.AreEqual(ActivityStatusCode.Unset, activity.Status);

diff --git a/tests/OpenAI.Tests.csproj b/tests/OpenAI.Tests.csproj
@@ -17,6 +17,6 @@
   </ItemGroup>
 
   <ItemGroup>
-    <Compile Include="..\src\Custom\Common\Instrumentation\*.cs" LinkBase="Instrumentation\Shared" />
+    <Compile Include="..\src\Utility\Instrumentation\*.cs" LinkBase="Instrumentation\Shared" />
   </ItemGroup>
 </Project>